Re: [PATCH 13/24] dma-buf: drop the DAG approach for the dma_resv object

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 04, 2022 at 04:08:20PM +0100, Christian König wrote:
> Am 22.12.21 um 22:43 schrieb Daniel Vetter:
> > On Tue, Dec 07, 2021 at 01:34:00PM +0100, Christian König wrote:
> > > So far we had the approach of using a directed acyclic
> > > graph with the dma_resv obj.
> > > 
> > > This turned out to have many downsides, especially it means
> > > that every single driver and user of this interface needs
> > > to be aware of this restriction when adding fences. If the
> > > rules for the DAG are not followed then we end up with
> > > potential hard to debug memory corruption, information
> > > leaks or even elephant big security holes because we allow
> > > userspace to access freed up memory.
> > > 
> > > Since we already took a step back from that by always
> > > looking at all fences we now go a step further and stop
> > > dropping the shared fences when a new exclusive one is
> > > added.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/dma-buf/dma-resv.c | 13 -------------
> > >   1 file changed, 13 deletions(-)
> > > 
> > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> > > index 9acceabc9399..ecb2ff606bac 100644
> > > --- a/drivers/dma-buf/dma-resv.c
> > > +++ b/drivers/dma-buf/dma-resv.c
> > No doc update at all!
> 
> Scratching my head I'm pretty sure I've updated at least the kerneldoc for
> dma_resv_add_excl_fence(). Must have gone lost in some rebase.
> 
> > 
> > I checked, we're not that shitty with docs,
> 
> Well I wouldn't say shitty, but they are not perfect either.

This was sarcasm, I meant to say that despite the struggles the docs
in-tree are pretty good nowadays. Email just sucks sometimes for
communication.

> >   Minimally the DOC: section
> > header and also the struct dma_resv kerneldoc. Also there's maybe more
> > references and stuff I've missed on a quick look, please check for them
> > (e.g. dma_buf.resv kerneldoc is rather important to keep correct too).
> > 
> > Code itself does what it says in the commit message, but we really need
> > the most accurate docs we can get for this stuff, or the confusion will
> > persist :-/
> 
> Yeah completely agree, going to fix that.

Awesome!

Cheers, Daniel

> 
> Thanks,
> Christian.
> 
> > 
> > Cheers, Daniel
> > 
> > > @@ -383,29 +383,16 @@ EXPORT_SYMBOL(dma_resv_replace_fences);
> > >   void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence)
> > >   {
> > >   	struct dma_fence *old_fence = dma_resv_excl_fence(obj);
> > > -	struct dma_resv_list *old;
> > > -	u32 i = 0;
> > >   	dma_resv_assert_held(obj);
> > > -	old = dma_resv_shared_list(obj);
> > > -	if (old)
> > > -		i = old->shared_count;
> > > -
> > >   	dma_fence_get(fence);
> > >   	write_seqcount_begin(&obj->seq);
> > >   	/* write_seqcount_begin provides the necessary memory barrier */
> > >   	RCU_INIT_POINTER(obj->fence_excl, fence);
> > > -	if (old)
> > > -		old->shared_count = 0;
> > >   	write_seqcount_end(&obj->seq);
> > > -	/* inplace update, no shared fences */
> > > -	while (i--)
> > > -		dma_fence_put(rcu_dereference_protected(old->shared[i],
> > > -						dma_resv_held(obj)));
> > > -
> > >   	dma_fence_put(old_fence);
> > >   }
> > >   EXPORT_SYMBOL(dma_resv_add_excl_fence);
> > > -- 
> > > 2.25.1
> > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch