dma_resv: prime lockdep annotations

Commit Message

Daniel Vetter Aug. 22, 2019, 6:54 a.m. UTC

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 <[e-mail]>
Cc: Christian König <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Tomeu Vizoso <[e-mail]>
Cc: Eric Anholt <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Cc: Gerd Hoffmann <[e-mail]>
Cc: Ben Skeggs <[e-mail]>
Cc: "VMware Graphics" <[e-mail]>
Cc: Thomas Hellstrom <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/dma-buf/dma-resv.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Chris Wilson Aug. 22, 2019, 7:48 a.m. UTC | #1

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 <[e-mail]>
Tested-by: Chris Wilson <[e-mail]>
-Chris

Christian König Aug. 22, 2019, 7:53 a.m. UTC | #2

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 <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Tomeu Vizoso <[e-mail]>
> Cc: Eric Anholt <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Gerd Hoffmann <[e-mail]>
> Cc: Ben Skeggs <[e-mail]>
> Cc: "VMware Graphics" <[e-mail]>
> Cc: Thomas Hellstrom <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>

Reviewed-by: Christian König <[e-mail]>

> ---
>   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

Rob Herring (Arm) Aug. 22, 2019, 12:56 p.m. UTC | #3

On Thu, Aug 22, 2019 at 1:55 AM Daniel Vetter <[e-mail]> 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 <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Tomeu Vizoso <[e-mail]>
> Cc: Eric Anholt <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Gerd Hoffmann <[e-mail]>
> Cc: Ben Skeggs <[e-mail]>
> Cc: "VMware Graphics" <[e-mail]>
> Cc: Thomas Hellstrom <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  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
>

Daniel Vetter Sept. 3, 2019, 8:16 a.m. UTC | #4

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 <[e-mail]>
> > Cc: Christian König <[e-mail]>
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Tomeu Vizoso <[e-mail]>
> > Cc: Eric Anholt <[e-mail]>
> > Cc: Dave Airlie <[e-mail]>
> > Cc: Gerd Hoffmann <[e-mail]>
> > Cc: Ben Skeggs <[e-mail]>
> > Cc: "VMware Graphics" <[e-mail]>
> > Cc: Thomas Hellstrom <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> 
> Reviewed-by: Christian König <[e-mail]>

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
>

Christian König Sept. 3, 2019, 9:02 a.m. UTC | #5

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 <[e-mail]>
>>> Cc: Christian König <[e-mail]>
>>> Cc: Chris Wilson <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: Rob Herring <[e-mail]>
>>> Cc: Tomeu Vizoso <[e-mail]>
>>> Cc: Eric Anholt <[e-mail]>
>>> Cc: Dave Airlie <[e-mail]>
>>> Cc: Gerd Hoffmann <[e-mail]>
>>> Cc: Ben Skeggs <[e-mail]>
>>> Cc: "VMware Graphics" <[e-mail]>
>>> Cc: Thomas Hellstrom <[e-mail]>
>>> Signed-off-by: Daniel Vetter <[e-mail]>
>> Reviewed-by: Christian König <[e-mail]>
> 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

Patch

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