- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH] dma-buf: heaps: remove duplicated cache sync

by Christian König

Am 31.08.21 um 05:44 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > >> Am 30.08.21 um 12:01 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> Current flow, one dmabuf maybe call cache sync many times if >>> it has beed mapped more than one time. >> Well I'm not an expert on DMA heaps, but this will most likely not work >> correctly. >> > All attachments of one dmabuf will add into a list, I think it means dmabuf > supports map more than one time. Could you tell me more about it? Yes, that's correct and all of those needs to be synced as far as I know. See the dma_sync_sgtable_for_cpu() is intentionally for each SG table given out. >>> Is there any case that attachments of one dmabuf will points to >>> different memory? If not, seems do sync only one time is more better. >> I think that this can happen, yes. >> >> Christian. >> > Seems it's a very special case on Android, if you don't mind, could you > tell me more about it? That might be the case, nevertheless this change here is illegal from the DMA API point of view as far as I can see. Regards, Christian. > >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> --- >>> drivers/dma-buf/heaps/system_heap.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c >>> index 23a7e74ef966..909ef652a8c8 100644 >>> --- a/drivers/dma-buf/heaps/system_heap.c >>> +++ b/drivers/dma-buf/heaps/system_heap.c >>> @@ -162,9 +162,10 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, >>> invalidate_kernel_vmap_range(buffer->vaddr, buffer->len); >>> >>> list_for_each_entry(a, &buffer->attachments, list) { >>> - if (!a->mapped) >>> - continue; >>> - dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >>> + if (a->mapped) { >>> + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >>> + break; >>> + } >>> } >>> mutex_unlock(&buffer->lock); >>> >>> @@ -183,9 +184,10 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, >>> flush_kernel_vmap_range(buffer->vaddr, buffer->len); >>> >>> list_for_each_entry(a, &buffer->attachments, list) { >>> - if (!a->mapped) >>> - continue; >>> - dma_sync_sgtable_for_device(a->dev, a->table, direction); >>> + if (!a->mapped) { >>> + dma_sync_sgtable_for_device(a->dev, a->table, direction); >>> + break; >>> + } >>> } >>> mutex_unlock(&buffer->lock); >>>

3 years, 9 months

1
0
0 0

[PATCH v5 20/20] dma-resv: Give the docs a do-over

by Daniel Vetter

