Am 02.06.21 um 14:33 schrieb Daniel Vetter:
On Wed, Jun 02, 2021 at 01:17:08PM +0200, Christian König wrote:
The code tries to acquire the rcu protected fence list, but then ignores
individual fences which have been modified while holding the rcu.
Stop that madness and just note cleanly that the list was concurrently modified.
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
Yeah it's debugfs, it's better not to be fancy here and if you race you
can just re-grab it all.
What's worse, we do grab the dma_resv_lock, which means no one should be
able to race with us. I think 100% right thing here is actually to drop
the rcu_read_lock too, and switch over to rcu_dereference_protected().
And also drop the seqcount check, that would be a bug. seqcount is only
to get a consistent snapshot of all fences on the read (i.e. protected by
rcu only) section. We hold the write lock with dma_resv_lock here.
Yes that what I had in mind as alternative as well.
Just wasn't 100% sure which way to go here.
Going to adjust that,
Christian.
Cheers, Daniel
---
drivers/dma-buf/dma-buf.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index eadd1eaa2fb5..d3b4e370dbc1 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1383,22 +1383,17 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
buf_obj->name ?: "");
robj = buf_obj->resv;
- while (true) {
- seq = read_seqcount_begin(&robj->seq);
- rcu_read_lock();
- fobj = rcu_dereference(robj->fence);
- shared_count = fobj ? fobj->shared_count : 0;
- fence = rcu_dereference(robj->fence_excl);
- if (!read_seqcount_retry(&robj->seq, seq))
- break;
- rcu_read_unlock();
- }
-
+ seq = read_seqcount_begin(&robj->seq);
+ rcu_read_lock();
+ fence = rcu_dereference(robj->fence_excl);
if (fence)
seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n",
fence->ops->get_driver_name(fence),
fence->ops->get_timeline_name(fence),
dma_fence_is_signaled(fence) ? "" : "un");
+
+ fobj = rcu_dereference(robj->fence);
+ shared_count = fobj ? fobj->shared_count : 0;
for (i = 0; i < shared_count; i++) {
fence = rcu_dereference(fobj->shared[i]);
if (!dma_fence_get_rcu(fence))
@@ -1410,6 +1405,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
dma_fence_put(fence);
}
rcu_read_unlock();
+ if (read_seqcount_retry(&robj->seq, seq))
+ seq_printf(s, "\tFences concurrently modified\n");
seq_puts(s, "\tAttached Devices:\n");
attach_count = 0;
--
2.25.1
