[PATCH] drm/amdgpu:fix race condition bug

[PATCH] drm/amdgpu:fix race condition bug

[Monk Liu]

Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
Signed-off-by: Monk Liu <Monk.Liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 6d86eae..b8c11fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
 
 	spin_lock(&ctx->ring_lock);
 
+	if (!seq)
+		seq = ctx->rings[ring->idx].sequence - 1;
+
 	if (seq >= cring->sequence) {
 		spin_unlock(&ctx->ring_lock);
 		return ERR_PTR(-EINVAL);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:fix race condition bug

[Christian König]

Am 07.04.2017 um 12:52 schrieb Monk Liu:
> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
> Signed-off-by: Monk Liu <Monk.Liu@amd.com>

NAK, that is a not backward compatible change to the IOCTL interface.

And BTW what's the background of it?

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 6d86eae..b8c11fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>   
>   	spin_lock(&ctx->ring_lock);
>   
> +	if (!seq)
> +		seq = ctx->rings[ring->idx].sequence - 1;
> +
>   	if (seq >= cring->sequence) {
>   		spin_unlock(&ctx->ring_lock);
>   		return ERR_PTR(-EINVAL);


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu:fix race condition bug

[Christian König]

Am 07.04.2017 um 13:26 schrieb Christian König:
> Am 07.04.2017 um 12:52 schrieb Monk Liu:
>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>
> NAK, that is a not backward compatible change to the IOCTL interface.

To make it clear, something like "if(seq == ~0L)" should work.

Christian.

>
> And BTW what's the background of it?
>
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 6d86eae..b8c11fe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct 
>> amdgpu_ctx *ctx,
>>         spin_lock(&ctx->ring_lock);
>>   +    if (!seq)
>> +        seq = ctx->rings[ring->idx].sequence - 1;
>> +
>>       if (seq >= cring->sequence) {
>>           spin_unlock(&ctx->ring_lock);
>>           return ERR_PTR(-EINVAL);
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

答复: [PATCH] drm/amdgpu:fix race condition bug

[Liu, Monk]

IOCTL will pass a seq value, or 0
And 0 means it want to let KMD choose the latest seq value

So this patch is needed for the caller which input 0 as seq value

I found some code invoke ctx_get_fence() like:

Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ;
Fence = amdgpu_ctx_get_fence(ctx, seq);

BTW: above code is from hybrid kernel branch, not in upstream branch

You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
Caller should not access sequence directly, 
And sequence should be accessed during protection of the ring_lock


Any idea ?


-----邮件原件-----
发件人: Christian König [mailto:deathsimple@vodafone.de] 
发送时间: 2017年4月7日 19:28
收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
主题: Re: [PATCH] drm/amdgpu:fix race condition bug

Am 07.04.2017 um 13:26 schrieb Christian König:
> Am 07.04.2017 um 12:52 schrieb Monk Liu:
>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>
> NAK, that is a not backward compatible change to the IOCTL interface.

To make it clear, something like "if(seq == ~0L)" should work.

Christian.

>
> And BTW what's the background of it?
>
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 6d86eae..b8c11fe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct 
>> amdgpu_ctx *ctx,
>>         spin_lock(&ctx->ring_lock);
>>   +    if (!seq)
>> +        seq = ctx->rings[ring->idx].sequence - 1;
>> +
>>       if (seq >= cring->sequence) {
>>           spin_unlock(&ctx->ring_lock);
>>           return ERR_PTR(-EINVAL);
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 答复: [PATCH] drm/amdgpu:fix race condition bug

[Christian König]

> IOCTL will pass a seq value, or 0
> And 0 means it want to let KMD choose the latest seq value
>
> So this patch is needed for the caller which input 0 as seq value
Yeah, I understand that. The problem is we already abused seq value 0 to 
be the "always" signaled fence number.

So your patch is accidentally redefining this. But we could use 
something like ~0ull instead.

> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ;
> Fence = amdgpu_ctx_get_fence(ctx, seq);
>
> BTW: above code is from hybrid kernel branch, not in upstream branch
>
> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
> Caller should not access sequence directly,
> And sequence should be accessed during protection of the ring_lock
Yeah, I see that this is actually a race.

Please use ~0ull to signal that we want to wait for the latest fence 
instead of 0.

With that done the change looks fine to me.

Christian.

Am 07.04.2017 um 15:32 schrieb Liu, Monk:
> IOCTL will pass a seq value, or 0
> And 0 means it want to let KMD choose the latest seq value
>
> So this patch is needed for the caller which input 0 as seq value
>
> I found some code invoke ctx_get_fence() like:
>
> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ;
> Fence = amdgpu_ctx_get_fence(ctx, seq);
>
> BTW: above code is from hybrid kernel branch, not in upstream branch
>
> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
> Caller should not access sequence directly,
> And sequence should be accessed during protection of the ring_lock
>
>
> Any idea ?
>
>
> -----邮件原件-----
> 发件人: Christian König [mailto:deathsimple@vodafone.de]
> 发送时间: 2017年4月7日 19:28
> 收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> 主题: Re: [PATCH] drm/amdgpu:fix race condition bug
>
> Am 07.04.2017 um 13:26 schrieb Christian König:
>> Am 07.04.2017 um 12:52 schrieb Monk Liu:
>>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> NAK, that is a not backward compatible change to the IOCTL interface.
> To make it clear, something like "if(seq == ~0L)" should work.
>
> Christian.
>
>> And BTW what's the background of it?
>>
>> Christian.
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index 6d86eae..b8c11fe 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct
>>> amdgpu_ctx *ctx,
>>>          spin_lock(&ctx->ring_lock);
>>>    +    if (!seq)
>>> +        seq = ctx->rings[ring->idx].sequence - 1;
>>> +
>>>        if (seq >= cring->sequence) {
>>>            spin_unlock(&ctx->ring_lock);
>>>            return ERR_PTR(-EINVAL);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

答复: 答复: [PATCH] drm/amdgpu:fix race condition bug

[Liu, Monk]

Christian,

UMD already use 0 to tell KMD it want to use latest sequence value ...
And for KMD, we always start sequence value from 1, so I think this patch doesn't break anything

But if you want to use ~0 presenting latest sequence, that will demand many changes in UMD side ...

BR Monk

-----邮件原件-----
发件人: Christian König [mailto:deathsimple@vodafone.de] 
发送时间: 2017年4月7日 21:42
收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
主题: Re: 答复: [PATCH] drm/amdgpu:fix race condition bug

> IOCTL will pass a seq value, or 0
> And 0 means it want to let KMD choose the latest seq value
>
> So this patch is needed for the caller which input 0 as seq value
Yeah, I understand that. The problem is we already abused seq value 0 to be the "always" signaled fence number.

So your patch is accidentally redefining this. But we could use something like ~0ull instead.

> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; Fence = 
> amdgpu_ctx_get_fence(ctx, seq);
>
> BTW: above code is from hybrid kernel branch, not in upstream branch
>
> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
> Caller should not access sequence directly, And sequence should be 
> accessed during protection of the ring_lock
Yeah, I see that this is actually a race.

Please use ~0ull to signal that we want to wait for the latest fence instead of 0.

With that done the change looks fine to me.

Christian.

Am 07.04.2017 um 15:32 schrieb Liu, Monk:
> IOCTL will pass a seq value, or 0
> And 0 means it want to let KMD choose the latest seq value
>
> So this patch is needed for the caller which input 0 as seq value
>
> I found some code invoke ctx_get_fence() like:
>
> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; Fence = 
> amdgpu_ctx_get_fence(ctx, seq);
>
> BTW: above code is from hybrid kernel branch, not in upstream branch
>
> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
> Caller should not access sequence directly, And sequence should be 
> accessed during protection of the ring_lock
>
>
> Any idea ?
>
>
> -----邮件原件-----
> 发件人: Christian König [mailto:deathsimple@vodafone.de]
> 发送时间: 2017年4月7日 19:28
> 收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> 主题: Re: [PATCH] drm/amdgpu:fix race condition bug
>
> Am 07.04.2017 um 13:26 schrieb Christian König:
>> Am 07.04.2017 um 12:52 schrieb Monk Liu:
>>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>> NAK, that is a not backward compatible change to the IOCTL interface.
> To make it clear, something like "if(seq == ~0L)" should work.
>
> Christian.
>
>> And BTW what's the background of it?
>>
>> Christian.
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index 6d86eae..b8c11fe 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct 
>>> amdgpu_ctx *ctx,
>>>          spin_lock(&ctx->ring_lock);
>>>    +    if (!seq)
>>> +        seq = ctx->rings[ring->idx].sequence - 1;
>>> +
>>>        if (seq >= cring->sequence) {
>>>            spin_unlock(&ctx->ring_lock);
>>>            return ERR_PTR(-EINVAL);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 答复: 答复: [PATCH] drm/amdgpu:fix race condition bug

[Christian König]

> UMD already use 0 to tell KMD it want to use latest sequence value ...
Well that was a bad idea. We already used 0 for the always signaled case 
IIRC.

> But if you want to use ~0 presenting latest sequence, that will demand many changes in UMD side ...
Please do so, using 0 clearly wasn't a good idea. And we should also add 
a define for this in amdgpu_drm.h.

We should probably send all interfaces to the public list for review, 
even when we aren't going to push them upstream.

Regards,
Christian.

Am 07.04.2017 um 15:59 schrieb Liu, Monk:
> Christian,
>
> UMD already use 0 to tell KMD it want to use latest sequence value ...
> And for KMD, we always start sequence value from 1, so I think this patch doesn't break anything
>
> But if you want to use ~0 presenting latest sequence, that will demand many changes in UMD side ...
>
> BR Monk
>
> -----邮件原件-----
> 发件人: Christian König [mailto:deathsimple@vodafone.de]
> 发送时间: 2017年4月7日 21:42
> 收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
> 主题: Re: 答复: [PATCH] drm/amdgpu:fix race condition bug
>
>> IOCTL will pass a seq value, or 0
>> And 0 means it want to let KMD choose the latest seq value
>>
>> So this patch is needed for the caller which input 0 as seq value
> Yeah, I understand that. The problem is we already abused seq value 0 to be the "always" signaled fence number.
>
> So your patch is accidentally redefining this. But we could use something like ~0ull instead.
>
>> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; Fence =
>> amdgpu_ctx_get_fence(ctx, seq);
>>
>> BTW: above code is from hybrid kernel branch, not in upstream branch
>>
>> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
>> Caller should not access sequence directly, And sequence should be
>> accessed during protection of the ring_lock
> Yeah, I see that this is actually a race.
>
> Please use ~0ull to signal that we want to wait for the latest fence instead of 0.
>
> With that done the change looks fine to me.
>
> Christian.
>
> Am 07.04.2017 um 15:32 schrieb Liu, Monk:
>> IOCTL will pass a seq value, or 0
>> And 0 means it want to let KMD choose the latest seq value
>>
>> So this patch is needed for the caller which input 0 as seq value
>>
>> I found some code invoke ctx_get_fence() like:
>>
>> Seq = parm.in.seq ? seq: ctx->rings[ring->idx].sequence -1 ; Fence =
>> amdgpu_ctx_get_fence(ctx, seq);
>>
>> BTW: above code is from hybrid kernel branch, not in upstream branch
>>
>> You can see above logic actually wrong, because the consistence of the sequence is not guaranteed.
>> Caller should not access sequence directly, And sequence should be
>> accessed during protection of the ring_lock
>>
>>
>> Any idea ?
>>
>>
>> -----邮件原件-----
>> 发件人: Christian König [mailto:deathsimple@vodafone.de]
>> 发送时间: 2017年4月7日 19:28
>> 收件人: Liu, Monk <Monk.Liu@amd.com>; amd-gfx@lists.freedesktop.org
>> 主题: Re: [PATCH] drm/amdgpu:fix race condition bug
>>
>> Am 07.04.2017 um 13:26 schrieb Christian König:
>>> Am 07.04.2017 um 12:52 schrieb Monk Liu:
>>>> Change-Id: Ib7a03f3cf5594deeb4ad333cc59b47a6bddfd1ad
>>>> Signed-off-by: Monk Liu <Monk.Liu@amd.com>
>>> NAK, that is a not backward compatible change to the IOCTL interface.
>> To make it clear, something like "if(seq == ~0L)" should work.
>>
>> Christian.
>>
>>> And BTW what's the background of it?
>>>
>>> Christian.
>>>
>>>> ---
>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 3 +++
>>>>     1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>>> index 6d86eae..b8c11fe 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>>> @@ -277,6 +277,9 @@ struct fence *amdgpu_ctx_get_fence(struct
>>>> amdgpu_ctx *ctx,
>>>>           spin_lock(&ctx->ring_lock);
>>>>     +    if (!seq)
>>>> +        seq = ctx->rings[ring->idx].sequence - 1;
>>>> +
>>>>         if (seq >= cring->sequence) {
>>>>             spin_unlock(&ctx->ring_lock);
>>>>             return ERR_PTR(-EINVAL);
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx