On 11/11/2019 15:42, Daniel Vetter wrote: > On Mon, Nov 11, 2019 at 2:11 PM Steven Price <steven.price@xxxxxxx> wrote: >> >> On 04/11/2019 17:37, Daniel Vetter 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. >>> >>> v4: Annotate with __init (Rob Herring) >>> >>> Cc: Rob Herring <robh@xxxxxxxxxx> >>> Cc: Alex Deucher <alexander.deucher@xxxxxxx> >>> Cc: Christian König <christian.koenig@xxxxxxx> >>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> >>> Cc: Rob Herring <robh@xxxxxxxxxx> >>> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> >>> Cc: Eric Anholt <eric@xxxxxxxxxx> >>> Cc: Dave Airlie <airlied@xxxxxxxxxx> >>> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx> >>> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx> >>> Cc: "VMware Graphics" <linux-graphics-maintainer@xxxxxxxxxx> >>> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx> >>> Reviewed-by: Christian König <christian.koenig@xxxxxxx> >>> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >>> Tested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> >>> --- >>> 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 709002515550..a05ff542be22 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 __init 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); >>> + >> >> Nit: trailing whitespace >> >>> + mmput(mm); >>> +} >>> +subsys_initcall(dma_resv_lockdep); >> >> This expects a function returning int, but dma_resv_lockdep() is void. >> Causing: >> >> drivers/dma-buf/dma-resv.c:119:17: error: initialization of ‘initcall_t’ >> {aka ‘int (*)(void)’} from incompatible pointer type ‘void (*)(void)’ >> [-Werror=incompatible-pointer-types] >> subsys_initcall(dma_resv_lockdep); >> >> The below fixes it for me. > > Uh, so _that_ was what the 0day thing was all about, I totally misread > that completely. Thanks for the patch. > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Aside, do you need commit rights for pushing this kind of stuff? I guess it's about time I got round to requesting that: https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/208 Thanks, Steve _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel