[PATCH] dma-buf/sw_sync: Fix timeline/pt overflow cases

[Sean Paul]

On Wed, Jun 28, 2017 at 08:45:55PM +0100, Chris Wilson wrote:
> Quoting Sean Paul (2017-06-28 17:47:24)
> > On Wed, Jun 28, 2017 at 05:00:20PM +0100, Chris Wilson wrote:
> > > Quoting Sean Paul (2017-06-28 16:51:11)
> > > > Protect against long-running processes from overflowing the timeline
> > > > and creating fences that go back in time. While we're at it, avoid
> > > > overflowing while we're incrementing the timeline.
> > > > 
> > > > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > > > ---
> > > >  drivers/dma-buf/sw_sync.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > > > index 69c5ff36e2f9..40934619ed88 100644
> > > > --- a/drivers/dma-buf/sw_sync.c
> > > > +++ b/drivers/dma-buf/sw_sync.c
> > > > @@ -142,7 +142,7 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> > > >  
> > > >         spin_lock_irqsave(&obj->child_list_lock, flags);
> > > >  
> > > > -       obj->value += inc;
> > > > +       obj->value += min(inc, ~0x0U - obj->value);
> > > 
> > > The timeline uses u32 seqno, so just obj->value += min(inc, INT_MAX);
> > > 
> > Hi Chris,
> > Thanks for the review.
> > 
> > I don't think that solves the same problem I was trying to solve. The issue is
> > that android userspace increments value by 0x7fffffff twice in order to ensure
> > all fences have signaled. This is causing value to overflow and is_signaled will
> > never be true. With your snippet, the possibility of overflow still exists.
> > 
> > > Better of course would be to report the error,
> > 
> > AFAIK, it's not an error to jump the timeline, perhaps just bad taste. Capping
> > value at UINT_MAX will ensure all fences are signaled, and the check below ensures
> > that fences can't be created beyond that (returning an error at that point in
> > time).
> 
> UINT_MAX doesn't imply all fences will be signaled either, the timeline
> is supposed to wrap.
> 
> The issue is timeline_fence_signaled() is using the wrong test, it
> should be return (int)(fence->seqno - parent->value) <= 0; If it helps
> extract a little helper from dma_fence_is_later().

Understood, thank you for clarifying. This still doesn't solve the issue of userspace
jumping the timeline by INT_MAX multiple times. In that case, value will rollover and
even the new signaled() will fail to report.

Sean

> -Chris

-- 
Sean Paul, Software Engineer, Google / Chromium OS