Linaro-mm-sig August 2021

linaro-mm-sig@lists.linaro.org

15 participants
43 discussions

Re: [Linaro-mm-sig] [PATCH v3 5/9] drm: protect magic_map, unique{_len} with master_lookup_lock

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:20PM +0800, Desmond Cheong Zhi Xi wrote: > Currently, drm_device.master_mutex is used to serialize writes to the > drm_master.magic_map idr and to protect drm_master.unique{_len}. > > In preparation for converting drm_device.master_mutex into an outer > rwsem that might be read locked before entering some of these > functions, we can instead serialize access to drm_master.magic_map and > drm_master.unique{_len} using drm_device.master_lookup_lock which is > an inner lock. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > --- > drivers/gpu/drm/drm_auth.c | 12 +++++++----- > drivers/gpu/drm/drm_ioctl.c | 10 ++++++---- > include/drm/drm_auth.h | 6 +++--- > include/drm/drm_device.h | 7 ++++++- > 4 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index b7230604496b..0acb444fbbac 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -98,10 +98,10 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) > struct drm_master *master; > int ret = 0; > > - mutex_lock(&dev->master_mutex); > + spin_lock(&dev->master_lookup_lock); > master = file_priv->master; > if (!master) { > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); > return -EINVAL; > } > > @@ -112,7 +112,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) > file_priv->magic = ret; > } > auth->magic = file_priv->magic; > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); > > DRM_DEBUG("%u\n", auth->magic); > > @@ -127,13 +127,13 @@ int drm_authmagic(struct drm_device *dev, void *data, > > DRM_DEBUG("%u\n", auth->magic); > > - mutex_lock(&dev->master_mutex); > + spin_lock(&dev->master_lookup_lock); > file = idr_find(&file_priv->master->magic_map, auth->magic); > if (file) { > file->authenticated = 1; > idr_replace(&file_priv->master->magic_map, NULL, auth->magic); > } > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); > > return file ? 0 : -EINVAL; > } > @@ -366,8 +366,10 @@ void drm_master_release(struct drm_file *file_priv) > if (!master) > goto unlock; > > + spin_lock(&dev->master_lookup_lock); > if (file_priv->magic) > idr_remove(&master->magic_map, file_priv->magic); > + spin_unlock(&dev->master_lookup_lock); > > if (!drm_is_current_master_locked(file_priv)) > goto out; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 4d029d3061d9..e5c3845b6e62 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -119,21 +119,21 @@ int drm_getunique(struct drm_device *dev, void *data, > struct drm_unique *u = data; > struct drm_master *master; > > - mutex_lock(&dev->master_mutex); > + spin_lock(&dev->master_lookup_lock); > master = file_priv->master; > if (!master) { > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); > return -EINVAL; > } > > if (u->unique_len >= master->unique_len) { > if (copy_to_user(u->unique, master->unique, master->unique_len)) { > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); copy_to_user while holding a spinlock isn't going to work well, at least when we take a fault. The might_fault() annotations (if enabled) should catch that. Which is really annoying, because it kinda puts a wrench into this neat plan here :-/ > return -EFAULT; > } > } > u->unique_len = master->unique_len; > - mutex_unlock(&dev->master_mutex); > + spin_unlock(&dev->master_lookup_lock); > > return 0; > } > @@ -405,7 +405,9 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f > * Version 1.1 includes tying of DRM to specific device > * Version 1.4 has proper PCI domain support > */ > + spin_lock(&dev->master_lookup_lock); > retcode = drm_set_busid(dev, file_priv); > + spin_unlock(&dev->master_lookup_lock); Similar issue with drm_set_busid, calling kmalloc under a spinlock isn't a good idea. This one here is at least much easier to fix by pushing the locking down a lot. I'm wondering a bit whether a better fix for these ioctls wouldn't be to - drop the DRM_MASTER flag from the ioctl table - take the rwsem in write mode (which would replace our current dev->master_lock) and check for master status while holding that lock I think that would result in simpler locking or am I missing something? Maybe this could even work as a replacment for the lookup spinlock, since we're untangling the nesting quite a bit? Really just tossing ideas around since I feel like we don't have the best one yet ... -Daniel > if (retcode) > goto done; > } > diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h > index ba248ca8866f..f5be73153798 100644 > --- a/include/drm/drm_auth.h > +++ b/include/drm/drm_auth.h > @@ -67,17 +67,17 @@ struct drm_master { > struct drm_device *dev; > /** > * @unique: Unique identifier: e.g. busid. Protected by > - * &drm_device.master_mutex. > + * &drm_device.master_lookup_lock. > */ > char *unique; > /** > * @unique_len: Length of unique field. Protected by > - * &drm_device.master_mutex. > + * &drm_device.master_lookup_lock. > */ > int unique_len; > /** > * @magic_map: Map of used authentication tokens. Protected by > - * &drm_device.master_mutex. > + * &drm_device.master_lookup_lock. > */ > struct idr magic_map; > void *driver_priv; > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 506eb2784819..cf5d15aeb25f 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -152,7 +152,12 @@ struct drm_device { > */ > struct mutex master_mutex; > > - /** @master_lookup_lock: Serializes &drm_file.master. */ > + /** > + * @master_lookup_lock: > + * > + * Serializes &drm_file.master, &drm_master.magic_map, > + * &drm_master.unique, and &drm_master.unique_len. > + */ > spinlock_t master_lookup_lock; > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH v3 4/9] drm: fix potential null ptr dereferences in drm_{auth, ioctl}

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:19PM +0800, Desmond Cheong Zhi Xi wrote: > There are three areas where we dereference struct drm_master without > checking if the pointer is non-NULL. > > 1. drm_getmagic is called from the ioctl_handler. Since > DRM_IOCTL_GET_MAGIC has no ioctl flags, drm_getmagic is run without > any check that drm_file.master has been set. > > 2. Similarly, drm_getunique is called from the ioctl_handler, but > DRM_IOCTL_GET_UNIQUE has no ioctl flags. So there is no guarantee that > drm_file.master has been set. I think the above two are impossible, due to the refcounting rules for struct file. > 3. drm_master_release can also be called without having a > drm_file.master set. Here is one error path: > drm_open(): > drm_open_helper(): > drm_master_open(): > drm_new_set_master(); <--- returns -ENOMEM, > drm_file.master not set > drm_file_free(): > drm_master_release(); <--- NULL ptr dereference > (file_priv->master->magic_map) > > Fix these by checking if the master pointers are NULL before use. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > --- > drivers/gpu/drm/drm_auth.c | 16 ++++++++++++++-- > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index f9267b21556e..b7230604496b 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -95,11 +95,18 @@ EXPORT_SYMBOL(drm_is_current_master); > int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) > { > struct drm_auth *auth = data; > + struct drm_master *master; > int ret = 0; > > mutex_lock(&dev->master_mutex); > + master = file_priv->master; > + if (!master) { > + mutex_unlock(&dev->master_mutex); > + return -EINVAL; > + } > + > if (!file_priv->magic) { > - ret = idr_alloc(&file_priv->master->magic_map, file_priv, > + ret = idr_alloc(&master->magic_map, file_priv, > 1, 0, GFP_KERNEL); > if (ret >= 0) > file_priv->magic = ret; > @@ -355,8 +362,12 @@ void drm_master_release(struct drm_file *file_priv) > > mutex_lock(&dev->master_mutex); > master = file_priv->master; > + > + if (!master) > + goto unlock; This is a bit convoluted, since we're in the single-threaded release path we don't need any locking for file_priv related things. Therefore we can pull the master check out and just directly return. But since it's a bit surprising maybe a comment that this can happen when drm_master_open in drm_open_helper fails? Another option, and maybe cleaner, would be to move the drm_master_release from drm_file_free into drm_close_helper. That would be fully symmetrical and should also fix the bug here? -Daniel > + > if (file_priv->magic) > - idr_remove(&file_priv->master->magic_map, file_priv->magic); > + idr_remove(&master->magic_map, file_priv->magic); > > if (!drm_is_current_master_locked(file_priv)) > goto out; > @@ -379,6 +390,7 @@ void drm_master_release(struct drm_file *file_priv) > drm_master_put(&file_priv->master); > spin_unlock(&dev->master_lookup_lock); > } > +unlock: > mutex_unlock(&dev->master_mutex); > } > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 26f3a9ede8fe..4d029d3061d9 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -121,6 +121,11 @@ int drm_getunique(struct drm_device *dev, void *data, > > mutex_lock(&dev->master_mutex); > master = file_priv->master; > + if (!master) { > + mutex_unlock(&dev->master_mutex); > + return -EINVAL; > + } > + > if (u->unique_len >= master->unique_len) { > if (copy_to_user(u->unique, master->unique, master->unique_len)) { > mutex_unlock(&dev->master_mutex); > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH v3 2/9] drm: hold master_lookup_lock when releasing a drm_file's master

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:17PM +0800, Desmond Cheong Zhi Xi wrote: > When drm_file.master changes value, the corresponding > drm_device.master_lookup_lock should be held. > > In drm_master_release, a call to drm_master_put sets the > file_priv->master to NULL, so we protect this section with > drm_device.master_lookup_lock. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> At this points all refcounts to drm_file have disappeared, so yeah this is a lockless access, but also no one can observe it anymore. See also next patch. Hence I think the current code is fine. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 8efb58aa7d95..8c0e0dba1611 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -373,8 +373,11 @@ void drm_master_release(struct drm_file *file_priv) > } > > /* drop the master reference held by the file priv */ > - if (file_priv->master) > + if (file_priv->master) { > + spin_lock(&dev->master_lookup_lock); > drm_master_put(&file_priv->master); > + spin_unlock(&dev->master_lookup_lock); > + } > mutex_unlock(&dev->master_mutex); > } > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH v3 3/9] drm: check for null master in drm_is_current_master_locked

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:18PM +0800, Desmond Cheong Zhi Xi wrote: > There is a window after calling drm_master_release, and before a file > is freed, where drm_file can have is_master set to true, but both the > drm_file and drm_device have no master. > > This could result in wrongly approving permissions in > drm_is_current_master_locked. Add a check that fpriv->master is > non-NULl to guard against this scenario. > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> This should be impossible, drm_master_release is only called when the struct file is released, which means all ioctls and anything else have finished (they hold a temporary reference). fpriv->master can change (if the drm_file becomes newly minted master and wasnt one before through the setmaster ioctl), but it cannot become NULL before it's completely gone from the system. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 8c0e0dba1611..f9267b21556e 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -66,7 +66,8 @@ static bool drm_is_current_master_locked(struct drm_file *fpriv) > lockdep_assert_once(lockdep_is_held(&fpriv->minor->dev->master_lookup_lock) || > lockdep_is_held(&fpriv->minor->dev->master_mutex)); > > - return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > + return (fpriv->is_master && fpriv->master && > + drm_lease_owner(fpriv->master) == fpriv->minor->dev->master); > } > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

