| Message ID | 149866562059.23475.15965626912972737879@mail.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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). Sean > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 69c5ff36e2f9..2503cf884018 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -345,6 +345,9 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, unsigned long arg) > if (copy_from_user(&value, (void __user *)arg, sizeof(value))) > return -EFAULT; > > + if (value > INT_MAX) > + return -EINVAL; > + > sync_timeline_signal(obj, value); > > > > > list_for_each_entry_safe(pt, next, &obj->active_list_head, > > active_list) { > > @@ -178,6 +178,11 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size, > > return NULL; > > > > spin_lock_irqsave(&obj->child_list_lock, flags); > > + if (value < obj->value) { > > + spin_unlock_irqrestore(&obj->child_list_lock, flags); > > + return NULL; > > + } > > Needs a u32 check. if ((int)(value - obj->value) < 0) return some_error; > -Chris
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@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
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 69c5ff36e2f9..2503cf884018 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -345,6 +345,9 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, unsigned long arg) if (copy_from_user(&value, (void __user *)arg, sizeof(value))) return -EFAULT; + if (value > INT_MAX) + return -EINVAL; + sync_timeline_signal(obj, value); >