<div dir="ltr">I think the kernel has problems with Android fences which were slowly broken as they were de-staged:<div><br></div><div>1. They allowed for fence/timeline value/seqno to overflow/rollover and that would break only if delta between timeline and earlier unsignaled fence exceeded 2^31 or so. Android relies on that behavior.<br></div><div><div>It was also possible to clean-up timeline by incrementing it for instance by calling timeline_inc(0x7FFFFFFF) twice. This doesn't work anymore because if no one is actively waiting for fence, active_list is empty and timeline_inc does not signal fences anymore; just increments timeline value and allows it to rollover without any effect.</div><div><br></div></div><div>2. They did guarantee that when timeline was destroyed all fences on timeline were signaled (or error state). Thus, if timeline was destroyed, or if process that owned the timeline died, processes that depended on it would not hang. Android also relies on that behavior for cleanup when a process crashes for example.</div><div><br><div>3. It seems from some stack traces that I have seen that timeline_inc signals fence from inside spinlock, which can cause fence_array to call fence_array_release, which calls flush_work() <br></div></div><div>e.g.</div><div><div><4>[  140.762030]  [<ffffffff858939fb>] dump_stack+0x4d/0x63</div><div><4>[  140.762035]  [<ffffffff8568b593>] ___might_sleep+0x149/0x14e</div><div><4>[  140.762039]  [<ffffffff8568b637>] __might_sleep+0x9f/0xa6</div><div><4>[  140.762045]  [<ffffffff8567ef2a>] flush_work+0x39/0x19a</div><div><4>[  140.762049]  [<ffffffff8568eb5c>] ? try_to_wake_up+0x20b/0x21b</div><div><4>[  140.762055]  [<ffffffff85a54cea>] fence_array_release+0x2e/0x63</div><div><4>[  140.762058]  [<ffffffff85a53a65>] fence_release+0x82/0x8e</div><div><4>[  140.762061]  [<ffffffff85a54cba>] fence_put+0x15/0x17</div><div><4>[  140.762065]  [<ffffffff85a54e08>] fence_array_cb_func+0x1f/0x39</div><div><4>[  140.762068]  [<ffffffff85a53881>] fence_signal_locked+0x8e/0xa3</div><div><4>[  140.762072]  [<ffffffff85a55cda>] sync_timeline_signal+0xcd/0x10a</div><div><4>[  140.762075]  [<ffffffff85a5613b>] sw_sync_ioctl+0x159/0x17f</div></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 28, 2017 at 2:00 PM, Sean Paul <span dir="ltr"><<a href="mailto:seanpaul@chromium.org" target="_blank">seanpaul@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jun 28, 2017 at 08:45:55PM +0100, Chris Wilson wrote:<br>
> Quoting Sean Paul (2017-06-28 17:47:24)<br>
> > On Wed, Jun 28, 2017 at 05:00:20PM +0100, Chris Wilson wrote:<br>
> > > Quoting Sean Paul (2017-06-28 16:51:11)<br>
> > > > Protect against long-running processes from overflowing the timeline<br>
> > > > and creating fences that go back in time. While we're at it, avoid<br>
> > > > overflowing while we're incrementing the timeline.<br>
> > > ><br>
> > > > Signed-off-by: Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>><br>
> > > > ---<br>
> > > >  drivers/dma-buf/sw_sync.c | 7 ++++++-<br>
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)<br>
> > > ><br>
> > > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c<br>
> > > > index 69c5ff36e2f9..40934619ed88 100644<br>
> > > > --- a/drivers/dma-buf/sw_sync.c<br>
> > > > +++ b/drivers/dma-buf/sw_sync.c<br>
> > > > @@ -142,7 +142,7 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)<br>
> > > ><br>
> > > >         spin_lock_irqsave(&obj->child_<wbr>list_lock, flags);<br>
> > > ><br>
> > > > -       obj->value += inc;<br>
> > > > +       obj->value += min(inc, ~0x0U - obj->value);<br>
> > ><br>
> > > The timeline uses u32 seqno, so just obj->value += min(inc, INT_MAX);<br>
> > ><br>
> > Hi Chris,<br>
> > Thanks for the review.<br>
> ><br>
> > I don't think that solves the same problem I was trying to solve. The issue is<br>
> > that android userspace increments value by 0x7fffffff twice in order to ensure<br>
> > all fences have signaled. This is causing value to overflow and is_signaled will<br>
> > never be true. With your snippet, the possibility of overflow still exists.<br>
> ><br>
> > > Better of course would be to report the error,<br>
> ><br>
> > AFAIK, it's not an error to jump the timeline, perhaps just bad taste. Capping<br>
> > value at UINT_MAX will ensure all fences are signaled, and the check below ensures<br>
> > that fences can't be created beyond that (returning an error at that point in<br>
> > time).<br>
><br>
> UINT_MAX doesn't imply all fences will be signaled either, the timeline<br>
> is supposed to wrap.<br>
><br>
> The issue is timeline_fence_signaled() is using the wrong test, it<br>
> should be return (int)(fence->seqno - parent->value) <= 0; If it helps<br>
> extract a little helper from dma_fence_is_later().<br>
<br>
</div></div>Understood, thank you for clarifying. This still doesn't solve the issue of userspace<br>
jumping the timeline by INT_MAX multiple times. In that case, value will rollover and<br>
even the new signaled() will fail to report.<br>
<span class="HOEnZb"><font color="#888888"><br>
Sean<br>
</font></span><span class="im HOEnZb"><br>
> -Chris<br>
<br>
--<br>
Sean Paul, Software Engineer, Google / Chromium OS<br>
</span><div class="HOEnZb"><div class="h5">______________________________<wbr>_________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.<wbr>org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br></div>