[PATCH v2 0/5] dma-fence: Deadline awareness

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Based on discussion from a previous series[1] to add a "boost" mechanism when, for example, vblank deadlines are missed. Instead of a boost callback, this approach adds a way to set a deadline on the fence, by which the waiter would like to see the fence signalled. I've not yet had a chance to re-work the drm/msm part of this, but wanted to send this out as an RFC in case I don't have a chance to finish the drm/msm part this week. Original description: In some cases, like double-buffered rendering, missing vblanks can trick the GPU into running at a lower frequence, when really we want to be running at a higher frequency to not miss the vblanks in the first place. This is partially inspired by a trick i915 does, but implemented via dma-fence for a couple of reasons: 1) To continue to be able to use the atomic helpers 2) To support cases where display and gpu are different drivers [1] https://patchwork.freedesktop.org/series/90331/ v1: https://patchwork.freedesktop.org/series/93035/ v2: Move filtering out of later deadlines to fence implementation to avoid increasing the size of dma_fence Rob Clark (5): dma-fence: Add deadline awareness drm/vblank: Add helper to get next vblank time drm/atomic-helper: Set fence deadline for vblank drm/scheduler: Add fence deadline support drm/msm: Add deadline based boost support drivers/dma-buf/dma-fence.c | 20 +++++++ drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++ drivers/gpu/drm/drm_vblank.c | 31 ++++++++++ drivers/gpu/drm/msm/msm_fence.c | 76 +++++++++++++++++++++++++ drivers/gpu/drm/msm/msm_fence.h | 20 +++++++ drivers/gpu/drm/msm/msm_gpu.h | 1 + drivers/gpu/drm/msm/msm_gpu_devfreq.c | 20 +++++++ drivers/gpu/drm/scheduler/sched_fence.c | 25 ++++++++ drivers/gpu/drm/scheduler/sched_main.c | 3 + include/drm/drm_vblank.h | 1 + include/drm/gpu_scheduler.h | 6 ++ include/linux/dma-fence.h | 16 ++++++ 12 files changed, 255 insertions(+) -- 2.31.1

2 years, 8 months

Re: [Linaro-mm-sig] IIO, dmabuf, io_uring

by Christoph Hellwig

On Fri, Aug 13, 2021 at 01:41:26PM +0200, Paul Cercueil wrote: > Hi, > > A few months ago we (ADI) tried to upstream the interface we use with our > high-speed ADCs and DACs. It is a system with custom ioctls on the iio > device node to dequeue and enqueue buffers (allocated with > dma_alloc_coherent), that can then be mmap'd by userspace applications. > Anyway, it was ultimately denied entry [1]; this API was okay in ~2014 when > it was designed but it feels like re-inventing the wheel in 2021. > > Back to the drawing table, and we'd like to design something that we can > actually upstream. This high-speed interface looks awfully similar to > DMABUF, so we may try to implement a DMABUF interface for IIO, unless > someone has a better idea. To me this does sound a lot like a dma buf use case. The interesting question to me is how to signal arrival of new data, or readyness to consume more data. I suspect that people that are actually using dmabuf heavily at the moment (dri/media folks) might be able to chime in a little more on that. > Our first usecase is, we want userspace applications to be able to dequeue > buffers of samples (from ADCs), and/or enqueue buffers of samples (for > DACs), and to be able to manipulate them (mmapped buffers). With a DMABUF > interface, I guess the userspace application would dequeue a dma buffer > from the driver, mmap it, read/write the data, unmap it, then enqueue it to > the IIO driver again so that it can be disposed of. Does that sound sane? > > Our second usecase is - and that's where things get tricky - to be able to > stream the samples to another computer for processing, over Ethernet or > USB. Our typical setup is a high-speed ADC/DAC on a dev board with a FPGA > and a weak soft-core or low-power CPU; processing the data in-situ is not > an option. Copying the data from one buffer to another is not an option > either (way too slow), so we absolutely want zero-copy. > > Usual userspace zero-copy techniques (vmsplice+splice, MSG_ZEROCOPY etc) > don't really work with mmapped kernel buffers allocated for DMA [2] and/or > have a huge overhead, so the way I see it, we would also need DMABUF > support in both the Ethernet stack and USB (functionfs) stack. However, as > far as I understood, DMABUF is mostly a DRM/V4L2 thing, so I am really not > sure we have the right idea here. > > And finally, there is the new kid in town, io_uring. I am not very literate > about the topic, but it does not seem to be able to handle DMA buffers > (yet?). The idea that we could dequeue a buffer of samples from the IIO > device and send it over the network in one single syscall is appealing, > though. Think of io_uring really just as an async syscall layer. It doesn't replace DMA buffers, but can be used as a different and for some workloads more efficient way to dispatch syscalls.

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH] drivers:gpu:drm:amd:amdgpu:fix a potential use-after-free

