Message ID | 20190822065457.2830-1-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | dma_resv: prime lockdep annotations | expand |
Quoting Daniel Vetter (2019-08-22 07:54:57) > +#if IS_ENABLED(CONFIG_LOCKDEP) > +static void dma_resv_lockdep(void) > +{ > + struct mm_struct *mm = mm_alloc(); > + struct dma_resv obj; > + > + if (!mm) > + return; > + > + dma_resv_init(&obj); > + > + down_read(&mm->mmap_sem); > + ww_mutex_lock(&obj.lock, NULL); > + fs_reclaim_acquire(GFP_KERNEL); > + fs_reclaim_release(GFP_KERNEL); > + ww_mutex_unlock(&obj.lock); > + up_read(&mm->mmap_sem); > + > + mmput(mm); > +} > +subsys_initcall(dma_resv_lockdep); > +#endif Adding a dma_resv_lock(); might_lock(&i915->drm.struct_mutex); dma_resv_unlock(); yielded [ 18.513633] ====================================================== [ 18.513636] WARNING: possible circular locking dependency detected [ 18.513639] 5.3.0-rc5+ #76 Not tainted [ 18.513640] ------------------------------------------------------ [ 18.513643] insmod/655 is trying to acquire lock: [ 18.513645] 00000000877909e7 (&dev->struct_mutex){+.+.}, at: i915_driver_probe+0x89c/0x1470 [i915] [ 18.513671] [ 18.513671] but task is already holding lock: [ 18.513673] 00000000a85ba8ec (reservation_ww_class_mutex){+.+.}, at: i915_driver_probe+0x8e1/0x1470 [i915] [ 18.513698] [ 18.513698] which lock already depends on the new lock. [ 18.513698] [ 18.513701] [ 18.513701] the existing dependency chain (in reverse order) is: [ 18.513703] [ 18.513703] -> #1 (reservation_ww_class_mutex){+.+.}: [ 18.513708] __ww_mutex_lock.constprop.17+0xbc/0xf90 [ 18.513739] i915_gem_init+0x518/0x750 [i915] [ 18.513762] i915_driver_probe+0x891/0x1470 [i915] [ 18.513785] i915_pci_probe+0x2f/0x110 [i915] [ 18.513789] pci_device_probe+0x99/0x110 [ 18.513792] really_probe+0xd1/0x360 [ 18.513794] driver_probe_device+0xaf/0xf0 [ 18.513796] device_driver_attach+0x4a/0x50 [ 18.513799] __driver_attach+0x80/0x140 [ 18.513801] bus_for_each_dev+0x5e/0x90 [ 18.513804] bus_add_driver+0x148/0x1e0 [ 18.513806] driver_register+0x66/0xb0 [ 18.513809] do_one_initcall+0x45/0x29f [ 18.513812] do_init_module+0x55/0x200 [ 18.513814] load_module+0x2519/0x2690 [ 18.513816] __do_sys_finit_module+0x8f/0xd0 [ 18.513818] do_syscall_64+0x4f/0x220 [ 18.513822] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 18.513824] [ 18.513824] -> #0 (&dev->struct_mutex){+.+.}: [ 18.513828] __lock_acquire+0xcb9/0x1520 [ 18.513831] lock_acquire+0x90/0x170 [ 18.513853] i915_driver_probe+0x8fd/0x1470 [i915] [ 18.513876] i915_pci_probe+0x2f/0x110 [i915] [ 18.513879] pci_device_probe+0x99/0x110 [ 18.513881] really_probe+0xd1/0x360 [ 18.513883] driver_probe_device+0xaf/0xf0 [ 18.513886] device_driver_attach+0x4a/0x50 [ 18.513888] __driver_attach+0x80/0x140 [ 18.513891] bus_for_each_dev+0x5e/0x90 [ 18.513893] bus_add_driver+0x148/0x1e0 [ 18.513895] driver_register+0x66/0xb0 [ 18.513897] do_one_initcall+0x45/0x29f [ 18.513899] do_init_module+0x55/0x200 [ 18.513902] load_module+0x2519/0x2690 [ 18.513904] __do_sys_finit_module+0x8f/0xd0 [ 18.513906] do_syscall_64+0x4f/0x220 [ 18.513909] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 18.513911] [ 18.513911] other info that might help us debug this: [ 18.513911] [ 18.513914] Possible unsafe locking scenario: [ 18.513914] [ 18.513916] CPU0 CPU1 [ 18.513918] ---- ---- [ 18.513920] lock(reservation_ww_class_mutex); [ 18.513922] lock(&dev->struct_mutex); [ 18.513924] lock(reservation_ww_class_mutex); [ 18.513927] lock(&dev->struct_mutex); [ 18.513929] [ 18.513929] *** DEADLOCK *** [ 18.513929] [ 18.513932] 3 locks held by insmod/655: [ 18.513933] #0: 000000004dccb591 (&dev->mutex){....}, at: device_driver_attach+0x18/0x50 [ 18.513938] #1: 000000009118ecae (&mm->mmap_sem#2){++++}, at: i915_driver_probe+0x8c8/0x1470 [i915] [ 18.513962] #2: 00000000a85ba8ec (reservation_ww_class_mutex){+.+.}, at: i915_driver_probe+0x8e1/0x1470 [i915] so Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Am 22.08.19 um 08:54 schrieb Daniel Vetter: > Full audit of everyone: > > - i915, radeon, amdgpu should be clean per their maintainers. > > - vram helpers should be fine, they don't do command submission, so > really no business holding struct_mutex while doing copy_*_user. But > I haven't checked them all. > > - panfrost seems to dma_resv_lock only in panfrost_job_push, which > looks clean. > > - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), > copying from/to userspace happens all in v3d_lookup_bos which is > outside of the critical section. > > - vmwgfx has a bunch of ioctls that do their own copy_*_user: > - vmw_execbuf_process: First this does some copies in > vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. > Then comes the usual ttm reserve/validate sequence, then actual > submission/fencing, then unreserving, and finally some more > copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of > details, but looks all safe. > - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be > seen, seems to only create a fence and copy it out. > - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be > found there. > Summary: vmwgfx seems to be fine too. > > - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the > copying from userspace before even looking up objects through their > handles, so safe. Plus the getparam/getcaps ioctl, also both safe. > > - qxl only has qxl_execbuffer_ioctl, which calls into > qxl_process_single_command. There's a lovely comment before the > __copy_from_user_inatomic that the slowpath should be copied from > i915, but I guess that never happened. Try not to be unlucky and get > your CS data evicted between when it's written and the kernel tries > to read it. The only other copy_from_user is for relocs, but those > are done before qxl_release_reserve_list(), which seems to be the > only thing reserving buffers (in the ttm/dma_resv sense) in that > code. So looks safe. > > - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in > usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this > everywhere and needs to be fixed up. > > v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a > dma_resv lock of a different object already. Christian mentioned that > ttm core does this too for ghost objects. intel-gfx-ci highlighted > that i915 has similar issues. > > Unfortunately we can't do this in the usual module init functions, > because kernel threads don't have an ->mm - we have to wait around for > some user thread to do this. > > Solution is to spawn a worker (but only once). It's horrible, but it > works. > > v3: We can allocate mm! (Chris). Horrible worker hack out, clean > initcall solution in. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Rob Herring <robh@kernel.org> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 42a8f3f11681..d233ef4cf0d7 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -34,6 +34,7 @@ > > #include <linux/dma-resv.h> > #include <linux/export.h> > +#include <linux/sched/mm.h> > > /** > * DOC: Reservation Object Overview > @@ -95,6 +96,29 @@ static void dma_resv_list_free(struct dma_resv_list *list) > kfree_rcu(list, rcu); > } > > +#if IS_ENABLED(CONFIG_LOCKDEP) > +static void dma_resv_lockdep(void) > +{ > + struct mm_struct *mm = mm_alloc(); > + struct dma_resv obj; > + > + if (!mm) > + return; > + > + dma_resv_init(&obj); > + > + down_read(&mm->mmap_sem); > + ww_mutex_lock(&obj.lock, NULL); > + fs_reclaim_acquire(GFP_KERNEL); > + fs_reclaim_release(GFP_KERNEL); > + ww_mutex_unlock(&obj.lock); > + up_read(&mm->mmap_sem); > + > + mmput(mm); > +} > +subsys_initcall(dma_resv_lockdep); > +#endif > + > /** > * dma_resv_init - initialize a reservation object > * @obj: the reservation object
On Thu, Aug 22, 2019 at 1:55 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Full audit of everyone: > > - i915, radeon, amdgpu should be clean per their maintainers. > > - vram helpers should be fine, they don't do command submission, so > really no business holding struct_mutex while doing copy_*_user. But > I haven't checked them all. > > - panfrost seems to dma_resv_lock only in panfrost_job_push, which > looks clean. > > - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), > copying from/to userspace happens all in v3d_lookup_bos which is > outside of the critical section. > > - vmwgfx has a bunch of ioctls that do their own copy_*_user: > - vmw_execbuf_process: First this does some copies in > vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. > Then comes the usual ttm reserve/validate sequence, then actual > submission/fencing, then unreserving, and finally some more > copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of > details, but looks all safe. > - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be > seen, seems to only create a fence and copy it out. > - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be > found there. > Summary: vmwgfx seems to be fine too. > > - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the > copying from userspace before even looking up objects through their > handles, so safe. Plus the getparam/getcaps ioctl, also both safe. > > - qxl only has qxl_execbuffer_ioctl, which calls into > qxl_process_single_command. There's a lovely comment before the > __copy_from_user_inatomic that the slowpath should be copied from > i915, but I guess that never happened. Try not to be unlucky and get > your CS data evicted between when it's written and the kernel tries > to read it. The only other copy_from_user is for relocs, but those > are done before qxl_release_reserve_list(), which seems to be the > only thing reserving buffers (in the ttm/dma_resv sense) in that > code. So looks safe. > > - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in > usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this > everywhere and needs to be fixed up. > > v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a > dma_resv lock of a different object already. Christian mentioned that > ttm core does this too for ghost objects. intel-gfx-ci highlighted > that i915 has similar issues. > > Unfortunately we can't do this in the usual module init functions, > because kernel threads don't have an ->mm - we have to wait around for > some user thread to do this. > > Solution is to spawn a worker (but only once). It's horrible, but it > works. > > v3: We can allocate mm! (Chris). Horrible worker hack out, clean > initcall solution in. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Rob Herring <robh@kernel.org> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 42a8f3f11681..d233ef4cf0d7 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -34,6 +34,7 @@ > > #include <linux/dma-resv.h> > #include <linux/export.h> > +#include <linux/sched/mm.h> > > /** > * DOC: Reservation Object Overview > @@ -95,6 +96,29 @@ static void dma_resv_list_free(struct dma_resv_list *list) > kfree_rcu(list, rcu); > } > > +#if IS_ENABLED(CONFIG_LOCKDEP) > +static void dma_resv_lockdep(void) __init > +{ > + struct mm_struct *mm = mm_alloc(); > + struct dma_resv obj; > + > + if (!mm) > + return; > + > + dma_resv_init(&obj); > + > + down_read(&mm->mmap_sem); > + ww_mutex_lock(&obj.lock, NULL); > + fs_reclaim_acquire(GFP_KERNEL); > + fs_reclaim_release(GFP_KERNEL); > + ww_mutex_unlock(&obj.lock); > + up_read(&mm->mmap_sem); > + > + mmput(mm); > +} > +subsys_initcall(dma_resv_lockdep); > +#endif > + > /** > * dma_resv_init - initialize a reservation object > * @obj: the reservation object > -- > 2.23.0.rc1 >
On Thu, Aug 22, 2019 at 07:53:53AM +0000, Koenig, Christian wrote: > Am 22.08.19 um 08:54 schrieb Daniel Vetter: > > Full audit of everyone: > > > > - i915, radeon, amdgpu should be clean per their maintainers. > > > > - vram helpers should be fine, they don't do command submission, so > > really no business holding struct_mutex while doing copy_*_user. But > > I haven't checked them all. > > > > - panfrost seems to dma_resv_lock only in panfrost_job_push, which > > looks clean. > > > > - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), > > copying from/to userspace happens all in v3d_lookup_bos which is > > outside of the critical section. > > > > - vmwgfx has a bunch of ioctls that do their own copy_*_user: > > - vmw_execbuf_process: First this does some copies in > > vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. > > Then comes the usual ttm reserve/validate sequence, then actual > > submission/fencing, then unreserving, and finally some more > > copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of > > details, but looks all safe. > > - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be > > seen, seems to only create a fence and copy it out. > > - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be > > found there. > > Summary: vmwgfx seems to be fine too. > > > > - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the > > copying from userspace before even looking up objects through their > > handles, so safe. Plus the getparam/getcaps ioctl, also both safe. > > > > - qxl only has qxl_execbuffer_ioctl, which calls into > > qxl_process_single_command. There's a lovely comment before the > > __copy_from_user_inatomic that the slowpath should be copied from > > i915, but I guess that never happened. Try not to be unlucky and get > > your CS data evicted between when it's written and the kernel tries > > to read it. The only other copy_from_user is for relocs, but those > > are done before qxl_release_reserve_list(), which seems to be the > > only thing reserving buffers (in the ttm/dma_resv sense) in that > > code. So looks safe. > > > > - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in > > usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this > > everywhere and needs to be fixed up. > > > > v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a > > dma_resv lock of a different object already. Christian mentioned that > > ttm core does this too for ghost objects. intel-gfx-ci highlighted > > that i915 has similar issues. > > > > Unfortunately we can't do this in the usual module init functions, > > because kernel threads don't have an ->mm - we have to wait around for > > some user thread to do this. > > > > Solution is to spawn a worker (but only once). It's horrible, but it > > works. > > > > v3: We can allocate mm! (Chris). Horrible worker hack out, clean > > initcall solution in. > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Christian König <christian.koenig@amd.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: Rob Herring <robh@kernel.org> > > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Cc: Eric Anholt <eric@anholt.net> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com> > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> Did you get a chance to give this a spin on the amd CI? -Daniel > > > --- > > drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > > index 42a8f3f11681..d233ef4cf0d7 100644 > > --- a/drivers/dma-buf/dma-resv.c > > +++ b/drivers/dma-buf/dma-resv.c > > @@ -34,6 +34,7 @@ > > > > #include <linux/dma-resv.h> > > #include <linux/export.h> > > +#include <linux/sched/mm.h> > > > > /** > > * DOC: Reservation Object Overview > > @@ -95,6 +96,29 @@ static void dma_resv_list_free(struct dma_resv_list *list) > > kfree_rcu(list, rcu); > > } > > > > +#if IS_ENABLED(CONFIG_LOCKDEP) > > +static void dma_resv_lockdep(void) > > +{ > > + struct mm_struct *mm = mm_alloc(); > > + struct dma_resv obj; > > + > > + if (!mm) > > + return; > > + > > + dma_resv_init(&obj); > > + > > + down_read(&mm->mmap_sem); > > + ww_mutex_lock(&obj.lock, NULL); > > + fs_reclaim_acquire(GFP_KERNEL); > > + fs_reclaim_release(GFP_KERNEL); > > + ww_mutex_unlock(&obj.lock); > > + up_read(&mm->mmap_sem); > > + > > + mmput(mm); > > +} > > +subsys_initcall(dma_resv_lockdep); > > +#endif > > + > > /** > > * dma_resv_init - initialize a reservation object > > * @obj: the reservation object >
Am 03.09.19 um 10:16 schrieb Daniel Vetter: > On Thu, Aug 22, 2019 at 07:53:53AM +0000, Koenig, Christian wrote: >> Am 22.08.19 um 08:54 schrieb Daniel Vetter: >>> Full audit of everyone: >>> >>> - i915, radeon, amdgpu should be clean per their maintainers. >>> >>> - vram helpers should be fine, they don't do command submission, so >>> really no business holding struct_mutex while doing copy_*_user. But >>> I haven't checked them all. >>> >>> - panfrost seems to dma_resv_lock only in panfrost_job_push, which >>> looks clean. >>> >>> - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), >>> copying from/to userspace happens all in v3d_lookup_bos which is >>> outside of the critical section. >>> >>> - vmwgfx has a bunch of ioctls that do their own copy_*_user: >>> - vmw_execbuf_process: First this does some copies in >>> vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. >>> Then comes the usual ttm reserve/validate sequence, then actual >>> submission/fencing, then unreserving, and finally some more >>> copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of >>> details, but looks all safe. >>> - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be >>> seen, seems to only create a fence and copy it out. >>> - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be >>> found there. >>> Summary: vmwgfx seems to be fine too. >>> >>> - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the >>> copying from userspace before even looking up objects through their >>> handles, so safe. Plus the getparam/getcaps ioctl, also both safe. >>> >>> - qxl only has qxl_execbuffer_ioctl, which calls into >>> qxl_process_single_command. There's a lovely comment before the >>> __copy_from_user_inatomic that the slowpath should be copied from >>> i915, but I guess that never happened. Try not to be unlucky and get >>> your CS data evicted between when it's written and the kernel tries >>> to read it. The only other copy_from_user is for relocs, but those >>> are done before qxl_release_reserve_list(), which seems to be the >>> only thing reserving buffers (in the ttm/dma_resv sense) in that >>> code. So looks safe. >>> >>> - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in >>> usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this >>> everywhere and needs to be fixed up. >>> >>> v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a >>> dma_resv lock of a different object already. Christian mentioned that >>> ttm core does this too for ghost objects. intel-gfx-ci highlighted >>> that i915 has similar issues. >>> >>> Unfortunately we can't do this in the usual module init functions, >>> because kernel threads don't have an ->mm - we have to wait around for >>> some user thread to do this. >>> >>> Solution is to spawn a worker (but only once). It's horrible, but it >>> works. >>> >>> v3: We can allocate mm! (Chris). Horrible worker hack out, clean >>> initcall solution in. >>> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> Cc: Christian König <christian.koenig@amd.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Rob Herring <robh@kernel.org> >>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>> Cc: Eric Anholt <eric@anholt.net> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>> Cc: Ben Skeggs <bskeggs@redhat.com> >>> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com> >>> Cc: Thomas Hellstrom <thellstrom@vmware.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> > Did you get a chance to give this a spin on the amd CI? No and sorry totally forgot to ask about that. Going to try to bring this up tomorrow once more, but don't expect that I can get this tested anytime soon. Christian. > -Daniel > >>> --- >>> drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >>> index 42a8f3f11681..d233ef4cf0d7 100644 >>> --- a/drivers/dma-buf/dma-resv.c >>> +++ b/drivers/dma-buf/dma-resv.c >>> @@ -34,6 +34,7 @@ >>> >>> #include <linux/dma-resv.h> >>> #include <linux/export.h> >>> +#include <linux/sched/mm.h> >>> >>> /** >>> * DOC: Reservation Object Overview >>> @@ -95,6 +96,29 @@ static void dma_resv_list_free(struct dma_resv_list *list) >>> kfree_rcu(list, rcu); >>> } >>> >>> +#if IS_ENABLED(CONFIG_LOCKDEP) >>> +static void dma_resv_lockdep(void) >>> +{ >>> + struct mm_struct *mm = mm_alloc(); >>> + struct dma_resv obj; >>> + >>> + if (!mm) >>> + return; >>> + >>> + dma_resv_init(&obj); >>> + >>> + down_read(&mm->mmap_sem); >>> + ww_mutex_lock(&obj.lock, NULL); >>> + fs_reclaim_acquire(GFP_KERNEL); >>> + fs_reclaim_release(GFP_KERNEL); >>> + ww_mutex_unlock(&obj.lock); >>> + up_read(&mm->mmap_sem); >>> + >>> + mmput(mm); >>> +} >>> +subsys_initcall(dma_resv_lockdep); >>> +#endif >>> + >>> /** >>> * dma_resv_init - initialize a reservation object >>> * @obj: the reservation object
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 42a8f3f11681..d233ef4cf0d7 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -34,6 +34,7 @@ #include <linux/dma-resv.h> #include <linux/export.h> +#include <linux/sched/mm.h> /** * DOC: Reservation Object Overview @@ -95,6 +96,29 @@ static void dma_resv_list_free(struct dma_resv_list *list) kfree_rcu(list, rcu); } +#if IS_ENABLED(CONFIG_LOCKDEP) +static void dma_resv_lockdep(void) +{ + struct mm_struct *mm = mm_alloc(); + struct dma_resv obj; + + if (!mm) + return; + + dma_resv_init(&obj); + + down_read(&mm->mmap_sem); + ww_mutex_lock(&obj.lock, NULL); + fs_reclaim_acquire(GFP_KERNEL); + fs_reclaim_release(GFP_KERNEL); + ww_mutex_unlock(&obj.lock); + up_read(&mm->mmap_sem); + + mmput(mm); +} +subsys_initcall(dma_resv_lockdep); +#endif + /** * dma_resv_init - initialize a reservation object * @obj: the reservation object
Full audit of everyone: - i915, radeon, amdgpu should be clean per their maintainers. - vram helpers should be fine, they don't do command submission, so really no business holding struct_mutex while doing copy_*_user. But I haven't checked them all. - panfrost seems to dma_resv_lock only in panfrost_job_push, which looks clean. - v3d holds dma_resv locks in the tail of its v3d_submit_cl_ioctl(), copying from/to userspace happens all in v3d_lookup_bos which is outside of the critical section. - vmwgfx has a bunch of ioctls that do their own copy_*_user: - vmw_execbuf_process: First this does some copies in vmw_execbuf_cmdbuf() and also in the vmw_execbuf_process() itself. Then comes the usual ttm reserve/validate sequence, then actual submission/fencing, then unreserving, and finally some more copy_to_user in vmw_execbuf_copy_fence_user. Glossing over tons of details, but looks all safe. - vmw_fence_event_ioctl: No ttm_reserve/dma_resv_lock anywhere to be seen, seems to only create a fence and copy it out. - a pile of smaller ioctl in vmwgfx_ioctl.c, no reservations to be found there. Summary: vmwgfx seems to be fine too. - virtio: There's virtio_gpu_execbuffer_ioctl, which does all the copying from userspace before even looking up objects through their handles, so safe. Plus the getparam/getcaps ioctl, also both safe. - qxl only has qxl_execbuffer_ioctl, which calls into qxl_process_single_command. There's a lovely comment before the __copy_from_user_inatomic that the slowpath should be copied from i915, but I guess that never happened. Try not to be unlucky and get your CS data evicted between when it's written and the kernel tries to read it. The only other copy_from_user is for relocs, but those are done before qxl_release_reserve_list(), which seems to be the only thing reserving buffers (in the ttm/dma_resv sense) in that code. So looks safe. - A debugfs file in nouveau_debugfs_pstate_set() and the usif ioctl in usif_ioctl() look safe. nouveau_gem_ioctl_pushbuf() otoh breaks this everywhere and needs to be fixed up. v2: Thomas pointed at that vmwgfx calls dma_resv_init while it holds a dma_resv lock of a different object already. Christian mentioned that ttm core does this too for ghost objects. intel-gfx-ci highlighted that i915 has similar issues. Unfortunately we can't do this in the usual module init functions, because kernel threads don't have an ->mm - we have to wait around for some user thread to do this. Solution is to spawn a worker (but only once). It's horrible, but it works. v3: We can allocate mm! (Chris). Horrible worker hack out, clean initcall solution in. Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Christian König <christian.koenig@amd.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Rob Herring <robh@kernel.org> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> Cc: Eric Anholt <eric@anholt.net> Cc: Dave Airlie <airlied@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com> Cc: Thomas Hellstrom <thellstrom@vmware.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)