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

Deploying new iterator interface for dma-buf

by Christian König

Next round for that one here, maybe the CI systems are now more gracefully with me :) I'm pretty sure that a couple of those dma_resv_for_each_fence_unlocked should actually be replaced with lock+dma_resv_for_each_fence, but that needs more auditing. Please review and comment. Thanks, Christian.

4 years, 3 months

2
27
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi everybody, we recently found that a good bunch of the RCU accesses to the dma_resv object are actually not correctly protected. Those where fixed by either dropping the RCU approach and taking appropriate locks or using a central function to return the current fences as array and then work with that snapshot. This set now tries to prevent adding any new broken code by rolling out two new interfaces to access the fences in a dma_resv object: dma_resv_for_each_fence() - Iterator which should be used while holding the reservation lock. dma_resv_for_each_fence_unlocked() - Iterator based on RCU which can be used without holding the reservation lock and automatic restart on concurrent modification. While doing this we also move the decision which fences to use for write and read accesses into the dma_resv object which results in a quite nice code de-duplication and simplification. The only two remaining users of the RCU shared fence interface are removing shared fences in amdkfd and debugfs code in qxl which will both be addresses in the next patch set. Please review and/or comment, Christian.

4 years, 3 months

2
39
0 0

Re: [Linaro-mm-sig] [PATCH v6 0/2] Add p2p via dmabuf to habanalabs

by Daniel Vetter

On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > Hi, > Re-sending this patch-set following the release of our user-space TPC > compiler and runtime library. > > I would appreciate a review on this. I think the big open we have is the entire revoke discussions. Having the option to let dma-buf hang around which map to random local memory ranges, without clear ownership link and a way to kill it sounds bad to me. I think there's a few options: - We require revoke support. But I've heard rdma really doesn't like that, I guess because taking out an MR while holding the dma_resv_lock would be an inversion, so can't be done. Jason, can you recap what exactly the hold-up was again that makes this a no-go? - The other option I discussed is a bit more the exlusive device ownership model we've had for gpus in drm of the really old kind. Roughly this would work like this, in terms of drm_device: - Only the current owner (drm_master in current drm code, but should probably rename that to drm_owner) is allowed to use the accel driver. So all ioctl would fail if you're not drm_master. - On dropmaster/file close we'd revoke as much as possible, e.g. in-flight commands, mmaps, anything really that can be revoked. - For non-revokable things like these dma-buf we'd keep a drm_master reference around. This would prevent the next open to acquire ownership rights, which at least prevents all the nasty potential problems. - admin (or well container orchestrator) then has responsibility to shoot down all process until the problem goes away (i.e. until you hit the one with the rdma MR which keeps the dma-buf alive) - Not sure there's another reasonable way to do this without inviting some problems once we get outside of the "single kernel instance per tenant" use-case. Wrt implementation there's the trouble of this reinventing a bunch of drm stuff and concepts, but that's maybe for after we've figured out semantics. Also would be great if you have a pull request for the userspace runtime that shows a bit how this all gets used and tied together. Or maybe some pointers, since I guess retconning a PR in github is maybe a bit much. Cheers, Daniel > > Thanks, > Oded > > Oded Gabbay (1): > habanalabs: define uAPI to export FD for DMA-BUF > > Tomer Tayar (1): > habanalabs: add support for dma-buf exporter > > drivers/misc/habanalabs/Kconfig | 1 + > drivers/misc/habanalabs/common/habanalabs.h | 22 + > drivers/misc/habanalabs/common/memory.c | 522 +++++++++++++++++++- > drivers/misc/habanalabs/gaudi/gaudi.c | 1 + > drivers/misc/habanalabs/goya/goya.c | 1 + > include/uapi/misc/habanalabs.h | 28 +- > 6 files changed, 570 insertions(+), 5 deletions(-) > > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 3 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 0/9] dma-fence: Deadline awareness

by Rob Clark