by Christian König

Am 13.08.21 um 05:28 schrieb lwt105: > in line 1503, "dma_fence_put(fence);" drop the reference to fence and may > cause fence to be released. However, fence is used subsequently in line > 1510 "fence->error". This may result in an use-after-free bug. > > It can be fixed by recording fence->error in an variable before dropping > the reference to fence and referencing it after dropping. > > Signed-off-by: lwt105 <3061522931(a)qq.com> Good catch. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 30fa1f61e0e5..99d03180e113 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -1486,7 +1486,7 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev, > struct drm_amdgpu_fence *fences) > { > uint32_t fence_count = wait->in.fence_count; > - unsigned int i; > + unsigned int i, error; > long r = 1; Would be nice to have if you could reuse the "r" variable here instead of a new one. Regards, Christian. > > for (i = 0; i < fence_count; i++) { > @@ -1500,6 +1500,7 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev, > continue; > > r = dma_fence_wait_timeout(fence, true, timeout); > + error = fence->error; > dma_fence_put(fence); > if (r < 0) > return r; > @@ -1507,8 +1508,8 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev, > if (r == 0) > break; > > - if (fence->error) > - return fence->error; > + if (error) > + return error; > } > > memset(wait, 0, sizeof(*wait));

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH 2/2] drm: unexport drm_ioctl_permit

