From 5baaac3184ab896d74993825858f1b1a46c460ce Mon Sep 17 00:00:00 2001 From: Christian König Date: Tue, 15 Jun 2021 15:10:03 +0200 Subject: dma-buf: add dma_resv_for_each_fence v3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A simpler version of the iterator to be used when the dma_resv object is locked. v2: fix index check here as well v3: minor coding improvement, some documentation cleanup Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20211006123609.2026-1-christian.koenig@amd.com --- drivers/dma-buf/dma-resv.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index a480af9581bd..2f98caa68ae5 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -423,6 +423,57 @@ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) } EXPORT_SYMBOL(dma_resv_iter_next_unlocked); +/** + * dma_resv_iter_first - first fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the first fence in the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor) +{ + struct dma_fence *fence; + + dma_resv_assert_held(cursor->obj); + + cursor->index = 0; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + + fence = dma_resv_excl_fence(cursor->obj); + if (!fence) + fence = dma_resv_iter_next(cursor); + + cursor->is_restarted = true; + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_iter_first); + +/** + * dma_resv_iter_next - next fence from a locked dma_resv object + * @cursor: cursor to record the current position + * + * Return the next fences from the dma_resv object while holding the + * &dma_resv.lock. + */ +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor) +{ + unsigned int idx; + + dma_resv_assert_held(cursor->obj); + + cursor->is_restarted = false; + if (!cursor->fences || cursor->index >= cursor->fences->shared_count) + return NULL; + + idx = cursor->index++; + return rcu_dereference_protected(cursor->fences->shared[idx], + dma_resv_held(cursor->obj)); +} +EXPORT_SYMBOL_GPL(dma_resv_iter_next); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object -- cgit From 63639d013a6ff202665f0fb7f4d810a5b6d46d79 Mon Sep 17 00:00:00 2001 From: Christian König Date: Thu, 23 Sep 2021 10:28:42 +0200 Subject: dma-buf: use the new iterator in dma_buf_debug_show MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplifying the code a bit. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20211005113742.1101-9-christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 61e20ae7b08b..8242b5d9baeb 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1356,10 +1356,9 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) { struct dma_buf *buf_obj; struct dma_buf_attachment *attach_obj; - struct dma_resv *robj; - struct dma_resv_list *fobj; + struct dma_resv_iter cursor; struct dma_fence *fence; - int count = 0, attach_count, shared_count, i; + int count = 0, attach_count; size_t size = 0; int ret; @@ -1386,21 +1385,10 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) file_inode(buf_obj->file)->i_ino, buf_obj->name ?: ""); - robj = buf_obj->resv; - fence = dma_resv_excl_fence(robj); - if (fence) - seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), - dma_fence_is_signaled(fence) ? "" : "un"); - - fobj = rcu_dereference_protected(robj->fence, - dma_resv_held(robj)); - shared_count = fobj ? fobj->shared_count : 0; - for (i = 0; i < shared_count; i++) { - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(robj)); - seq_printf(s, "\tShared fence: %s %s %ssignalled\n", + dma_resv_for_each_fence(&cursor, buf_obj->resv, true, fence) { + seq_printf(s, "\t%s fence: %s %s %ssignalled\n", + dma_resv_iter_is_exclusive(&cursor) ? + "Exclusive" : "Shared", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un"); -- cgit From 0a42016d9319db24d15789574fe132d8159d7578 Mon Sep 17 00:00:00 2001 From: Christian König Date: Fri, 24 Sep 2021 11:31:22 +0200 Subject: dma-buf: use the new iterator in dma_resv_poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the code a bit. Signed-off-by: Christian König Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20211005113742.1101-10-christian.koenig@amd.com --- drivers/dma-buf/dma-buf.c | 36 ++++++------------------------------ 1 file changed, 6 insertions(+), 30 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 8242b5d9baeb..beb504a92d60 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -209,19 +209,14 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) dma_fence_put(fence); } -static bool dma_buf_poll_shared(struct dma_resv *resv, +static bool dma_buf_poll_add_cb(struct dma_resv *resv, bool write, struct dma_buf_poll_cb_t *dcb) { - struct dma_resv_list *fobj = dma_resv_shared_list(resv); + struct dma_resv_iter cursor; struct dma_fence *fence; - int i, r; - - if (!fobj) - return false; + int r; - for (i = 0; i < fobj->shared_count; ++i) { - fence = rcu_dereference_protected(fobj->shared[i], - dma_resv_held(resv)); + dma_resv_for_each_fence(&cursor, resv, write, fence) { dma_fence_get(fence); r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); if (!r) @@ -232,24 +227,6 @@ static bool dma_buf_poll_shared(struct dma_resv *resv, return false; } -static bool dma_buf_poll_excl(struct dma_resv *resv, - struct dma_buf_poll_cb_t *dcb) -{ - struct dma_fence *fence = dma_resv_excl_fence(resv); - int r; - - if (!fence) - return false; - - dma_fence_get(fence); - r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); - if (!r) - return true; - dma_fence_put(fence); - - return false; -} - static __poll_t dma_buf_poll(struct file *file, poll_table *poll) { struct dma_buf *dmabuf; @@ -282,8 +259,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLOUT) { - if (!dma_buf_poll_shared(resv, dcb) && - !dma_buf_poll_excl(resv, dcb)) + if (!dma_buf_poll_add_cb(resv, true, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else @@ -303,7 +279,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLIN) { - if (!dma_buf_poll_excl(resv, dcb)) + if (!dma_buf_poll_add_cb(resv, false, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else -- cgit From 5e51cc0005c6ed1b793c228632f36269615f7c31 Mon Sep 17 00:00:00 2001 From: Tvrtko Ursulin Date: Fri, 8 Oct 2021 10:50:07 +0100 Subject: dma-resv: Fix dma_resv_get_fences and dma_resv_copy_fences after conversion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cache the count of shared fences in the iterator to avoid dereferencing the dma_resv_object outside the RCU protection. Otherwise iterator and its users can observe an incosistent state which makes it impossible to use safely. Such as: <6> [187.517041] [IGT] gem_sync: executing <7> [187.536343] i915 0000:00:02.0: [drm:i915_gem_context_create_ioctl [i915]] HW context 1 created <7> [187.536793] i915 0000:00:02.0: [drm:i915_gem_context_create_ioctl [i915]] HW context 1 created <6> [187.551235] [IGT] gem_sync: starting subtest basic-many-each <1> [188.935462] BUG: kernel NULL pointer dereference, address: 0000000000000010 <1> [188.935485] #PF: supervisor write access in kernel mode <1> [188.935495] #PF: error_code(0x0002) - not-present page <6> [188.935504] PGD 0 P4D 0 <4> [188.935512] Oops: 0002 [#1] PREEMPT SMP NOPTI <4> [188.935521] CPU: 2 PID: 1467 Comm: gem_sync Not tainted 5.15.0-rc4-CI-Patchwork_21264+ #1 <4> [188.935535] Hardware name: /NUC6CAYB, BIOS AYAPLCEL.86A.0049.2018.0508.1356 05/08/2018 <4> [188.935546] RIP: 0010:dma_resv_get_fences+0x116/0x2d0 <4> [188.935560] Code: 10 85 c0 7f c9 be 03 00 00 00 e8 15 8b df ff eb bd e8 8e c6 ff ff eb b6 41 8b 04 24 49 8b 55 00 48 89 e7 8d 48 01 41 89 0c 24 <4c> 89 34 c2 e8 41 f2 ff ff 49 89 c6 48 85 c0 75 8c 48 8b 44 24 10 <4> [188.935583] RSP: 0018:ffffc900011dbcc8 EFLAGS: 00010202 <4> [188.935593] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000000000001 <4> [188.935603] RDX: 0000000000000010 RSI: ffffffff822e343c RDI: ffffc900011dbcc8 <4> [188.935613] RBP: ffffc900011dbd48 R08: ffff88812d255bb8 R09: 00000000fffffffe <4> [188.935623] R10: 0000000000000001 R11: 0000000000000000 R12: ffffc900011dbd44 <4> [188.935633] R13: ffffc900011dbd50 R14: ffff888113d29cc0 R15: 0000000000000000 <4> [188.935643] FS: 00007f68d17e9700(0000) GS:ffff888277900000(0000) knlGS:0000000000000000 <4> [188.935655] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [188.935665] CR2: 0000000000000010 CR3: 000000012d0a4000 CR4: 00000000003506e0 <4> [188.935676] Call Trace: <4> [188.935685] i915_gem_object_wait+0x1ff/0x410 [i915] <4> [188.935988] i915_gem_wait_ioctl+0xf2/0x2a0 [i915] <4> [188.936272] ? i915_gem_object_wait+0x410/0x410 [i915] <4> [188.936533] drm_ioctl_kernel+0xae/0x140 <4> [188.936546] drm_ioctl+0x201/0x3d0 <4> [188.936555] ? i915_gem_object_wait+0x410/0x410 [i915] <4> [188.936820] ? __fget_files+0xc2/0x1c0 <4> [188.936830] ? __fget_files+0xda/0x1c0 <4> [188.936839] __x64_sys_ioctl+0x6d/0xa0 <4> [188.936848] do_syscall_64+0x3a/0xb0 <4> [188.936859] entry_SYSCALL_64_after_hwframe+0x44/0xae If the shared object has changed during the RCU unlocked period callers will correctly handle the restart on the next iteration. Signed-off-by: Tvrtko Ursulin Fixes: 96601e8a4755 ("dma-buf: use new iterator in dma_resv_copy_fences") Fixes: d3c80698c9f5 ("dma-buf: use new iterator in dma_resv_get_fences v3") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4274 Cc: Christian König Cc: Daniel Vetter Cc: Sumit Semwal Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Link: https://patchwork.freedesktop.org/patch/msgid/20211008095007.972693-1-tvrtko.ursulin@linux.intel.com Reviewed-by: Christian König Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 2f98caa68ae5..9eb2baa387d4 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -333,10 +333,14 @@ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) { cursor->seq = read_seqcount_begin(&cursor->obj->seq); cursor->index = -1; - if (cursor->all_fences) + cursor->shared_count = 0; + if (cursor->all_fences) { cursor->fences = dma_resv_shared_list(cursor->obj); - else + if (cursor->fences) + cursor->shared_count = cursor->fences->shared_count; + } else { cursor->fences = NULL; + } cursor->is_restarted = true; } @@ -363,7 +367,7 @@ static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) continue; } else if (!cursor->fences || - cursor->index >= cursor->fences->shared_count) { + cursor->index >= cursor->shared_count) { cursor->fence = NULL; break; @@ -499,10 +503,8 @@ int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src) dma_resv_list_free(list); dma_fence_put(excl); - if (cursor.fences) { - unsigned int cnt = cursor.fences->shared_count; - - list = dma_resv_list_alloc(cnt); + if (cursor.shared_count) { + list = dma_resv_list_alloc(cursor.shared_count); if (!list) { dma_resv_iter_end(&cursor); return -ENOMEM; @@ -573,7 +575,7 @@ int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **fence_excl, if (fence_excl) dma_fence_put(*fence_excl); - count = cursor.fences ? cursor.fences->shared_count : 0; + count = cursor.shared_count; count += fence_excl ? 0 : 1; /* Eventually re-allocate the array */ -- cgit From 8c0fd126263730c35927cc8445727afb79219a19 Mon Sep 17 00:00:00 2001 From: Guangming Cao Date: Fri, 8 Oct 2021 15:54:20 +0800 Subject: dma-buf: acquire name lock before read/write dma_buf.name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because dma-buf.name can be freed in func: "dma_buf_set_name", so, we need to acquire lock first before we read/write dma_buf.name to prevent Use After Free(UAF) issue. Signed-off-by: Guangming Cao Reviewed-by: Christian König Signed-off-by: Christian König Link: https://patchwork.freedesktop.org/patch/msgid/20211008075420.42874-1-guangming.cao@mediatek.com --- drivers/dma-buf/dma-buf.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 63d32261b63f..92492e314bd9 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1389,6 +1389,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) if (ret) goto error_unlock; + + spin_lock(&buf_obj->name_lock); seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", buf_obj->size, buf_obj->file->f_flags, buf_obj->file->f_mode, @@ -1396,6 +1398,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) buf_obj->exp_name, file_inode(buf_obj->file)->i_ino, buf_obj->name ?: ""); + spin_unlock(&buf_obj->name_lock); robj = buf_obj->resv; fence = dma_resv_excl_fence(robj); -- cgit From ff2d23843f7fb4f13055be5a4a9a20ddd04e6e9c Mon Sep 17 00:00:00 2001 From: Michel Dänzer Date: Fri, 23 Jul 2021 09:58:57 +0200 Subject: dma-buf/poll: Get a file reference for outstanding fence callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes sure we don't hit the BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); in dma_buf_release, which could be triggered by user space closing the dma-buf file description while there are outstanding fence callbacks from dma_buf_poll. Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer Reviewed-by: Christian König Link: https://patchwork.freedesktop.org/patch/msgid/20210723075857.4065-1-michel@daenzer.net Signed-off-by: Christian König --- drivers/dma-buf/dma-buf.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'drivers/dma-buf') diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index beb504a92d60..35fe1cb5ad98 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -67,12 +67,9 @@ static void dma_buf_release(struct dentry *dentry) BUG_ON(dmabuf->vmapping_counter); /* - * Any fences that a dma-buf poll can wait on should be signaled - * before releasing dma-buf. This is the responsibility of each - * driver that uses the reservation objects. - * - * If you hit this BUG() it means someone dropped their ref to the - * dma-buf while still having pending operation to the buffer. + * If you hit this BUG() it could mean: + * * There's a file reference imbalance in dma_buf_poll / dma_buf_poll_cb or somewhere else + * * dmabuf->cb_in/out.active are non-0 despite no pending fence callback */ BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); @@ -200,6 +197,7 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) { struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb; + struct dma_buf *dmabuf = container_of(dcb->poll, struct dma_buf, poll); unsigned long flags; spin_lock_irqsave(&dcb->poll->lock, flags); @@ -207,6 +205,8 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) dcb->active = 0; spin_unlock_irqrestore(&dcb->poll->lock, flags); dma_fence_put(fence); + /* Paired with get_file in dma_buf_poll */ + fput(dmabuf->file); } static bool dma_buf_poll_add_cb(struct dma_resv *resv, bool write, @@ -259,6 +259,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLOUT) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_add_cb(resv, true, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); @@ -279,6 +282,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLIN) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_add_cb(resv, false, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); -- cgit