On Thu, Sep 9, 2021 at 9:42 AM Simon Ser <contact(a)emersion.fr> wrote: > > On Thursday, September 9th, 2021 at 18:31, Rob Clark <robdclark(a)gmail.com> wrote: > > > Yes, I think it would.. and "dma-buf/sync_file: Add SET_DEADLINE > > ioctl" adds such an ioctl.. just for the benefit of igt tests at this > > point, but the thought was it would be also used by compositors that > > are doing such frame scheduling. Ofc danvet is a bit grumpy that > > there isn't a more real (than igt) userspace for the ioctl yet ;-) > > Ah, very nice, I somehow missed it. > > I guess one issue is that explicit sync isn't quite plumbed through > compositors yet, so without Jason's DMA-BUF to sync_file IOCTL it'd be > a bit difficult to use. > > Can anybody set the deadline? I wonder if clients should be allowed to. In its current form, anyone who has the fd can.. I'm not sure how (or even if) we could limit it beyond that. I suppose hypothetically you could use this for completely non-compositor related things, like compute jobs where you want the result by some deadline. (OTOH, it could be the driver using this internally when the app is stalling on a result) > What happens if the deadline is exceeded? I'd assume nothing in > particular, the deadline being just a hint? Nothing in particular, it is just a hint. The main intention is to provide a feedback hint to the drivers in scenarios like vblank deadlines, where being 1ms late is just as bad as being $frame_duration-1ms late. From my experiments and profiling it is useful in a couple scenarios: 1) input latency, ie. go from idle to fullscreen animation, where GPU has been idle for a while and not busy enough *yet* for devfreq to kick in and start ramping up the freq before we miss the first vblank 2) double buffering, for ex. if you are 1ms late you end up stalling 15ms before the gpu can start rendering the next frame.. in the absence of feedback, devfreq would ramp down in this scenario instead of up BR, -R

4 years, 3 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 0/9] dma-fence: Deadline awareness

by Rob Clark

On Thu, Sep 9, 2021 at 9:16 AM Simon Ser <contact(a)emersion.fr> wrote: > > Out of curiosity, would it be reasonable to allow user-space (more > precisely, the compositor) to set the deadline via an IOCTL without > actually performing an atomic commit with the FB? > > Some compositors might want to wait themselves for FB fence completions > to ensure a client doesn't block the whole desktop (by submitting a > very costly rendering job). In this case it would make sense for the > compositor to indicate that it intends to display the buffer on next > vblank if it's ready by that point, without queueing a page-flip yet. Yes, I think it would.. and "dma-buf/sync_file: Add SET_DEADLINE ioctl" adds such an ioctl.. just for the benefit of igt tests at this point, but the thought was it would be also used by compositors that are doing such frame scheduling. Ofc danvet is a bit grumpy that there isn't a more real (than igt) userspace for the ioctl yet ;-) BR, -R

4 years, 3 months

1
0
0 0

[PATCH AUTOSEL 5.14 010/252] dma-buf: fix dma_resv_test_signaled test_all handling v2

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 9d38814d1e346ea37a51cbf31f4424c9d059459e ] As the name implies if testing all fences is requested we should indeed test all fences and not skip the exclusive one because we see shared ones. v2: fix logic once more Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210702111642.17259-3-christ… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-resv.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index f26c71747d43..e744fd87c63c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -615,25 +615,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { - unsigned int seq, shared_count; + struct dma_fence *fence; + unsigned int seq; int ret; rcu_read_lock(); retry: ret = true; - shared_count = 0; seq = read_seqcount_begin(&obj->seq); if (test_all) { struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i; - - if (fobj) - shared_count = fobj->shared_count; + unsigned int i, shared_count; + shared_count = fobj ? fobj->shared_count : 0; for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence; - fence = rcu_dereference(fobj->shared[i]); ret = dma_resv_test_signaled_single(fence); if (ret < 0) @@ -641,24 +637,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) else if (!ret) break; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; } - if (!shared_count) { - struct dma_fence *fence_excl = dma_resv_excl_fence(obj); - - if (fence_excl) { - ret = dma_resv_test_signaled_single(fence_excl); - if (ret < 0) - goto retry; + fence = dma_resv_excl_fence(obj); + if (ret && fence) { + ret = dma_resv_test_signaled_single(fence); + if (ret < 0) + goto retry; - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - } } + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; + rcu_read_unlock(); return ret; } -- 2.30.2

4 years, 3 months

1
0
0 0