by Daniel Vetter

On Fri, Aug 13, 2021 at 04:54:50PM +0800, Desmond Cheong Zhi Xi wrote: > Since the last user of drm_ioctl_permit was removed, and it's now only > used in drm_ioctl.c, unexport the symbol. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> Applied to drm-misc-next for 5.16, thanks for your patch. -Daniel > --- > drivers/gpu/drm/drm_ioctl.c | 15 +-------------- > include/drm/drm_ioctl.h | 1 - > 2 files changed, 1 insertion(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index eb4ec3fab7d1..fe271f6f96ab 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -522,19 +522,7 @@ int drm_version(struct drm_device *dev, void *data, > return err; > } > > -/** > - * drm_ioctl_permit - Check ioctl permissions against caller > - * > - * @flags: ioctl permission flags. > - * @file_priv: Pointer to struct drm_file identifying the caller. > - * > - * Checks whether the caller is allowed to run an ioctl with the > - * indicated permissions. > - * > - * Returns: > - * Zero if allowed, -EACCES otherwise. > - */ > -int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) > +static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) > { > /* ROOT_ONLY is only for CAP_SYS_ADMIN */ > if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN))) > @@ -557,7 +545,6 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) > > return 0; > } > -EXPORT_SYMBOL(drm_ioctl_permit); > > #define DRM_IOCTL_DEF(ioctl, _func, _flags) \ > [DRM_IOCTL_NR(ioctl)] = { \ > diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h > index 13a68cdcea36..fd29842127e5 100644 > --- a/include/drm/drm_ioctl.h > +++ b/include/drm/drm_ioctl.h > @@ -174,7 +174,6 @@ struct drm_ioctl_desc { > .name = #ioctl \ > } > > -int drm_ioctl_permit(u32 flags, struct drm_file *file_priv); > long drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); > long drm_ioctl_kernel(struct file *, drm_ioctl_t, void *, u32); > #ifdef CONFIG_COMPAT > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

Re: [Linaro-mm-sig] [PATCH 1/2] drm: avoid races with modesetting rights

by Daniel Vetter

On Fri, Aug 13, 2021 at 04:54:49PM +0800, Desmond Cheong Zhi Xi wrote: > In drm_client_modeset.c and drm_fb_helper.c, > drm_master_internal_{acquire,release} are used to avoid races with DRM > userspace. These functions hold onto drm_device.master_mutex while > committing, and bail if there's already a master. > > However, ioctls can still race between themselves. A > time-of-check-to-time-of-use error can occur if an ioctl that changes > the modeset has its rights revoked after it validates its permissions, > but before it completes. > > There are three ioctls that can affect modesetting permissions: > > - DROP_MASTER ioctl removes rights for a master and its leases > > - REVOKE_LEASE ioctl revokes rights for a specific lease > > - SET_MASTER ioctl sets the device master if the master role hasn't > been acquired yet > > All these races can be avoided by introducing an SRCU that acts as a > barrier for ioctls that can change modesetting permissions. Processes > that perform modesetting should hold a read lock on the new > drm_device.master_barrier_srcu, and ioctls that change these > permissions should call synchronize_srcu before returning. > > This ensures that any process that might have seen old permissions are > flushed out before DROP_MASTER/REVOKE_LEASE/SET_MASTER ioctls return > to userspace. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> This looks pretty solid, but I think there's one gap where we can still race. Scenario. Process A has a drm fd with master rights and two threads: - thread 1 does a long-running display operation (like a modeset or whatever) - thread 2 does a drop-master Then we start a new process B, which acquires master in drm_open (there is no other one left). This is like setmaster ioctl, but your DRM_MASTER_FLUSH bit doesn't work there. The other thing is that for modeset stuff (which this all is) srcu is probably massive overkill, and a simple rwsem should be good enough too. Maybe even better, since the rwsem guarantees that no new reader can start once you try to acquire the write side. Finally, and this is a bit a bikeshed: I don't like much how DRM_MASTER_FLUSH leaks the need of these very few places into the very core drm_ioctl function. One idea I had was to use task_work in a special function, roughly void master_flush() { down_write(master_rwsem); up_write(master_rwms); } void drm_master_flush() { init_task_work(fpriv->master_flush_work, master_flush) task_work_add(fpriv->master_flush_work); /* if task_work_add fails we're exiting, at which point the lack * of master flush doesn't matter); } And maybe put a comment above the function explaining why and how this works. We could even do a drm_master_unlock_and_flush helper, since that's really what everyone wants, and it would make it very clear which master state changes need this flush. Instead of setting a flag bit in an ioctl table very far away ... Thoughts? -Daniel > --- > drivers/gpu/drm/drm_auth.c | 17 ++++++++++++++--- > drivers/gpu/drm/drm_client_modeset.c | 10 ++++++---- > drivers/gpu/drm/drm_drv.c | 2 ++ > drivers/gpu/drm/drm_fb_helper.c | 20 ++++++++++++-------- > drivers/gpu/drm/drm_internal.h | 5 +++-- > drivers/gpu/drm/drm_ioctl.c | 25 +++++++++++++++++++++---- > include/drm/drm_device.h | 11 +++++++++++ > include/drm/drm_ioctl.h | 7 +++++++ > 8 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 60a6b21474b1..004506608e76 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -29,6 +29,7 @@ > */ > > #include <linux/slab.h> > +#include <linux/srcu.h> > > #include <drm/drm_auth.h> > #include <drm/drm_drv.h> > @@ -448,21 +449,31 @@ void drm_master_put(struct drm_master **master) > EXPORT_SYMBOL(drm_master_put); > > /* Used by drm_client and drm_fb_helper */ > -bool drm_master_internal_acquire(struct drm_device *dev) > +bool drm_master_internal_acquire(struct drm_device *dev, int *idx) > { > + *idx = srcu_read_lock(&dev->master_barrier_srcu); > + > mutex_lock(&dev->master_mutex); > if (dev->master) { > mutex_unlock(&dev->master_mutex); > + srcu_read_unlock(&dev->master_barrier_srcu, *idx); > return false; > } > + mutex_unlock(&dev->master_mutex); > > return true; > } > EXPORT_SYMBOL(drm_master_internal_acquire); > > /* Used by drm_client and drm_fb_helper */ > -void drm_master_internal_release(struct drm_device *dev) > +void drm_master_internal_release(struct drm_device *dev, int idx) > { > - mutex_unlock(&dev->master_mutex); > + srcu_read_unlock(&dev->master_barrier_srcu, idx); > } > EXPORT_SYMBOL(drm_master_internal_release); > + > +/* Used by drm_ioctl */ > +void drm_master_flush(struct drm_device *dev) > +{ > + synchronize_srcu(&dev->master_barrier_srcu); > +} > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index ced09c7c06f9..9885f36f71b7 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -1165,13 +1165,14 @@ int drm_client_modeset_commit(struct drm_client_dev *client) > { > struct drm_device *dev = client->dev; > int ret; > + int idx; > > - if (!drm_master_internal_acquire(dev)) > + if (!drm_master_internal_acquire(dev, &idx)) > return -EBUSY; > > ret = drm_client_modeset_commit_locked(client); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > > return ret; > } > @@ -1215,8 +1216,9 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > { > struct drm_device *dev = client->dev; > int ret = 0; > + int idx; > > - if (!drm_master_internal_acquire(dev)) > + if (!drm_master_internal_acquire(dev, &idx)) > return -EBUSY; > > mutex_lock(&client->modeset_mutex); > @@ -1226,7 +1228,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode) > drm_client_modeset_dpms_legacy(client, mode); > mutex_unlock(&client->modeset_mutex); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > > return ret; > } > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 7a5097467ba5..c313f0674db3 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -574,6 +574,7 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) > mutex_destroy(&dev->clientlist_mutex); > mutex_destroy(&dev->filelist_mutex); > mutex_destroy(&dev->struct_mutex); > + cleanup_srcu_struct(&dev->master_barrier_srcu); > drm_legacy_destroy_members(dev); > } > > @@ -612,6 +613,7 @@ static int drm_dev_init(struct drm_device *dev, > mutex_init(&dev->filelist_mutex); > mutex_init(&dev->clientlist_mutex); > mutex_init(&dev->master_mutex); > + init_srcu_struct(&dev->master_barrier_srcu); > > ret = drmm_add_action(dev, drm_dev_init_release, NULL); > if (ret) > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 3ab078321045..0d594bc15f18 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1116,13 +1116,14 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > int ret; > + int idx; > > if (oops_in_progress) > return -EBUSY; > > mutex_lock(&fb_helper->lock); > > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1136,7 +1137,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > ret = setcmap_legacy(cmap, info); > mutex_unlock(&fb_helper->client.modeset_mutex); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > > @@ -1160,9 +1161,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > struct drm_device *dev = fb_helper->dev; > struct drm_crtc *crtc; > int ret = 0; > + int idx; > > mutex_lock(&fb_helper->lock); > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1204,7 +1206,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, > ret = -ENOTTY; > } > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > return ret; > @@ -1474,12 +1476,13 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > struct drm_fb_helper *fb_helper = info->par; > struct drm_device *dev = fb_helper->dev; > int ret; > + int idx; > > if (oops_in_progress) > return -EBUSY; > > mutex_lock(&fb_helper->lock); > - if (!drm_master_internal_acquire(dev)) { > + if (!drm_master_internal_acquire(dev, &idx)) { > ret = -EBUSY; > goto unlock; > } > @@ -1489,7 +1492,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > else > ret = pan_display_legacy(var, info); > > - drm_master_internal_release(dev); > + drm_master_internal_release(dev, idx); > unlock: > mutex_unlock(&fb_helper->lock); > > @@ -1948,6 +1951,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > int err = 0; > + int idx; > > if (!drm_fbdev_emulation || !fb_helper) > return 0; > @@ -1959,13 +1963,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > return err; > } > > - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { > + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev, &idx)) { > fb_helper->delayed_hotplug = true; > mutex_unlock(&fb_helper->lock); > return err; > } > > - drm_master_internal_release(fb_helper->dev); > + drm_master_internal_release(fb_helper->dev, idx); > > drm_dbg_kms(fb_helper->dev, "\n"); > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 17f3548c8ed2..578fd2769913 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -142,8 +142,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > int drm_master_open(struct drm_file *file_priv); > void drm_master_release(struct drm_file *file_priv); > -bool drm_master_internal_acquire(struct drm_device *dev); > -void drm_master_internal_release(struct drm_device *dev); > +bool drm_master_internal_acquire(struct drm_device *dev, int *idx); > +void drm_master_internal_release(struct drm_device *dev, int idx); > +void drm_master_flush(struct drm_device *dev); > > /* drm_sysfs.c */ > extern struct class *drm_class; > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index be4a52dc4d6f..eb4ec3fab7d1 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -600,8 +600,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH), > > - DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, 0), > - DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, 0), > + DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, > + DRM_MASTER_FLUSH), > + DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, > + DRM_MASTER_FLUSH), > > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY), > DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), > @@ -722,7 +724,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER), > - DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER), > + DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, > + DRM_MASTER | DRM_MASTER_FLUSH), > }; > > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) > @@ -781,13 +784,17 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > struct drm_file *file_priv = file->private_data; > struct drm_device *dev = file_priv->minor->dev; > int retcode; > + int idx; > > if (drm_dev_is_unplugged(dev)) > return -ENODEV; > > + if (unlikely(flags & DRM_MASTER)) > + idx = srcu_read_lock(&dev->master_barrier_srcu); > + > retcode = drm_ioctl_permit(flags, file_priv); > if (unlikely(retcode)) > - return retcode; > + goto release_master; > > /* Enforce sane locking for modern driver ioctls. */ > if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || > @@ -798,6 +805,16 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > retcode = func(dev, kdata, file_priv); > mutex_unlock(&drm_global_mutex); > } > + > +release_master: > + if (unlikely(flags & DRM_MASTER)) > + srcu_read_unlock(&dev->master_barrier_srcu, idx); > + /* After flushing, processes are guaranteed to see the new master/lease > + * permissions, and any process which might have seen the old > + * permissions is guaranteed to have finished. > + */ > + if (unlikely(flags & DRM_MASTER_FLUSH)) > + drm_master_flush(dev); > return retcode; > } > EXPORT_SYMBOL(drm_ioctl_kernel); > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 604b1d1b2d72..0ac5fdb375f8 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -111,6 +111,17 @@ struct drm_device { > */ > struct drm_master *master; > > + /** > + * @master_barrier_srcu: > + * > + * Used to synchronize modesetting rights between multiple users. Users > + * that can change the modeset or display state must hold an > + * srcu_read_lock() on @master_barrier_srcu, and ioctls that can change > + * modesetting rights should call synchronize_srcu() before returning > + * to userspace. > + */ > + struct srcu_struct master_barrier_srcu; > + > /** > * @driver_features: per-device driver features > * > diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h > index afb27cb6a7bd..13a68cdcea36 100644 > --- a/include/drm/drm_ioctl.h > +++ b/include/drm/drm_ioctl.h > @@ -130,6 +130,13 @@ enum drm_ioctl_flags { > * not set DRM_AUTH because they do not require authentication. > */ > DRM_RENDER_ALLOW = BIT(5), > + /** > + * @DRM_MASTER_FLUSH: > + * > + * This must be set for any ioctl which can change the modesetting > + * permissions for DRM users. > + */ > + DRM_MASTER_FLUSH = BIT(6), > }; > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

2 years, 8 months

[PATCH v5 13/20] drm/gem: Delete gem array fencing helpers

by Daniel Vetter

Integrated into the scheduler now and all users converted over. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/drm_gem.c | 96 --------------------------------------- include/drm/drm_gem.h | 5 -- 2 files changed, 101 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 09c820045859..37e2e2820f08 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1272,99 +1272,3 @@ drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, ww_acquire_fini(acquire_ctx); } EXPORT_SYMBOL(drm_gem_unlock_reservations); - -/** - * drm_gem_fence_array_add - Adds the fence to an array of fences to be - * waited on, deduplicating fences from the same context. - * - * @fence_array: array of dma_fence * for the job to block on. - * @fence: the dma_fence to add to the list of dependencies. - * - * This functions consumes the reference for @fence both on success and error - * cases. - * - * Returns: - * 0 on success, or an error on failing to expand the array. - */ -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence) -{ - struct dma_fence *entry; - unsigned long index; - u32 id = 0; - int ret; - - if (!fence) - return 0; - - /* Deduplicate if we already depend on a fence from the same context. - * This lets the size of the array of deps scale with the number of - * engines involved, rather than the number of BOs. - */ - xa_for_each(fence_array, index, entry) { - if (entry->context != fence->context) - continue; - - if (dma_fence_is_later(fence, entry)) { - dma_fence_put(entry); - xa_store(fence_array, index, fence, GFP_KERNEL); - } else { - dma_fence_put(fence); - } - return 0; - } - - ret = xa_alloc(fence_array, &id, fence, xa_limit_32b, GFP_KERNEL); - if (ret != 0) - dma_fence_put(fence); - - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add); - -/** - * drm_gem_fence_array_add_implicit - Adds the implicit dependencies tracked - * in the GEM object's reservation object to an array of dma_fences for use in - * scheduling a rendering job. - * - * This should be called after drm_gem_lock_reservations() on your array of - * GEM objects used in the job but before updating the reservations with your - * own fences. - * - * @fence_array: array of dma_fence * for the job to block on. - * @obj: the gem object to add new dependencies from. - * @write: whether the job might write the object (so we need to depend on - * shared fences in the reservation object). - */ -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write) -{ - int ret; - struct dma_fence **fences; - unsigned int i, fence_count; - - if (!write) { - struct dma_fence *fence = - dma_resv_get_excl_unlocked(obj->resv); - - return drm_gem_fence_array_add(fence_array, fence); - } - - ret = dma_resv_get_fences(obj->resv, NULL, - &fence_count, &fences); - if (ret || !fence_count) - return ret; - - for (i = 0; i < fence_count; i++) { - ret = drm_gem_fence_array_add(fence_array, fences[i]); - if (ret) - break; - } - - for (; i < fence_count; i++) - dma_fence_put(fences[i]); - kfree(fences); - return ret; -} -EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 35e7f44c2a75..e55a767188af 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -407,11 +407,6 @@ int drm_gem_lock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); -int drm_gem_fence_array_add(struct xarray *fence_array, - struct dma_fence *fence); -int drm_gem_fence_array_add_implicit(struct xarray *fence_array, - struct drm_gem_object *obj, - bool write); int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset); -- 2.32.0

2 years, 8 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig August 2021