dma-fence: fix dma_fence_get_rcu_safe

Commit Message

Christian König Sept. 4, 2017, 1:27 p.m. UTC

From: Christian König <[e-mail]>

The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
acquire a reference it doesn't necessary mean that there is no fence at all.

It usually mean that the fence was replaced by a new one and in this situation
we certainly want to have the new one as result and *NOT* NULL.

Signed-off-by: Christian König <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 include/linux/dma-fence.h | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

Comments

Chris Wilson Sept. 4, 2017, 1:40 p.m. UTC | #1

Quoting Christian König (2017-09-04 14:27:33)
> From: Christian König <[e-mail]>
> 
> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
> acquire a reference it doesn't necessary mean that there is no fence at all.
> 
> It usually mean that the fence was replaced by a new one and in this situation
> we certainly want to have the new one as result and *NOT* NULL.

Which is not guaranteed by the code you wrote either.

The point of the comment is that the mb is only inside the successful
kref_atomic_inc_unless_zero, and that only after that mb do you know
whether or not you have the current fence.

You can argue that you want to replace the
	if (!dma_fence_get_rcu())
		return NULL
with
	if (!dma_fence_get_rcu()
		continue;
but it would be incorrect to say that by simply ignoring the
post-condition check that you do have the right fence.
-Chris

Christian König Sept. 11, 2017, 8:50 a.m. UTC | #2

Sorry for the delayed response, but your mail somehow ended up in the 
Spam folder.

Am 04.09.2017 um 15:40 schrieb Chris Wilson:
> Quoting Christian König (2017-09-04 14:27:33)
>> From: Christian König <[e-mail]>
>>
>> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
>> acquire a reference it doesn't necessary mean that there is no fence at all.
>>
>> It usually mean that the fence was replaced by a new one and in this situation
>> we certainly want to have the new one as result and *NOT* NULL.
> Which is not guaranteed by the code you wrote either.
>
> The point of the comment is that the mb is only inside the successful
> kref_atomic_inc_unless_zero, and that only after that mb do you know
> whether or not you have the current fence.
>
> You can argue that you want to replace the
> 	if (!dma_fence_get_rcu())
> 		return NULL
> with
> 	if (!dma_fence_get_rcu()
> 		continue;
> but it would be incorrect to say that by simply ignoring the
> post-condition check that you do have the right fence.

You are completely missing the point here.

It is irrelevant if you have the current fence or not when you return. 
You can only guarantee that it is the current fence when you take a look 
and that is exactly what we want to avoid.

So the existing code is complete nonsense. Instead what we need to 
guarantee is that we return *ANY* fence which we can grab a reference for.

See the usual life of a fence* variable looks like this:
1. assigning pointer to fence A;
2. assigning pointer to fence B;
3. assigning pointer to fence C;
....

When dma_fence_get_rcu_safe() is called between step #1 and step #2 for 
example it is perfectly valid to just return either fence A or fence B.

But it is invalid to return NULL because that suggests that we don't 
need to sync at all.

Regards,
Christian.

Chris Wilson Sept. 11, 2017, 8:59 a.m. UTC | #3

Quoting Christian König (2017-09-11 09:50:40)
> Sorry for the delayed response, but your mail somehow ended up in the 
> Spam folder.
> 
> Am 04.09.2017 um 15:40 schrieb Chris Wilson:
> > Quoting Christian König (2017-09-04 14:27:33)
> >> From: Christian König <[e-mail]>
> >>
> >> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
> >> acquire a reference it doesn't necessary mean that there is no fence at all.
> >>
> >> It usually mean that the fence was replaced by a new one and in this situation
> >> we certainly want to have the new one as result and *NOT* NULL.
> > Which is not guaranteed by the code you wrote either.
> >
> > The point of the comment is that the mb is only inside the successful
> > kref_atomic_inc_unless_zero, and that only after that mb do you know
> > whether or not you have the current fence.
> >
> > You can argue that you want to replace the
> >       if (!dma_fence_get_rcu())
> >               return NULL
> > with
> >       if (!dma_fence_get_rcu()
> >               continue;
> > but it would be incorrect to say that by simply ignoring the
> > post-condition check that you do have the right fence.
> 
> You are completely missing the point here.
> 
> It is irrelevant if you have the current fence or not when you return. 
> You can only guarantee that it is the current fence when you take a look 
> and that is exactly what we want to avoid.
> 
> So the existing code is complete nonsense. Instead what we need to 
> guarantee is that we return *ANY* fence which we can grab a reference for.

Not quite. We can grab a reference on a fence that was already freed and
reused between the rcu_dereference() and dma_fence_get_rcu().
-Chris

Christian König Sept. 11, 2017, 9:06 a.m. UTC | #4

Am 11.09.2017 um 10:59 schrieb Chris Wilson:
> Quoting Christian König (2017-09-11 09:50:40)
>> Sorry for the delayed response, but your mail somehow ended up in the
>> Spam folder.
>>
>> Am 04.09.2017 um 15:40 schrieb Chris Wilson:
>>> Quoting Christian König (2017-09-04 14:27:33)
>>>> From: Christian König <[e-mail]>
>>>>
>>>> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
>>>> acquire a reference it doesn't necessary mean that there is no fence at all.
>>>>
>>>> It usually mean that the fence was replaced by a new one and in this situation
>>>> we certainly want to have the new one as result and *NOT* NULL.
>>> Which is not guaranteed by the code you wrote either.
>>>
>>> The point of the comment is that the mb is only inside the successful
>>> kref_atomic_inc_unless_zero, and that only after that mb do you know
>>> whether or not you have the current fence.
>>>
>>> You can argue that you want to replace the
>>>        if (!dma_fence_get_rcu())
>>>                return NULL
>>> with
>>>        if (!dma_fence_get_rcu()
>>>                continue;
>>> but it would be incorrect to say that by simply ignoring the
>>> post-condition check that you do have the right fence.
>> You are completely missing the point here.
>>
>> It is irrelevant if you have the current fence or not when you return.
>> You can only guarantee that it is the current fence when you take a look
>> and that is exactly what we want to avoid.
>>
>> So the existing code is complete nonsense. Instead what we need to
>> guarantee is that we return *ANY* fence which we can grab a reference for.
> Not quite. We can grab a reference on a fence that was already freed and
> reused between the rcu_dereference() and dma_fence_get_rcu().

Reusing a memory structure before the RCU grace period is completed is 
illegal, otherwise the whole RCU approach won't work.

So that case can't happen.

Regards,
Christian.

> -Chris

Chris Wilson Sept. 11, 2017, 9:23 a.m. UTC | #5

Quoting Christian König (2017-09-11 10:06:50)
> Am 11.09.2017 um 10:59 schrieb Chris Wilson:
> > Quoting Christian König (2017-09-11 09:50:40)
> >> Sorry for the delayed response, but your mail somehow ended up in the
> >> Spam folder.
> >>
> >> Am 04.09.2017 um 15:40 schrieb Chris Wilson:
> >>> Quoting Christian König (2017-09-04 14:27:33)
> >>>> From: Christian König <[e-mail]>
> >>>>
> >>>> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
> >>>> acquire a reference it doesn't necessary mean that there is no fence at all.
> >>>>
> >>>> It usually mean that the fence was replaced by a new one and in this situation
> >>>> we certainly want to have the new one as result and *NOT* NULL.
> >>> Which is not guaranteed by the code you wrote either.
> >>>
> >>> The point of the comment is that the mb is only inside the successful
> >>> kref_atomic_inc_unless_zero, and that only after that mb do you know
> >>> whether or not you have the current fence.
> >>>
> >>> You can argue that you want to replace the
> >>>        if (!dma_fence_get_rcu())
> >>>                return NULL
> >>> with
> >>>        if (!dma_fence_get_rcu()
> >>>                continue;
> >>> but it would be incorrect to say that by simply ignoring the
> >>> post-condition check that you do have the right fence.
> >> You are completely missing the point here.
> >>
> >> It is irrelevant if you have the current fence or not when you return.
> >> You can only guarantee that it is the current fence when you take a look
> >> and that is exactly what we want to avoid.
> >>
> >> So the existing code is complete nonsense. Instead what we need to
> >> guarantee is that we return *ANY* fence which we can grab a reference for.
> > Not quite. We can grab a reference on a fence that was already freed and
> > reused between the rcu_dereference() and dma_fence_get_rcu().
> 
> Reusing a memory structure before the RCU grace period is completed is 
> illegal, otherwise the whole RCU approach won't work.

RCU only protects that the pointer remains valid. If you use
SLAB_TYPESAFE_BY_RCU, it is possible to reuse the pointer within a grace
period. It does happen and that is the point the comment is trying to
make.
-Chris

Christian König Sept. 11, 2017, 9:57 a.m. UTC | #6

Am 11.09.2017 um 11:23 schrieb Chris Wilson:
> Quoting Christian König (2017-09-11 10:06:50)
>> Am 11.09.2017 um 10:59 schrieb Chris Wilson:
>>> Quoting Christian König (2017-09-11 09:50:40)
>>>> Sorry for the delayed response, but your mail somehow ended up in the
>>>> Spam folder.
>>>>
>>>> Am 04.09.2017 um 15:40 schrieb Chris Wilson:
>>>>> Quoting Christian König (2017-09-04 14:27:33)
>>>>>> From: Christian König <[e-mail]>
>>>>>>
>>>>>> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
>>>>>> acquire a reference it doesn't necessary mean that there is no fence at all.
>>>>>>
>>>>>> It usually mean that the fence was replaced by a new one and in this situation
>>>>>> we certainly want to have the new one as result and *NOT* NULL.
>>>>> Which is not guaranteed by the code you wrote either.
>>>>>
>>>>> The point of the comment is that the mb is only inside the successful
>>>>> kref_atomic_inc_unless_zero, and that only after that mb do you know
>>>>> whether or not you have the current fence.
>>>>>
>>>>> You can argue that you want to replace the
>>>>>         if (!dma_fence_get_rcu())
>>>>>                 return NULL
>>>>> with
>>>>>         if (!dma_fence_get_rcu()
>>>>>                 continue;
>>>>> but it would be incorrect to say that by simply ignoring the
>>>>> post-condition check that you do have the right fence.
>>>> You are completely missing the point here.
>>>>
>>>> It is irrelevant if you have the current fence or not when you return.
>>>> You can only guarantee that it is the current fence when you take a look
>>>> and that is exactly what we want to avoid.
>>>>
>>>> So the existing code is complete nonsense. Instead what we need to
>>>> guarantee is that we return *ANY* fence which we can grab a reference for.
>>> Not quite. We can grab a reference on a fence that was already freed and
>>> reused between the rcu_dereference() and dma_fence_get_rcu().
>> Reusing a memory structure before the RCU grace period is completed is
>> illegal, otherwise the whole RCU approach won't work.
> RCU only protects that the pointer remains valid. If you use
> SLAB_TYPESAFE_BY_RCU, it is possible to reuse the pointer within a grace
> period. It does happen and that is the point the comment is trying to
> make.

Yeah, but that is illegal with a fence objects.

When anybody allocates fences this way it breaks at least 
reservation_object_get_fences_rcu(), 
reservation_object_wait_timeout_rcu() and 
reservation_object_test_signaled_single().

Cause all of them rely on dma_fence_get() to return NULL when the fence 
isn't valid any more to restart the operation.

When dma_fence_get_rcu() returns a reallocated fence the operation 
wouldn't correctly restart and the end result most likely not be correct 
at all.

Using SLAB_TYPESAFE_BY_RCU is only valid if you can ensure that you have 
the right object using a second criteria and that is not the case with 
fences.

Regards,
Christian.

Chris Wilson Sept. 11, 2017, 10:01 a.m. UTC | #7

Quoting Christian König (2017-09-11 10:57:57)
> Am 11.09.2017 um 11:23 schrieb Chris Wilson:
> > Quoting Christian König (2017-09-11 10:06:50)
> >> Am 11.09.2017 um 10:59 schrieb Chris Wilson:
> >>> Quoting Christian König (2017-09-11 09:50:40)
> >>>> Sorry for the delayed response, but your mail somehow ended up in the
> >>>> Spam folder.
> >>>>
> >>>> Am 04.09.2017 um 15:40 schrieb Chris Wilson:
> >>>>> Quoting Christian König (2017-09-04 14:27:33)
> >>>>>> From: Christian König <[e-mail]>
> >>>>>>
> >>>>>> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
> >>>>>> acquire a reference it doesn't necessary mean that there is no fence at all.
> >>>>>>
> >>>>>> It usually mean that the fence was replaced by a new one and in this situation
> >>>>>> we certainly want to have the new one as result and *NOT* NULL.
> >>>>> Which is not guaranteed by the code you wrote either.
> >>>>>
> >>>>> The point of the comment is that the mb is only inside the successful
> >>>>> kref_atomic_inc_unless_zero, and that only after that mb do you know
> >>>>> whether or not you have the current fence.
> >>>>>
> >>>>> You can argue that you want to replace the
> >>>>>         if (!dma_fence_get_rcu())
> >>>>>                 return NULL
> >>>>> with
> >>>>>         if (!dma_fence_get_rcu()
> >>>>>                 continue;
> >>>>> but it would be incorrect to say that by simply ignoring the
> >>>>> post-condition check that you do have the right fence.
> >>>> You are completely missing the point here.
> >>>>
> >>>> It is irrelevant if you have the current fence or not when you return.
> >>>> You can only guarantee that it is the current fence when you take a look
> >>>> and that is exactly what we want to avoid.
> >>>>
> >>>> So the existing code is complete nonsense. Instead what we need to
> >>>> guarantee is that we return *ANY* fence which we can grab a reference for.
> >>> Not quite. We can grab a reference on a fence that was already freed and
> >>> reused between the rcu_dereference() and dma_fence_get_rcu().
> >> Reusing a memory structure before the RCU grace period is completed is
> >> illegal, otherwise the whole RCU approach won't work.
> > RCU only protects that the pointer remains valid. If you use
> > SLAB_TYPESAFE_BY_RCU, it is possible to reuse the pointer within a grace
> > period. It does happen and that is the point the comment is trying to
> > make.
> 
> Yeah, but that is illegal with a fence objects.
> 
> When anybody allocates fences this way it breaks at least 
> reservation_object_get_fences_rcu(), 
> reservation_object_wait_timeout_rcu() and 
> reservation_object_test_signaled_single().

Many, many months ago I sent patches to fix them all.
-Chris

Christian König Sept. 11, 2017, 11:06 a.m. UTC | #8

Am 11.09.2017 um 12:01 schrieb Chris Wilson:
> [SNIP]
>> Yeah, but that is illegal with a fence objects.
>>
>> When anybody allocates fences this way it breaks at least
>> reservation_object_get_fences_rcu(),
>> reservation_object_wait_timeout_rcu() and
>> reservation_object_test_signaled_single().
> Many, many months ago I sent patches to fix them all.

Found those after a bit a searching. Yeah, those patches where proposed 
more than a year ago, but never pushed upstream.

Not sure if we really should go this way. dma_fence objects are shared 
between drivers and since we can't judge if it's the correct fence based 
on a criteria in the object (only the read counter which is outside) all 
drivers need to be correct for this.

I would rather go the way and change dma_fence_release() to wrap 
fence->ops->release into call_rcu() to keep the whole RCU handling 
outside of the individual drivers.

Regards,
Christian.

Daniel Vetter Sept. 20, 2017, 6:20 p.m. UTC | #9

On Mon, Sep 11, 2017 at 01:06:32PM +0200, Christian König wrote:
> Am 11.09.2017 um 12:01 schrieb Chris Wilson:
> > [SNIP]
> > > Yeah, but that is illegal with a fence objects.
> > >
> > > When anybody allocates fences this way it breaks at least
> > > reservation_object_get_fences_rcu(),
> > > reservation_object_wait_timeout_rcu() and
> > > reservation_object_test_signaled_single().
> > Many, many months ago I sent patches to fix them all.
>
> Found those after a bit a searching. Yeah, those patches where proposed more
> than a year ago, but never pushed upstream.
>
> Not sure if we really should go this way. dma_fence objects are shared
> between drivers and since we can't judge if it's the correct fence based on
> a criteria in the object (only the read counter which is outside) all
> drivers need to be correct for this.
>
> I would rather go the way and change dma_fence_release() to wrap
> fence->ops->release into call_rcu() to keep the whole RCU handling outside
> of the individual drivers.

Hm, I entirely dropped the ball on this, I kinda assumed that we managed
to get some agreement on this between i915 and dma_fence. Adding a pile
more people.

Joonas, Tvrtko, I guess we need to fix this one way or the other.
-Daniel

Christian König Sept. 21, 2017, 7 a.m. UTC | #10

Am 20.09.2017 um 20:20 schrieb Daniel Vetter:
> On Mon, Sep 11, 2017 at 01:06:32PM +0200, Christian König wrote:
>> Am 11.09.2017 um 12:01 schrieb Chris Wilson:
>>> [SNIP]
>>>> Yeah, but that is illegal with a fence objects.
>>>>
>>>> When anybody allocates fences this way it breaks at least
>>>> reservation_object_get_fences_rcu(),
>>>> reservation_object_wait_timeout_rcu() and
>>>> reservation_object_test_signaled_single().
>>> Many, many months ago I sent patches to fix them all.
>> Found those after a bit a searching. Yeah, those patches where proposed more
>> than a year ago, but never pushed upstream.
>>
>> Not sure if we really should go this way. dma_fence objects are shared
>> between drivers and since we can't judge if it's the correct fence based on
>> a criteria in the object (only the read counter which is outside) all
>> drivers need to be correct for this.
>>
>> I would rather go the way and change dma_fence_release() to wrap
>> fence->ops->release into call_rcu() to keep the whole RCU handling outside
>> of the individual drivers.
> Hm, I entirely dropped the ball on this, I kinda assumed that we managed
> to get some agreement on this between i915 and dma_fence. Adding a pile
> more people.

For the meantime I've send a v2 of this patch to fix at least the buggy 
return of NULL when we fail to grab the RCU reference but keeping the 
extra checking for now.

Can I get an rb on this please so that we fix at least the bug at hand?

Thanks,
Christian.

>
> Joonas, Tvrtko, I guess we need to fix this one way or the other.
> -Daniel

Maarten Lankhorst Sept. 21, 2017, 7:29 a.m. UTC | #11

Op 21-09-17 om 09:00 schreef Christian König:
> Am 20.09.2017 um 20:20 schrieb Daniel Vetter:
>> On Mon, Sep 11, 2017 at 01:06:32PM +0200, Christian König wrote:
>>> Am 11.09.2017 um 12:01 schrieb Chris Wilson:
>>>> [SNIP]
>>>>> Yeah, but that is illegal with a fence objects.
>>>>>
>>>>> When anybody allocates fences this way it breaks at least
>>>>> reservation_object_get_fences_rcu(),
>>>>> reservation_object_wait_timeout_rcu() and
>>>>> reservation_object_test_signaled_single().
>>>> Many, many months ago I sent patches to fix them all.
>>> Found those after a bit a searching. Yeah, those patches where proposed more
>>> than a year ago, but never pushed upstream.
>>>
>>> Not sure if we really should go this way. dma_fence objects are shared
>>> between drivers and since we can't judge if it's the correct fence based on
>>> a criteria in the object (only the read counter which is outside) all
>>> drivers need to be correct for this.
>>>
>>> I would rather go the way and change dma_fence_release() to wrap
>>> fence->ops->release into call_rcu() to keep the whole RCU handling outside
>>> of the individual drivers.
>> Hm, I entirely dropped the ball on this, I kinda assumed that we managed
>> to get some agreement on this between i915 and dma_fence. Adding a pile
>> more people.
>
> For the meantime I've send a v2 of this patch to fix at least the buggy return of NULL when we fail to grab the RCU reference but keeping the extra checking for now.
>
> Can I get an rb on this please so that we fix at least the bug at hand?
>
> Thanks,
> Christian. 
Done.

Joonas Lahtinen Sept. 29, 2017, 12:34 p.m. UTC | #12

On Wed, 2017-09-20 at 20:20 +0200, Daniel Vetter wrote:
> On Mon, Sep 11, 2017 at 01:06:32PM +0200, Christian König wrote:
> > Am 11.09.2017 um 12:01 schrieb Chris Wilson:
> > > [SNIP]
> > > > Yeah, but that is illegal with a fence objects.
> > > > 
> > > > When anybody allocates fences this way it breaks at least
> > > > reservation_object_get_fences_rcu(),
> > > > reservation_object_wait_timeout_rcu() and
> > > > reservation_object_test_signaled_single().
> > > 
> > > Many, many months ago I sent patches to fix them all.
> > 
> > Found those after a bit a searching. Yeah, those patches where proposed more
> > than a year ago, but never pushed upstream.
> > 
> > Not sure if we really should go this way. dma_fence objects are shared
> > between drivers and since we can't judge if it's the correct fence based on
> > a criteria in the object (only the read counter which is outside) all
> > drivers need to be correct for this.
> > 
> > I would rather go the way and change dma_fence_release() to wrap
> > fence->ops->release into call_rcu() to keep the whole RCU handling outside
> > of the individual drivers.
> 
> Hm, I entirely dropped the ball on this, I kinda assumed that we managed
> to get some agreement on this between i915 and dma_fence. Adding a pile
> more people.
> 
> Joonas, Tvrtko, I guess we need to fix this one way or the other.

I definitely didn't get the memo or notice this before. Tvrtko/Chris?

Regars, Joonas

Patch

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7..37f3d67 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -246,27 +246,8 @@  dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep)
 		struct dma_fence *fence;
 
 		fence = rcu_dereference(*fencep);
-		if (!fence || !dma_fence_get_rcu(fence))
-			return NULL;
-
-		/* The atomic_inc_not_zero() inside dma_fence_get_rcu()
-		 * provides a full memory barrier upon success (such as now).
-		 * This is paired with the write barrier from assigning
-		 * to the __rcu protected fence pointer so that if that
-		 * pointer still matches the current fence, we know we
-		 * have successfully acquire a reference to it. If it no
-		 * longer matches, we are holding a reference to some other
-		 * reallocated pointer. This is possible if the allocator
-		 * is using a freelist like SLAB_TYPESAFE_BY_RCU where the
-		 * fence remains valid for the RCU grace period, but it
-		 * may be reallocated. When using such allocators, we are
-		 * responsible for ensuring the reference we get is to
-		 * the right fence, as below.
-		 */
-		if (fence == rcu_access_pointer(*fencep))
-			return rcu_pointer_handoff(fence);
-
-		dma_fence_put(fence);
+		if (!fence || dma_fence_get_rcu(fence))
+			return fence;
 	} while (1);
 }