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@xxxxxxxxxxxx> > > > > --- > > > > 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