[PATCH AUTOSEL 5.14 010/252] dma-buf: fix dma_resv_test_signaled test_all handling v2

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 9d38814d1e346ea37a51cbf31f4424c9d059459e ] As the name implies if testing all fences is requested we should indeed test all fences and not skip the exclusive one because we see shared ones. v2: fix logic once more Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210702111642.17259-3-christ… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-resv.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index f26c71747d43..e744fd87c63c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -615,25 +615,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { - unsigned int seq, shared_count; + struct dma_fence *fence; + unsigned int seq; int ret; rcu_read_lock(); retry: ret = true; - shared_count = 0; seq = read_seqcount_begin(&obj->seq); if (test_all) { struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i; - - if (fobj) - shared_count = fobj->shared_count; + unsigned int i, shared_count; + shared_count = fobj ? fobj->shared_count : 0; for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence; - fence = rcu_dereference(fobj->shared[i]); ret = dma_resv_test_signaled_single(fence); if (ret < 0) @@ -641,24 +637,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) else if (!ret) break; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; } - if (!shared_count) { - struct dma_fence *fence_excl = dma_resv_excl_fence(obj); - - if (fence_excl) { - ret = dma_resv_test_signaled_single(fence_excl); - if (ret < 0) - goto retry; + fence = dma_resv_excl_fence(obj); + if (ret && fence) { + ret = dma_resv_test_signaled_single(fence); + if (ret < 0) + goto retry; - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - } } + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; + rcu_read_unlock(); return ret; } -- 2.30.2

4 years, 3 months

1
0
0 0

Harden the dma-fence documentation a bit more

by Christian König

Hi guys, while it is in most cases technically possible to not have a reference to the dma_fence when adding a callback it is usually a good idea to make sure to always have a reference anyway. Otherwise we can indeed see cases where this doesn't really work as intended like for example in the now fixed EPOLL code. Regards, Christian.

4 years, 3 months

2
5
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Add support for mapping buffers with DMA attributes

by Daniel Vetter