Specifically document the new/clarified rules around how the shared fences do not have any ordering requirements against the exclusive fence. But also document all the things a bit better, given how central struct dma_resv to dynamic buffer management the docs have been very inadequat. - Lots more links to other pieces of the puzzle. Unfortunately ttm_buffer_object has no docs, so no links :-( - Explain/complain a bit about dma_resv_locking_ctx(). I still don't like that one, but fixing the ttm call chains is going to be horrible. Plus we want to plug in real slowpath locking when we do that anyway. - Main part of the patch is some actual docs for struct dma_resv. Overall I think we still have a lot of bad naming in this area (e.g. dma_resv.fence is singular, but contains the multiple shared fences), but I think that's more indicative of how the semantics and rules are just not great. Another thing that's real awkard is how chaining exclusive fences right now means direct dma_resv.exclusive_fence pointer access with an rcu_assign_pointer. Not so great either. v2: - Fix a pile of typos (Matt, Jason) - Hammer it in that breaking the rules leads to use-after-free issues around dma-buf sharing (Christian) Reviewed-by: Christian König <christian.koenig(a)amd.com> Cc: Jason Ekstrand <jason(a)jlekstrand.net> Cc: Matthew Auld <matthew.auld(a)intel.com> Reviewed-by: Matthew Auld <matthew.auld(a)intel.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> 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/dma-buf/dma-resv.c | 24 ++++++--- include/linux/dma-buf.h | 7 +++ include/linux/dma-resv.h | 104 +++++++++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index e744fd87c63c..84fbe60629e3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -48,6 +48,8 @@ * write operations) or N shared fences (read operations). The RCU * mechanism is used to protect read access to fences from locked * write-side updates. + * + * See struct dma_resv for more details. */ DEFINE_WD_CLASS(reservation_ww_class); @@ -137,7 +139,11 @@ EXPORT_SYMBOL(dma_resv_fini); * @num_fences: number of fences we want to add * * Should be called before dma_resv_add_shared_fence(). Must - * be called with obj->lock held. + * be called with @obj locked through dma_resv_lock(). + * + * Note that the preallocated slots need to be re-reserved if @obj is unlocked + * at any time before calling dma_resv_add_shared_fence(). This is validated + * when CONFIG_DEBUG_MUTEXES is enabled. * * RETURNS * Zero for success, or -errno @@ -234,8 +240,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max); * @obj: the reservation object * @fence: the shared fence to add * - * Add a fence to a shared slot, obj->lock must be held, and + * Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and * dma_resv_reserve_shared() has been called. + * + * See also &dma_resv.fence for a discussion of the semantics. */ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -278,9 +286,11 @@ EXPORT_SYMBOL(dma_resv_add_shared_fence); /** * dma_resv_add_excl_fence - Add an exclusive fence. * @obj: the reservation object - * @fence: the shared fence to add + * @fence: the exclusive fence to add * - * Add a fence to the exclusive slot. The obj->lock must be held. + * Add a fence to the exclusive slot. @obj must be locked with dma_resv_lock(). + * Note that this function replaces all fences attached to @obj, see also + * &dma_resv.fence_excl for a discussion of the semantics. */ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -609,9 +619,11 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) * fence * * Callers are not required to hold specific locks, but maybe hold - * dma_resv_lock() already + * dma_resv_lock() already. + * * RETURNS - * true if all fences signaled, else false + * + * True if all fences signaled, else false. */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 678b2006be78..fc62b5f9980c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -420,6 +420,13 @@ struct dma_buf { * - Dynamic importers should set fences for any access that they can't * disable immediately from their &dma_buf_attach_ops.move_notify * callback. + * + * IMPORTANT: + * + * All drivers must obey the struct dma_resv rules, specifically the + * rules for updating fences, see &dma_resv.fence_excl and + * &dma_resv.fence. If these dependency rules are broken access tracking + * can be lost resulting in use after free issues. */ struct dma_resv *resv; diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index e1ca2080a1ff..9100dd3dc21f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -62,16 +62,90 @@ struct dma_resv_list { /** * struct dma_resv - a reservation object manages fences for a buffer - * @lock: update side lock - * @seq: sequence count for managing RCU read-side synchronization - * @fence_excl: the exclusive fence, if there is one currently - * @fence: list of current shared fences + * + * There are multiple uses for this, with sometimes slightly different rules in + * how the fence slots are used. + * + * One use is to synchronize cross-driver access to a struct dma_buf, either for + * dynamic buffer management or just to handle implicit synchronization between + * different users of the buffer in userspace. See &dma_buf.resv for a more + * in-depth discussion. + * + * The other major use is to manage access and locking within a driver in a + * buffer based memory manager. struct ttm_buffer_object is the canonical + * example here, since this is where reservation objects originated from. But + * use in drivers is spreading and some drivers also manage struct + * drm_gem_object with the same scheme. */ struct dma_resv { + /** + * @lock: + * + * Update side lock. Don't use directly, instead use the wrapper + * functions like dma_resv_lock() and dma_resv_unlock(). + * + * Drivers which use the reservation object to manage memory dynamically + * also use this lock to protect buffer object state like placement, + * allocation policies or throughout command submission. + */ struct ww_mutex lock; + + /** + * @seq: + * + * Sequence count for managing RCU read-side synchronization, allows + * read-only access to @fence_excl and @fence while ensuring we take a + * consistent snapshot. + */ seqcount_ww_mutex_t seq; + /** + * @fence_excl: + * + * The exclusive fence, if there is one currently. + * + * There are two ways to update this fence: + * + * - First by calling dma_resv_add_excl_fence(), which replaces all + * fences attached to the reservation object. To guarantee that no + * fences are lost, this new fence must signal only after all previous + * fences, both shared and exclusive, have signalled. In some cases it + * is convenient to achieve that by attaching a struct dma_fence_array + * with all the new and old fences. + * + * - Alternatively the fence can be set directly, which leaves the + * shared fences unchanged. To guarantee that no fences are lost, this + * new fence must signal only after the previous exclusive fence has + * signalled. Since the shared fences are staying intact, it is not + * necessary to maintain any ordering against those. If semantically + * only a new access is added without actually treating the previous + * one as a dependency the exclusive fences can be strung together + * using struct dma_fence_chain. + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_fence __rcu *fence_excl; + + /** + * @fence: + * + * List of current shared fences. + * + * There are no ordering constraints of shared fences against the + * exclusive fence slot. If a waiter needs to wait for all access, it + * has to wait for both sets of fences to signal. + * + * A new fence is added by calling dma_resv_add_shared_fence(). Since + * this often needs to be done past the point of no return in command + * submission it cannot fail, and therefore sufficient slots need to be + * reserved by calling dma_resv_reserve_shared(). + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_resv_list __rcu *fence; }; @@ -98,6 +172,13 @@ static inline void dma_resv_reset_shared_max(struct dma_resv *obj) {} * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow() called on @obj. + * + * Unlocked by calling dma_resv_unlock(). + * + * See also dma_resv_lock_interruptible() for the interruptible variant. */ static inline int dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -119,6 +200,12 @@ static inline int dma_resv_lock(struct dma_resv *obj, * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow_interruptible() called on + * @obj. + * + * Unlocked by calling dma_resv_unlock(). */ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -134,6 +221,8 @@ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, * Acquires the reservation object after a die case. This function * will sleep until the lock becomes available. See dma_resv_lock() as * well. + * + * See also dma_resv_lock_slow_interruptible() for the interruptible variant. */ static inline void dma_resv_lock_slow(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -167,7 +256,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj, * if they overlap with a writer. * * Also note that since no context is provided, no deadlock protection is - * possible. + * possible, which is also not needed for a trylock. * * Returns true if the lock was acquired, false otherwise. */ @@ -193,6 +282,11 @@ static inline bool dma_resv_is_locked(struct dma_resv *obj) * * Returns the context used to lock a reservation object or NULL if no context * was used or the object is not locked at all. + * + * WARNING: This interface is pretty horrible, but TTM needs it because it + * doesn't pass the struct ww_acquire_ctx around in some very long callchains. + * Everyone else just uses it to check whether they're holding a reservation or + * not. */ static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) { -- 2.32.0

3 years, 9 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: heaps: remove duplicated cache sync

by Christian König

Am 30.08.21 um 12:01 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Current flow, one dmabuf maybe call cache sync many times if > it has beed mapped more than one time. Well I'm not an expert on DMA heaps, but this will most likely not work correctly. > Is there any case that attachments of one dmabuf will points to > different memory? If not, seems do sync only one time is more better. I think that this can happen, yes. Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..909ef652a8c8 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -162,9 +162,10 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > invalidate_kernel_vmap_range(buffer->vaddr, buffer->len); > > list_for_each_entry(a, &buffer->attachments, list) { > - if (!a->mapped) > - continue; > - dma_sync_sgtable_for_cpu(a->dev, a->table, direction); > + if (a->mapped) { > + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); > + break; > + } > } > mutex_unlock(&buffer->lock); > > @@ -183,9 +184,10 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > flush_kernel_vmap_range(buffer->vaddr, buffer->len); > > list_for_each_entry(a, &buffer->attachments, list) { > - if (!a->mapped) > - continue; > - dma_sync_sgtable_for_device(a->dev, a->table, direction); > + if (!a->mapped) { > + dma_sync_sgtable_for_device(a->dev, a->table, direction); > + break; > + } > } > mutex_unlock(&buffer->lock); >

3 years, 9 months

1
0
0 0

[PATCH v5 12/20] drm/msm: Use scheduler dependency handling

by Daniel Vetter

drm_sched_job_init is already at the right place, so this boils down to deleting code. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Sean Paul <sean(a)poorly.run> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-arm-msm(a)vger.kernel.org Cc: freedreno(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/msm/msm_gem.h | 5 ----- drivers/gpu/drm/msm/msm_gem_submit.c | 19 +++++-------------- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 ------------ 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index f9e3ffb2309a..8bf0ac707fd7 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -312,11 +312,6 @@ struct msm_gem_submit { struct ww_acquire_ctx ticket; uint32_t seqno; /* Sequence number of the submit on the ring */ - /* Array of struct dma_fence * to block on before submitting this job. - */ - struct xarray deps; - unsigned long last_dep; - /* Hw fence, which is created when the scheduler executes the job, and * is signaled when the hw finishes (via seqno write from cmdstream) */ diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 96cea0ba4cfd..fb5a2eab27a2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); } - xa_init_flags(&submit->deps, XA_FLAGS_ALLOC); - kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace; @@ -72,8 +70,6 @@ void __msm_gem_submit_destroy(struct kref *kref) { struct msm_gem_submit *submit = container_of(kref, struct msm_gem_submit, ref); - unsigned long index; - struct dma_fence *fence; unsigned i; if (submit->fence_id) { @@ -82,12 +78,6 @@ void __msm_gem_submit_destroy(struct kref *kref) mutex_unlock(&submit->queue->lock); } - xa_for_each (&submit->deps, index, fence) { - dma_fence_put(fence); - } - - xa_destroy(&submit->deps); - dma_fence_put(submit->user_fence); dma_fence_put(submit->hw_fence); @@ -343,8 +333,9 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) if (no_implicit) continue; - ret = drm_gem_fence_array_add_implicit(&submit->deps, obj, - write); + ret = drm_sched_job_add_implicit_dependencies(&submit->base, + obj, + write); if (ret) break; } @@ -588,7 +579,7 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit, if (ret) break; - ret = drm_gem_fence_array_add(&submit->deps, fence); + ret = drm_sched_job_add_dependency(&submit->base, fence); if (ret) break; @@ -798,7 +789,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, goto out_unlock; } - ret = drm_gem_fence_array_add(&submit->deps, in_fence); + ret = drm_sched_job_add_dependency(&submit->base, in_fence); if (ret) goto out_unlock; } diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index bd54c1412649..652b1dedd7c1 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -11,17 +11,6 @@ static uint num_hw_submissions = 8; MODULE_PARM_DESC(num_hw_submissions, "The max # of jobs to write into ringbuffer (default 8)"); module_param(num_hw_submissions, uint, 0600); -static struct dma_fence *msm_job_dependency(struct drm_sched_job *job, - struct drm_sched_entity *s_entity) -{ - struct msm_gem_submit *submit = to_msm_submit(job); - - if (!xa_empty(&submit->deps)) - return xa_erase(&submit->deps, submit->last_dep++); - - return NULL; -} - static struct dma_fence *msm_job_run(struct drm_sched_job *job) { struct msm_gem_submit *submit = to_msm_submit(job); @@ -52,7 +41,6 @@ static void msm_job_free(struct drm_sched_job *job) } const struct drm_sched_backend_ops msm_sched_ops = { - .dependency = msm_job_dependency, .run_job = msm_job_run, .free_job = msm_job_free }; -- 2.32.0

3 years, 9 months

3
3
0 0

[PATCH v5 02/20] drm/msm: Fix drm/sched point of no return rules

by Daniel Vetter

Originally drm_sched_job_init was the point of no return, after which drivers must submit a job. I've split that up, which allows us to fix this issue pretty easily. Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things. Fixes: 1d8a5ca436ee ("drm/msm: Conversion to drm scheduler") Cc: Rob Clark <robdclark(a)chromium.org> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Sean Paul <sean(a)poorly.run> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-arm-msm(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: freedreno(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 6d6c44f0e1f3..d0ed4ddc509e 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,9 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); } - /* FIXME: this is way too early */ - drm_sched_job_arm(&job->base); - xa_init_flags(&submit->deps, XA_FLAGS_ALLOC); kref_init(&submit->ref); @@ -883,6 +880,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, submit->user_fence = dma_fence_get(&submit->base.s_fence->finished); + /* point of no return, we _have_ to submit no matter what */ + drm_sched_job_arm(&submit->base); + /* * Allocate an id which can be used by WAIT_FENCE ioctl to map back * to the underlying fence. @@ -892,17 +892,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0; - goto out; } - if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { + if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) { struct sync_file *sync_file = sync_file_create(submit->user_fence); if (!sync_file) { ret = -ENOMEM; - goto out; + } else { + fd_install(out_fence_fd, sync_file->file); + args->fence_fd = out_fence_fd; } - fd_install(out_fence_fd, sync_file->file); - args->fence_fd = out_fence_fd; } submit_attach_object_fences(submit); -- 2.32.0

3 years, 9 months

2
10
0 0

Re: [Linaro-mm-sig] [PATCH] parisc/parport_gsc: switch from 'pci_' to 'dma_' API

by Robin Murphy

On 2021-08-23 22:30, Christophe JAILLET wrote: > The wrappers in include/linux/pci-dma-compat.h should go away. > > The patch has been generated with the coccinelle script below. > > @@ > expression e1, e2, e3, e4; > @@ > - pci_free_consistent(e1, e2, e3, e4) > + dma_free_coherent(&e1->dev, e2, e3, e4) > > Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> > --- > If needed, see post from Christoph Hellwig on the kernel-janitors ML: > https://marc.info/?l=kernel-janitors&m=158745678307186&w=4 > > This has *NOT* been compile tested because I don't have the needed > configuration. > ssdfs > --- > drivers/parport/parport_gsc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/parport/parport_gsc.c b/drivers/parport/parport_gsc.c > index 1e43b3f399a8..db912fa6b6df 100644 > --- a/drivers/parport/parport_gsc.c > +++ b/drivers/parport/parport_gsc.c > @@ -390,9 +390,8 @@ static int __exit parport_remove_chip(struct parisc_device *dev) > if (p->irq != PARPORT_IRQ_NONE) > free_irq(p->irq, p); > if (priv->dma_buf) > - pci_free_consistent(priv->dev, PAGE_SIZE, > - priv->dma_buf, > - priv->dma_handle); > + dma_free_coherent(&priv->dev->dev, PAGE_SIZE, > + priv->dma_buf, priv->dma_handle); Hmm, seeing a free on its own made me wonder where the corresponding alloc was, but on closer inspection it seems there isn't one. AFAICS priv->dma_buf is only ever assigned with NULL (and priv->dev doesn't seem to be assigned at all), so this could likely just be removed. In fact it looks like all the references to DMA in this driver are just copy-paste from parport_pc and unused. Robin. > kfree (p->private_data); > parport_put_port(p); > kfree (ops); /* hope no-one cached it */ >

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [syzbot] WARNING in drm_gem_shmem_vm_open

by Daniel Vetter

On Fri, Aug 20, 2021 at 9:23 PM Thomas Zimmermann <tzimmermann(a)suse.de> wrote: > Hi > > Am 20.08.21 um 17:45 schrieb syzbot: > > syzbot has bisected this issue to: > > Good bot! > > > > > commit ea40d7857d5250e5400f38c69ef9e17321e9c4a2 > > Author: Daniel Vetter <daniel.vetter(a)ffwll.ch> > > Date: Fri Oct 9 23:21:56 2020 +0000 > > > > drm/vkms: fbdev emulation support > > Here's a guess. > > GEM SHMEM + fbdev emulation requires that > (drm_mode_config.prefer_shadow_fbdev = true). Otherwise, deferred I/O > and SHMEM conflict over the use of page flags IIRC. But we should only set up defio if fb->dirty is set, which vkms doesn't do. So there's something else going on? So there must be something else funny going on here I think ... No idea what's going on really. -Daniel > From a quick grep, vkms doesn't set prefer_shadow_fbdev and an alarming > amount of SHMEM-based drivers don't do either. > > Best regards > Thomas > > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11c31d55300000 > > start commit: 614cb2751d31 Merge tag 'trace-v5.14-rc6' of git://git.kern.. > > git tree: upstream > > final oops: https://syzkaller.appspot.com/x/report.txt?x=13c31d55300000 > > console output: https://syzkaller.appspot.com/x/log.txt?x=15c31d55300000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=96f0602203250753 > > dashboard link: https://syzkaller.appspot.com/bug?extid=91525b2bd4b5dff71619 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=122bce0e300000 > > > > Reported-by: syzbot+91525b2bd4b5dff71619(a)syzkaller.appspotmail.com > > Fixes: ea40d7857d52 ("drm/vkms: fbdev emulation support") > > > > For information about bisection process see: https://goo.gl/tpsmEJ#bisection > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [Intel-gfx] [PATCH v3 7/9] drm: update global mutex lock in the ioctl handler

by Daniel Vetter

On Thu, Aug 19, 2021 at 12:53 PM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > > On 18/8/21 7:02 pm, Daniel Vetter wrote: > > On Wed, Aug 18, 2021 at 03:38:22PM +0800, Desmond Cheong Zhi Xi wrote: > >> In a future patch, a read lock on drm_device.master_rwsem is > >> held in the ioctl handler before the check for ioctl > >> permissions. However, this produces the following lockdep splat: > >> > >> ====================================================== > >> WARNING: possible circular locking dependency detected > >> 5.14.0-rc6-CI-Patchwork_20831+ #1 Tainted: G U > >> ------------------------------------------------------ > >> kms_lease/1752 is trying to acquire lock: > >> ffffffff827bad88 (drm_global_mutex){+.+.}-{3:3}, at: drm_open+0x64/0x280 > >> > >> but task is already holding lock: > >> ffff88812e350108 (&dev->master_rwsem){++++}-{3:3}, at: > >> drm_ioctl_kernel+0xfb/0x1a0 > >> > >> which lock already depends on the new lock. > >> > >> the existing dependency chain (in reverse order) is: > >> > >> -> #2 (&dev->master_rwsem){++++}-{3:3}: > >> lock_acquire+0xd3/0x310 > >> down_read+0x3b/0x140 > >> drm_master_internal_acquire+0x1d/0x60 > >> drm_client_modeset_commit+0x10/0x40 > >> __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0 > >> drm_fb_helper_set_par+0x34/0x40 > >> intel_fbdev_set_par+0x11/0x40 [i915] > >> fbcon_init+0x270/0x4f0 > >> visual_init+0xc6/0x130 > >> do_bind_con_driver+0x1de/0x2c0 > >> do_take_over_console+0x10e/0x180 > >> do_fbcon_takeover+0x53/0xb0 > >> register_framebuffer+0x22d/0x310 > >> __drm_fb_helper_initial_config_and_unlock+0x36c/0x540 > >> intel_fbdev_initial_config+0xf/0x20 [i915] > >> async_run_entry_fn+0x28/0x130 > >> process_one_work+0x26d/0x5c0 > >> worker_thread+0x37/0x390 > >> kthread+0x13b/0x170 > >> ret_from_fork+0x1f/0x30 > >> > >> -> #1 (&helper->lock){+.+.}-{3:3}: > >> lock_acquire+0xd3/0x310 > >> __mutex_lock+0xa8/0x930 > >> __drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xb0 > >> intel_fbdev_restore_mode+0x2b/0x50 [i915] > >> drm_lastclose+0x27/0x50 > >> drm_release_noglobal+0x42/0x60 > >> __fput+0x9e/0x250 > >> task_work_run+0x6b/0xb0 > >> exit_to_user_mode_prepare+0x1c5/0x1d0 > >> syscall_exit_to_user_mode+0x19/0x50 > >> do_syscall_64+0x46/0xb0 > >> entry_SYSCALL_64_after_hwframe+0x44/0xae > >> > >> -> #0 (drm_global_mutex){+.+.}-{3:3}: > >> validate_chain+0xb39/0x1e70 > >> __lock_acquire+0x5a1/0xb70 > >> lock_acquire+0xd3/0x310 > >> __mutex_lock+0xa8/0x930 > >> drm_open+0x64/0x280 > >> drm_stub_open+0x9f/0x100 > >> chrdev_open+0x9f/0x1d0 > >> do_dentry_open+0x14a/0x3a0 > >> dentry_open+0x53/0x70 > >> drm_mode_create_lease_ioctl+0x3cb/0x970 > >> drm_ioctl_kernel+0xc9/0x1a0 > >> drm_ioctl+0x201/0x3d0 > >> __x64_sys_ioctl+0x6a/0xa0 > >> do_syscall_64+0x37/0xb0 > >> entry_SYSCALL_64_after_hwframe+0x44/0xae > >> > >> other info that might help us debug this: > >> Chain exists of: > >> drm_global_mutex --> &helper->lock --> &dev->master_rwsem > >> Possible unsafe locking scenario: > >> CPU0 CPU1 > >> ---- ---- > >> lock(&dev->master_rwsem); > >> lock(&helper->lock); > >> lock(&dev->master_rwsem); > >> lock(drm_global_mutex); > >> > >> *** DEADLOCK *** > >> > >> The lock hierarchy inversion happens because we grab the > >> drm_global_mutex while already holding on to master_rwsem. To avoid > >> this, we do some prep work to grab the drm_global_mutex before > >> checking for ioctl permissions. > >> > >> At the same time, we update the check for the global mutex to use the > >> drm_dev_needs_global_mutex helper function. > > > > This is intentional, essentially we force all non-legacy drivers to have > > unlocked ioctl (otherwise everyone forgets to set that flag). > > > > For non-legacy drivers the global lock only ensures ordering between > > drm_open and lastclose (I think at least), and between > > drm_dev_register/unregister and the backwards ->load/unload callbacks > > (which are called in the wrong place, but we cannot fix that for legacy > > drivers). > > > > ->load/unload should be completely unused (maybe radeon still uses it), > > and ->lastclose is also on the decline. > > > > Ah ok got it, I'll change the check back to > drm_core_check_feature(dev, DRIVER_LEGACY) then. > > > Maybe we should update the comment of drm_global_mutex to explain what it > > protects and why. > > > > The comments in drm_dev_needs_global_mutex make sense I think, I just > didn't read the code closely enough. > > > I'm also confused how this patch connects to the splat, since for i915 we > > Right, my bad, this is a separate instance of circular locking. I was > too hasty when I saw that for legacy drivers we might grab master_rwsem > then drm_global_mutex in the ioctl handler. > > > shouldn't be taking the drm_global_lock here at all. The problem seems to > > be the drm_open_helper when we create a new lease, which is an entirely > > different can of worms. > > > > I'm honestly not sure how to best do that, but we should be able to create > > a file and then call drm_open_helper directly, or well a version of that > > which never takes the drm_global_mutex. Because that is not needed for > > nested drm_file opening: > > - legacy drivers never go down this path because leases are only supported > > with modesetting, and modesetting is only supported for non-legacy > > drivers > > - the races against dev->open_count due to last_close or ->load callbacks > > don't matter, because for the entire ioctl we already have an open > > drm_file and that wont disappear. > > > > So this should work, but I'm not entirely sure how to make it work. > > -Daniel > > > > One idea that comes to mind is to change the outcome of > drm_dev_needs_global_mutex while we're in the ioctl, but that requires > more locking which sounds like a bad idea. > > Another idea, which is quite messy, but just for thoughts, uses the idea > of pushing the master_rwsem read lock down: Yeah I think that's cleaner, and I think that also should work a lot better for the other ioctls: - We don't have a need to flush readers anymore since we'll just take the rwsem in write mode - There's much less inversions, and maybe we could even get rid of the spinlock since at that point all readers should at least have the rwsem read-locked. > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 7f523e1c5650..5d05e744b728 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -712,7 +712,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_RENDER_ALLOW), > DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0), > DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0), > - DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER), > + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 0), > 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), > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index 983701198ffd..a25bc69522b4 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -500,6 +500,19 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > return -EINVAL; > } > > + /* Clone the lessor file to create a new file for us */ > + DRM_DEBUG_LEASE("Allocating lease file\n"); > + lessee_file = file_clone_open(lessor_file); > + if (IS_ERR(lessee_file)) > + return PTR_ERR(lessee_file); > + > + down_read(&dev->master_rwsem); > + > + if (!drm_is_current_master(lessor_priv)) { > + ret = -EACCES; > + goto out_file; > + } > + > lessor = drm_file_get_master(lessor_priv); > /* Do not allow sub-leases */ > if (lessor->lessor) { > @@ -547,14 +560,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > goto out_leases; > } > > - /* Clone the lessor file to create a new file for us */ > - DRM_DEBUG_LEASE("Allocating lease file\n"); > - lessee_file = file_clone_open(lessor_file); > - if (IS_ERR(lessee_file)) { > - ret = PTR_ERR(lessee_file); > - goto out_lessee; > - } > - > lessee_priv = lessee_file->private_data; > /* Change the file to a master one */ > drm_master_put(&lessee_priv->master); > @@ -571,17 +576,19 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > fd_install(fd, lessee_file); > > drm_master_put(&lessor); > + up_read(&dev->master_rwsem); > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); > return 0; > > -out_lessee: > - drm_master_put(&lessee); > - > out_leases: > put_unused_fd(fd); > > out_lessor: > drm_master_put(&lessor); > + > +out_file: > + up_read(&dev->master_rwsem); > + fput(lessee_file); > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); > return ret; > } > > > Something like this would also address the other deadlock we'd hit in > drm_mode_create_lease_ioctl(): > > drm_ioctl_kernel(): > down_read(&master_rwsem); <--- down_read() > drm_mode_create_lease_ioctl(): > drm_lease_create(): > file_clone_open(): > ... > drm_open(): > drm_open_helper(): > drm_master_open(): > down_write(&master_rwsem); <--- down_write() > > Overall, I think the suggestion to push master_rwsem write locks down > into ioctls would solve the nesting problem for those ioctls. Yup, my gut feeling agress. And the above is a nice solution without having to dig out all the code for creating a file directly (it's doable I think at least, we do it for dma-buf). > Although I'm still a little concerned that, just like here, there might > be deeply embedded nested locking, so locking becomes prone to breaking. > It does smell a bit to me. Yeah, that's pretty much the bane of locking cleanup/rework. You have to do it to figure out what goes boom :-/ Even with the most careful audit there's surprises left. -Daniel > >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > >> --- > >> drivers/gpu/drm/drm_ioctl.c | 18 +++++++++--------- > >> 1 file changed, 9 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > >> index 880fc565d599..2cb57378a787 100644 > >> --- a/drivers/gpu/drm/drm_ioctl.c > >> +++ b/drivers/gpu/drm/drm_ioctl.c > >> @@ -779,19 +779,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > >> if (drm_dev_is_unplugged(dev)) > >> return -ENODEV; > >> > >> + /* Enforce sane locking for modern driver ioctls. */ > >> + if (unlikely(drm_dev_needs_global_mutex(dev)) && !(flags & DRM_UNLOCKED)) > >> + mutex_lock(&drm_global_mutex); > >> + > >> retcode = drm_ioctl_permit(flags, file_priv); > >> if (unlikely(retcode)) > >> - return retcode; > >> + goto out; > >> > >> - /* Enforce sane locking for modern driver ioctls. */ > >> - if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || > >> - (flags & DRM_UNLOCKED)) > >> - retcode = func(dev, kdata, file_priv); > >> - else { > >> - mutex_lock(&drm_global_mutex); > >> - retcode = func(dev, kdata, file_priv); > >> + retcode = func(dev, kdata, file_priv); > >> + > >> +out: > >> + if (unlikely(drm_dev_needs_global_mutex(dev)) && !(flags & DRM_UNLOCKED)) > >> mutex_unlock(&drm_global_mutex); > >> - } > >> return retcode; > >> } > >> EXPORT_SYMBOL(drm_ioctl_kernel); > >> -- > >> 2.25.1 > >> > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 8/9] kernel: export task_work_add

by Christoph Hellwig

On Wed, Aug 18, 2021 at 03:38:23PM +0800, Desmond Cheong Zhi Xi wrote: > +EXPORT_SYMBOL(task_work_add); EXPORT_SYMBOL_GPL for this kinds of functionality, please.

3 years, 9 months

1
0
0 0

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

by Daniel Vetter

On Wed, Aug 18, 2021 at 5:37 PM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > > On 18/8/21 6:11 pm, Daniel Vetter wrote: > > 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. > > > > Right, will drop those two parts from the patch. > > >> 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? > > > > Sounds good. This can actually also happen in the failure path of > mock_drm_getfile if anon_inode_getfile fails. I'll leave a short note > about both of them. > > > 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 > > > Hmmm maybe the first option to move the check out of the lock might be > better. If I'm not wrong, we would otherwise also need to move > drm_master_release into drm_client_close. Do we have to? If I haven't missed anything, the drm_client stuff only calls drm_file_alloc and doesn't set up a master. So this should work? -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

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drm/prime: fix a potential double put (release) bug

by Christian König

Am 18.08.21 um 15:02 schrieb Wentao_Liang: > In line 317 (#1), drm_gem_prime_import() is called, it will call > drm_gem_prime_import_dev(). At the end of the function > drm_gem_prime_import_dev() (line 956, #2), "dma_buf_put(dma_buf);" puts > dma_buf->file and may cause it to be released. However, after > drm_gem_prime_import() returning, the dma_buf may be put again by the > same put function in lines 342, 351 and 358 (#3, #4, #5). Putting the > dma_buf improperly more than once can lead to an incorrect dma_buf- >> file put. > We believe that the put of the dma_buf in the function > drm_gem_prime_import() is unnecessary (#2). We can fix the above bug by > removing the redundant "dma_buf_put(dma_buf);" in line 956. Guys I'm getting tired of NAKing those incorrect reference count analysis. The dma_buf_put() in the error handling of drm_gem_prime_import_dev() function is balanced with the get_dma_buf() in the same function directly above. This is for the creating a GEM object for a DMA-buf imported from other device use case and certainly correct. The various dma_buf_put() in drm_gem_prime_fd_to_handle() is balanced with the dma_buf_get(prime_fd) at the beginning of the function. This is for extracting the DMA-buf from the file descriptor and keeping a reference to it while we are busy importing it (e.g. to prevent a race when somebody changes the fd at the same time). As far as I can see this is correct as well. Regards, Christian. > > 314 if (dev->driver->gem_prime_import) > 315 obj = dev->driver->gem_prime_import(dev, dma_buf); > 316 else > 317 obj = drm_gem_prime_import(dev, dma_buf); > //#1 call to drm_gem_prime_import > // ->drm_gem_prime_import_dev > // ->dma_buf_put > ... > > 336 ret = drm_prime_add_buf_handle(&file_priv->prime, > 337 dma_buf, *handle); > > ... > > 342 dma_buf_put(dma_buf); //#3 put again > 343 > 344 return 0; > 345 > 346 fail: > > 351 dma_buf_put(dma_buf); //#4 put again > 352 return ret; > > 356 out_put: > 357 mutex_unlock(&file_priv->prime.lock); > 358 dma_buf_put(dma_buf); //#5 put again > 359 return ret; > 360 } > > 905 struct drm_gem_object *drm_gem_prime_import_dev > (struct drm_device *dev, > 906 struct dma_buf *dma_buf, > 907 struct device *attach_dev) > 908 { > > ... > > 952 fail_unmap: > 953 dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); > 954 fail_detach: > 955 dma_buf_detach(dma_buf, attach); > 956 dma_buf_put(dma_buf); //#2 the first put of dma_buf > // (unnecessary) > 957 > 958 return ERR_PTR(ret); > 959 } > > Signed-off-by: Wentao_Liang <Wentao_Liang_g(a)163.com> > --- > drivers/gpu/drm/drm_prime.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 2a54f86856af..cef03ad0d5cd 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -953,7 +953,6 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, > dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); > fail_detach: > dma_buf_detach(dma_buf, attach); > - dma_buf_put(dma_buf); > > return ERR_PTR(ret); > }

3 years, 9 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

by Christian König

Am 18.08.21 um 15:13 schrieb Sa, Nuno: >> From: Christian König <christian.koenig(a)amd.com> >> Sent: Wednesday, August 18, 2021 2:58 PM >> To: Daniel Vetter <daniel(a)ffwll.ch> >> Cc: Sa, Nuno <Nuno.Sa(a)analog.com>; linaro-mm-sig(a)lists.linaro.org; >> dri-devel(a)lists.freedesktop.org; linux-media(a)vger.kernel.org; Rob >> Clark <rob(a)ti.com> >> Subject: Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if >> dmabuf object is NULL >> >> [External] >> >> Am 18.08.21 um 14:46 schrieb Daniel Vetter: >>> On Wed, Aug 18, 2021 at 02:31:34PM +0200, Christian König wrote: >>>> Am 18.08.21 um 14:17 schrieb Sa, Nuno: >>>>>> From: Christian König <christian.koenig(a)amd.com> >>>>>> Sent: Wednesday, August 18, 2021 2:10 PM >>>>>> To: Sa, Nuno <Nuno.Sa(a)analog.com>; linaro-mm- >> sig(a)lists.linaro.org; >>>>>> dri-devel(a)lists.freedesktop.org; linux-media(a)vger.kernel.org >>>>>> Cc: Rob Clark <rob(a)ti.com>; Sumit Semwal >>>>>> <sumit.semwal(a)linaro.org> >>>>>> Subject: Re: [PATCH] dma-buf: return -EINVAL if dmabuf object >> is >>>>>> NULL >>>>>> >>>>>> [External] >>>>>> >>>>>> To be honest I think the if(WARN_ON(!dmabuf)) return -EINVAL >>>>>> handling >>>>>> here is misleading in the first place. >>>>>> >>>>>> Returning -EINVAL on a hard coding error is not good practice and >>>>>> should >>>>>> probably be removed from the DMA-buf subsystem in general. >>>>> Would you say to just return 0 then? I don't think that having the >>>>> dereference is also good.. >>>> No, just run into the dereference. >>>> >>>> Passing NULL as the core object you are working on is a hard coding >> error >>>> and not something we should bubble up as recoverable error. >>>> >>>>> I used -EINVAL to be coherent with the rest of the code. >>>> I rather suggest to remove the check elsewhere as well. >>> It's a lot more complicated, and WARN_ON + bail out is rather >>> well-established code-pattern. There's been plenty of discussions in >> the >>> past that a BUG_ON is harmful since it makes debugging a major >> pain, e.g. >>> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefens… >> ook.com/?url=https*3A*2F*2Flore.kernel.org*2Flkml*2FCA*2B55aFw >> yNTLuZgOWMTRuabWobF27ygskuxvFd-P0n- >> 3UNT*3D0Og*40mail.gmail.com*2F&data=04*7C01*7Cchristian.k >> oenig*40amd.com*7C19f53e2a2d1843b65adc08d962463b78*7C3dd896 >> 1fe4884e608e11a82d994e183d*7C0*7C0*7C637648876076613233*7CU >> nknown*7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL >> CJBTiI6Ik1haWwiLCJXVCI6Mn0*3D*7C1000&sdata=ajyBnjePRak3 >> o7ObpBAuJNd08HgkANM9C*2BgzOAeHrMk*3D&reserved=0__;J >> SUlJSUlJSUlJSUlJSUlJSUlJSUlJSU!!A3Ni8CS0y2Y!qiDegx4svPUMZrvnzUo >> X7VKvvFpDcedH9gYbRCiWfe_N3fw4zpmA54qxefvMiQ$ >>> There's also a checkpatch check for this. >>> >>> commit 9d3e3c705eb395528fd8f17208c87581b134da48 >>> Author: Joe Perches <joe(a)perches.com> >>> Date: Wed Sep 9 15:37:27 2015 -0700 >>> >>> checkpatch: add warning on BUG/BUG_ON use >>> >>> Anyone who is paranoid about security crashes their machine on any >> WARNING >>> anyway (like syzkaller does). >>> >>> My rule of thumb is that if the WARN_ON + bail-out code is just an if >>> (WARN_ON()) return; then it's fine, if it's more then BUG_ON is the >> better >>> choice perhaps. >>> >>> I think the worst choice is just removing all these checks, because a >> few >>> code reorgs later you might not Oops immediately afterwards >> anymore, and >>> then we'll merge potentially very busted new code. Which is no >> good. >> >> Well BUG_ON(some_codition) is a different problem which I agree on >> with >> Linus that this is problematic. >> >> But "if (WARN_ON(!dmabuf)) return -EINVAL;" is really bad coding >> style >> as well since it hides real problems which are hard errors behind >> warnings. > I agree that doing these kind of checks in the core object of an API is > abusing parameter "validation". I guess a good pattern is having the > warning and let the code flow. But since these checks are already all > over the place I'm not sure the way to go. I'm very new to dma-buf > and I was just checking the code and saw this was not be coherent with > the rest of the API so I thought it would be a straight easy patch... Well, > I could not be more wrong :) Well that existing stuff might actually depend on this is a really good argument to keep it for now or at least until we have a consent on what to do. > Anyways, depending on what's decided, I can send another patch trying > to make these stuff more coherent. At this point, my feeling is that this > patch is to be dropped... At least for now I would hold it back. Thanks, Christian. > > - Nuno Sá > >> Returning -EINVAL indicates a recoverable error which is usually caused >> by userspace giving invalid parameters and should never be abused to >> indicate a driver coding error. >> >> Functions are either intended to take NULL as valid parameter, e.g. like >> kfree(NULL). Or they are intended to work on an object which is >> mandatory to provide. >> >> Christian. >> >>> -Daniel >>> >>> >>> >>>> Christian. >>>> >>>>> - Nuno Sá >>>>> >>>>>> Christian. >>>>>> >>>>>> Am 18.08.21 um 13:58 schrieb Nuno Sá: >>>>>>> On top of warning about a NULL object, we also want to return >> with a >>>>>>> proper error code (as done in 'dma_buf_begin_cpu_access()'). >>>>>> Otherwise, >>>>>>> we will get a NULL pointer dereference. >>>>>>> >>>>>>> Fixes: fc13020e086b ("dma-buf: add support for kernel cpu >> access") >>>>>>> Signed-off-by: Nuno Sá <nuno.sa(a)analog.com> >>>>>>> --- >>>>>>> drivers/dma-buf/dma-buf.c | 3 ++- >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma- >> buf/dma- >>>>>> buf.c >>>>>>> index 63d32261b63f..8ec7876dd523 100644 >>>>>>> --- a/drivers/dma-buf/dma-buf.c >>>>>>> +++ b/drivers/dma-buf/dma-buf.c >>>>>>> @@ -1231,7 +1231,8 @@ int dma_buf_end_cpu_access(struct >>>>>> dma_buf *dmabuf, >>>>>>> { >>>>>>> int ret = 0; >>>>>>> >>>>>>> - WARN_ON(!dmabuf); >>>>>>> + if (WARN_ON(!dmabuf)) >>>>>>> + return -EINVAL; >>>>>>> >>>>>>> might_lock(&dmabuf->resv->lock.base); >>>>>>> >>>> _______________________________________________ >>>> Linaro-mm-sig mailing list >>>> Linaro-mm-sig(a)lists.linaro.org >>>> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefens… >> ook.com/?url=https*3A*2F*2Flists.linaro.org*2Fmailman*2Flistinfo*2 >> Flinaro-mm- >> sig&data=04*7C01*7Cchristian.koenig*40amd.com*7C19f53e2a2 >> d1843b65adc08d962463b78*7C3dd8961fe4884e608e11a82d994e183d* >> 7C0*7C0*7C637648876076613233*7CUnknown*7CTWFpbGZsb3d8eyJ >> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 >> *3D*7C1000&sdata=0E5L4Kid5ZPeKT8Uxx7K61fBXmI4TOsz*2F5IL >> sFpLB*2Fo*3D&reserved=0__;JSUlJSUlJSUlJSUlJSUlJSUlJSUl!!A3N >> i8CS0y2Y!qiDegx4svPUMZrvnzUoX7VKvvFpDcedH9gYbRCiWfe_N3fw4z >> pmA54oQstzSNA$

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

by Christian König

Am 18.08.21 um 14:17 schrieb Sa, Nuno: >> From: Christian König <christian.koenig(a)amd.com> >> Sent: Wednesday, August 18, 2021 2:10 PM >> To: Sa, Nuno <Nuno.Sa(a)analog.com>; linaro-mm-sig(a)lists.linaro.org; >> dri-devel(a)lists.freedesktop.org; linux-media(a)vger.kernel.org >> Cc: Rob Clark <rob(a)ti.com>; Sumit Semwal >> <sumit.semwal(a)linaro.org> >> Subject: Re: [PATCH] dma-buf: return -EINVAL if dmabuf object is >> NULL >> >> [External] >> >> To be honest I think the if(WARN_ON(!dmabuf)) return -EINVAL >> handling >> here is misleading in the first place. >> >> Returning -EINVAL on a hard coding error is not good practice and >> should >> probably be removed from the DMA-buf subsystem in general. > Would you say to just return 0 then? I don't think that having the > dereference is also good.. No, just run into the dereference. Passing NULL as the core object you are working on is a hard coding error and not something we should bubble up as recoverable error. > I used -EINVAL to be coherent with the rest of the code. I rather suggest to remove the check elsewhere as well. Christian. > > - Nuno Sá > >> Christian. >> >> Am 18.08.21 um 13:58 schrieb Nuno Sá: >>> On top of warning about a NULL object, we also want to return with a >>> proper error code (as done in 'dma_buf_begin_cpu_access()'). >> Otherwise, >>> we will get a NULL pointer dereference. >>> >>> Fixes: fc13020e086b ("dma-buf: add support for kernel cpu access") >>> Signed-off-by: Nuno Sá <nuno.sa(a)analog.com> >>> --- >>> drivers/dma-buf/dma-buf.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma- >> buf.c >>> index 63d32261b63f..8ec7876dd523 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -1231,7 +1231,8 @@ int dma_buf_end_cpu_access(struct >> dma_buf *dmabuf, >>> { >>> int ret = 0; >>> >>> - WARN_ON(!dmabuf); >>> + if (WARN_ON(!dmabuf)) >>> + return -EINVAL; >>> >>> might_lock(&dmabuf->resv->lock.base); >>>

3 years, 9 months

2
2
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: return -EINVAL if dmabuf object is NULL

by Christian König

To be honest I think the if(WARN_ON(!dmabuf)) return -EINVAL handling here is misleading in the first place. Returning -EINVAL on a hard coding error is not good practice and should probably be removed from the DMA-buf subsystem in general. Christian. Am 18.08.21 um 13:58 schrieb Nuno Sá: > On top of warning about a NULL object, we also want to return with a > proper error code (as done in 'dma_buf_begin_cpu_access()'). Otherwise, > we will get a NULL pointer dereference. > > Fixes: fc13020e086b ("dma-buf: add support for kernel cpu access") > Signed-off-by: Nuno Sá <nuno.sa(a)analog.com> > --- > drivers/dma-buf/dma-buf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 63d32261b63f..8ec7876dd523 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1231,7 +1231,8 @@ int dma_buf_end_cpu_access(struct dma_buf *dmabuf, > { > int ret = 0; > > - WARN_ON(!dmabuf); > + if (WARN_ON(!dmabuf)) > + return -EINVAL; > > might_lock(&dmabuf->resv->lock.base); >

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 8/9] kernel: export task_work_add

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:23PM +0800, Desmond Cheong Zhi Xi wrote: > The task_work_add function is needed to prevent userspace races with > DRM modesetting rights. > > Some DRM ioctls can change modesetting permissions while other > concurrent users are performing modesetting. To prevent races with > userspace, such functions should flush readers of old permissions > before returning to user mode. As the function that changes > permissions might itself be a reader of the old permissions, we intend > to schedule this flush using task_work_add. > > However, when DRM is compiled as a loadable kernel module without > exporting task_work_add, we get the following compilation error: > > ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined! > > Reported-by: kernel test robot <lkp(a)intel.com> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> Just realized another benefit of pushing the dev->master_rwsem write locks down into ioctls that need them: We wouldn't need this function here exported for use in drm. But also I'm not sure that works any better than the design in your current patch set ... -Daniel > --- > kernel/task_work.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/task_work.c b/kernel/task_work.c > index 1698fbe6f0e1..90000404af2b 100644 > --- a/kernel/task_work.c > +++ b/kernel/task_work.c > @@ -60,6 +60,7 @@ int task_work_add(struct task_struct *task, struct callback_head *work, > > return 0; > } > +EXPORT_SYMBOL(task_work_add); > > /** > * task_work_cancel_match - cancel a pending work added by task_work_add() > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 7/9] drm: update global mutex lock in the ioctl handler

by Daniel Vetter

On Wed, Aug 18, 2021 at 03:38:22PM +0800, Desmond Cheong Zhi Xi wrote: > In a future patch, a read lock on drm_device.master_rwsem is > held in the ioctl handler before the check for ioctl > permissions. However, this produces the following lockdep splat: > > ====================================================== > WARNING: possible circular locking dependency detected > 5.14.0-rc6-CI-Patchwork_20831+ #1 Tainted: G U > ------------------------------------------------------ > kms_lease/1752 is trying to acquire lock: > ffffffff827bad88 (drm_global_mutex){+.+.}-{3:3}, at: drm_open+0x64/0x280 > > but task is already holding lock: > ffff88812e350108 (&dev->master_rwsem){++++}-{3:3}, at: > drm_ioctl_kernel+0xfb/0x1a0 > > which lock already depends on the new lock. > > the existing dependency chain (in reverse order) is: > > -> #2 (&dev->master_rwsem){++++}-{3:3}: > lock_acquire+0xd3/0x310 > down_read+0x3b/0x140 > drm_master_internal_acquire+0x1d/0x60 > drm_client_modeset_commit+0x10/0x40 > __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0 > drm_fb_helper_set_par+0x34/0x40 > intel_fbdev_set_par+0x11/0x40 [i915] > fbcon_init+0x270/0x4f0 > visual_init+0xc6/0x130 > do_bind_con_driver+0x1de/0x2c0 > do_take_over_console+0x10e/0x180 > do_fbcon_takeover+0x53/0xb0 > register_framebuffer+0x22d/0x310 > __drm_fb_helper_initial_config_and_unlock+0x36c/0x540 > intel_fbdev_initial_config+0xf/0x20 [i915] > async_run_entry_fn+0x28/0x130 > process_one_work+0x26d/0x5c0 > worker_thread+0x37/0x390 > kthread+0x13b/0x170 > ret_from_fork+0x1f/0x30 > > -> #1 (&helper->lock){+.+.}-{3:3}: > lock_acquire+0xd3/0x310 > __mutex_lock+0xa8/0x930 > __drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xb0 > intel_fbdev_restore_mode+0x2b/0x50 [i915] > drm_lastclose+0x27/0x50 > drm_release_noglobal+0x42/0x60 > __fput+0x9e/0x250 > task_work_run+0x6b/0xb0 > exit_to_user_mode_prepare+0x1c5/0x1d0 > syscall_exit_to_user_mode+0x19/0x50 > do_syscall_64+0x46/0xb0 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > -> #0 (drm_global_mutex){+.+.}-{3:3}: > validate_chain+0xb39/0x1e70 > __lock_acquire+0x5a1/0xb70 > lock_acquire+0xd3/0x310 > __mutex_lock+0xa8/0x930 > drm_open+0x64/0x280 > drm_stub_open+0x9f/0x100 > chrdev_open+0x9f/0x1d0 > do_dentry_open+0x14a/0x3a0 > dentry_open+0x53/0x70 > drm_mode_create_lease_ioctl+0x3cb/0x970 > drm_ioctl_kernel+0xc9/0x1a0 > drm_ioctl+0x201/0x3d0 > __x64_sys_ioctl+0x6a/0xa0 > do_syscall_64+0x37/0xb0 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > other info that might help us debug this: > Chain exists of: > drm_global_mutex --> &helper->lock --> &dev->master_rwsem > Possible unsafe locking scenario: > CPU0 CPU1 > ---- ---- > lock(&dev->master_rwsem); > lock(&helper->lock); > lock(&dev->master_rwsem); > lock(drm_global_mutex); > > *** DEADLOCK *** > > The lock hierarchy inversion happens because we grab the > drm_global_mutex while already holding on to master_rwsem. To avoid > this, we do some prep work to grab the drm_global_mutex before > checking for ioctl permissions. > > At the same time, we update the check for the global mutex to use the > drm_dev_needs_global_mutex helper function. This is intentional, essentially we force all non-legacy drivers to have unlocked ioctl (otherwise everyone forgets to set that flag). For non-legacy drivers the global lock only ensures ordering between drm_open and lastclose (I think at least), and between drm_dev_register/unregister and the backwards ->load/unload callbacks (which are called in the wrong place, but we cannot fix that for legacy drivers). ->load/unload should be completely unused (maybe radeon still uses it), and ->lastclose is also on the decline. Maybe we should update the comment of drm_global_mutex to explain what it protects and why. I'm also confused how this patch connects to the splat, since for i915 we shouldn't be taking the drm_global_lock here at all. The problem seems to be the drm_open_helper when we create a new lease, which is an entirely different can of worms. I'm honestly not sure how to best do that, but we should be able to create a file and then call drm_open_helper directly, or well a version of that which never takes the drm_global_mutex. Because that is not needed for nested drm_file opening: - legacy drivers never go down this path because leases are only supported with modesetting, and modesetting is only supported for non-legacy drivers - the races against dev->open_count due to last_close or ->load callbacks don't matter, because for the entire ioctl we already have an open drm_file and that wont disappear. So this should work, but I'm not entirely sure how to make it work. -Daniel > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > --- > drivers/gpu/drm/drm_ioctl.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 880fc565d599..2cb57378a787 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -779,19 +779,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata, > if (drm_dev_is_unplugged(dev)) > return -ENODEV; > > + /* Enforce sane locking for modern driver ioctls. */ > + if (unlikely(drm_dev_needs_global_mutex(dev)) && !(flags & DRM_UNLOCKED)) > + mutex_lock(&drm_global_mutex); > + > retcode = drm_ioctl_permit(flags, file_priv); > if (unlikely(retcode)) > - return retcode; > + goto out; > > - /* Enforce sane locking for modern driver ioctls. */ > - if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) || > - (flags & DRM_UNLOCKED)) > - retcode = func(dev, kdata, file_priv); > - else { > - mutex_lock(&drm_global_mutex); > - retcode = func(dev, kdata, file_priv); > + retcode = func(dev, kdata, file_priv); > + > +out: > + if (unlikely(drm_dev_needs_global_mutex(dev)) && !(flags & DRM_UNLOCKED)) > mutex_unlock(&drm_global_mutex); > - } > return retcode; > } > EXPORT_SYMBOL(drm_ioctl_kernel); > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

[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

3 years, 9 months

4
11
0 0

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.

3 years, 9 months

2
1
0 0

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

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

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

3 years, 9 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig