On Wed, Jun 02, 2021 at 02:36:58PM +0200, Christian König wrote:
>
>
> 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.
I think for paranoia we could read the seqcount and do a WARN_ON if it
ever increments while we hold dma_resv_lock ourselves. But I think this
one here is the only one where this applies, and it's debugfs only, so
meh?
-Daniel
> 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
> > >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