On Mon, Aug 30, 2021 at 10:39:11AM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > When mapping the memory represented by a dma-buf into a device's > address space, it might be desireable to map the memory with > certain DMA attributes. Thus, introduce the dma_mapping_attrs > field in the dma_buf_attachment structure so that when > the memory is mapped with dma_buf_map_attachment, it is mapped > with the desired DMA attributes. > > Signed-off-by: Isaac J. Manjarres <isaacm(a)codeaurora.org> > Signed-off-by: Sandeep Patil <sspatil(a)google.com> > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Can you pls include the code that's going to use this here too? At a glance all the attributes you might want to set are supposed to be under the control of the exporter, not the importer. -Daniel > --- > drivers/dma-buf/heaps/cma_heap.c | 6 ++++-- > drivers/dma-buf/heaps/system_heap.c | 6 ++++-- > include/linux/dma-buf.h | 3 +++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index 0c05b79870f9..2c9feb3bfc3e 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -99,9 +99,10 @@ static struct sg_table *cma_heap_map_dma_buf(struct dma_buf_attachment *attachme > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = &a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(-ENOMEM); > a->mapped = true; > @@ -113,9 +114,10 @@ static void cma_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int cma_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..fc7b1e02988e 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -130,9 +130,10 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(ret); > > @@ -145,9 +146,10 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..4d650731766e 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -379,6 +379,8 @@ struct dma_buf_attach_ops { > * @importer_ops: importer operations for this attachment, if provided > * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. > * @importer_priv: importer specific attachment data. > + * @dma_map_attrs: DMA attributes to be used when the exporter maps the buffer > + * through dma_buf_map_attachment. > * > * This structure holds the attachment information between the dma_buf buffer > * and its user device(s). The list contains one attachment struct per device > @@ -399,6 +401,7 @@ struct dma_buf_attachment { > const struct dma_buf_attach_ops *importer_ops; > void *importer_priv; > void *priv; > + unsigned long dma_map_attrs; > }; > > /** > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 3 months

1
0
0 0

[PATCH 1/2] dma-buf: nuke DMA_FENCE_TRACE macros v2

by Christian König

Only the DRM GPU scheduler, radeon and amdgpu where using them and they depend on a non existing config option to actually emit some code. v2: keep the signal path as is for now Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 10 +--------- drivers/gpu/drm/radeon/radeon_fence.c | 24 ++++------------------- drivers/gpu/drm/scheduler/sched_fence.c | 18 ++--------------- include/linux/dma-fence.h | 22 --------------------- 4 files changed, 7 insertions(+), 67 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 0b1c48590c43..c65994e382bd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -246,7 +246,6 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) struct amdgpu_fence_driver *drv = &ring->fence_drv; struct amdgpu_device *adev = ring->adev; uint32_t seq, last_seq; - int r; do { last_seq = atomic_read(&ring->fence_drv.last_seq); @@ -278,12 +277,7 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) if (!fence) continue; - r = dma_fence_signal(fence); - if (!r) - DMA_FENCE_TRACE(fence, "signaled from irq context\n"); - else - BUG(); - + dma_fence_signal(fence); dma_fence_put(fence); pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); @@ -639,8 +633,6 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) if (!timer_pending(&ring->fence_drv.fallback_timer)) amdgpu_fence_schedule_fallback(ring); - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", ring->idx); - return true; } diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 18f2c2e0dfb3..3f351d222cbb 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -176,18 +176,11 @@ static int radeon_fence_check_signaled(wait_queue_entry_t *wait, unsigned mode, */ seq = atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq); if (seq >= fence->seq) { - int ret = dma_fence_signal_locked(&fence->base); - - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->base, "was already signaled\n"); - + dma_fence_signal_locked(&fence->base); radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring); __remove_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); dma_fence_put(&fence->base); - } else - DMA_FENCE_TRACE(&fence->base, "pending\n"); + } return 0; } @@ -422,8 +415,6 @@ static bool radeon_fence_enable_signaling(struct dma_fence *f) fence->fence_wake.func = radeon_fence_check_signaled; __add_wait_queue(&rdev->fence_queue, &fence->fence_wake); dma_fence_get(f); - - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", fence->ring); return true; } @@ -441,11 +432,7 @@ bool radeon_fence_signaled(struct radeon_fence *fence) return true; if (radeon_fence_seq_signaled(fence->rdev, fence->seq, fence->ring)) { - int ret; - - ret = dma_fence_signal(&fence->base); - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from radeon_fence_signaled\n"); + dma_fence_signal(&fence->base); return true; } return false; @@ -550,7 +537,6 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo { uint64_t seq[RADEON_NUM_RINGS] = {}; long r; - int r_sig; /* * This function should not be called on !radeon fences. @@ -567,9 +553,7 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo return r; } - r_sig = dma_fence_signal(&fence->base); - if (!r_sig) - DMA_FENCE_TRACE(&fence->base, "signaled from fence_wait\n"); + dma_fence_signal(&fence->base); return r; } diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..3736746c47bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -50,26 +50,12 @@ static void __exit drm_sched_fence_slab_fini(void) void drm_sched_fence_scheduled(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->scheduled); - - if (!ret) - DMA_FENCE_TRACE(&fence->scheduled, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->scheduled, - "was already signaled\n"); + dma_fence_signal(&fence->scheduled); } void drm_sched_fence_finished(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->finished); - - if (!ret) - DMA_FENCE_TRACE(&fence->finished, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->finished, - "was already signaled\n"); + dma_fence_signal(&fence->finished); } static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence) diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 6ffb4b2c6371..4cc119ab272f 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -590,26 +590,4 @@ struct dma_fence *dma_fence_get_stub(void); struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); -#define DMA_FENCE_TRACE(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - if (IS_ENABLED(CONFIG_DMA_FENCE_TRACE)) \ - pr_info("f %llu#%llu: " fmt, \ - __ff->context, __ff->seqno, ##args); \ - } while (0) - -#define DMA_FENCE_WARN(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_warn("f %llu#%llu: " fmt, __ff->context, __ff->seqno,\ - ##args); \ - } while (0) - -#define DMA_FENCE_ERR(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_err("f %llu#%llu: " fmt, __ff->context, __ff->seqno, \ - ##args); \ - } while (0) - #endif /* __LINUX_DMA_FENCE_H */ -- 2.25.1

4 years, 3 months

2
9
0 0

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

4 years, 3 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

4 years, 3 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); >

4 years, 3 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

4 years, 3 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

4 years, 3 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 */ >

4 years, 3 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

4 years, 3 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

4 years, 4 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.

4 years, 4 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

4 years, 4 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); > }

4 years, 4 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$

4 years, 4 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); >>>

4 years, 4 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); >

4 years, 4 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

4 years, 4 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig