Re: [PATCH] dma-buf/resv: fix exclusive fence get

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 30, 2019 at 08:57:32AM +0000, Koenig, Christian wrote:
> Am 30.09.19 um 09:22 schrieb Daniel Vetter:
> > On Sun, Sep 22, 2019 at 2:08 PM Qiang Yu <yuq825@gmail.com> wrote:
> >> This causes kernel crash when testing lima driver.
> >>
> >> Cc: Christian König <christian.koenig@amd.com>
> >> Fixes: b8c036dfc66f ("dma-buf: simplify reservation_object_get_fences_rcu a bit")
> >> Signed-off-by: Qiang Yu <yuq825@gmail.com>
> > Selftest for this would be lovely, now that the basic infrastructure
> > is in place ...
> 
> What do you have in mind? I wouldn't even know where to start to write 
> an unit test for this.

1. set a few fences (both excl + shared) in a dma_resv
2. get them
3. check that we got them all
4. notice that the exlusive fence isn't actually in the array (because we
increment the index before storing, so the exclusive fence ended past the
array). For robustness the test should check that the fences are listed in
any order, not just the one the current implementation gives you.

I guess the actual crash happens when we're unlucky and overflow the
allocation, which is probably more rare. But KASAN should help catch that
too (run that in your CI if you don't do that yet, it's pretty
impressive).

Or am I totally misunderstanding what's going wrong here?
-Daniel
> 
> Christian.
> 
> > -Daniel
> >
> >> ---
> >>   drivers/dma-buf/dma-resv.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> >> index 42a8f3f11681..709002515550 100644
> >> --- a/drivers/dma-buf/dma-resv.c
> >> +++ b/drivers/dma-buf/dma-resv.c
> >> @@ -471,7 +471,7 @@ int dma_resv_get_fences_rcu(struct dma_resv *obj,
> >>          if (pfence_excl)
> >>                  *pfence_excl = fence_excl;
> >>          else if (fence_excl)
> >> -               shared[++shared_count] = fence_excl;
> >> +               shared[shared_count++] = fence_excl;
> >>
> >>          if (!shared_count) {
> >>                  kfree(shared);
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> 

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