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

[Chris Wilson]

Quoting Sean Paul (2017-06-28 22:00:02)
> 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.

Right, all the intermediate fences need to be signaled as the timeline
jumps. That seems to be handled by walking the active_list. However,
that is broken because not all fences are on the active_list and the
active_list itself is not updated atomically!
-Chris