* [PATCH-next] Fix unintentional integer overflow
@ 2024-10-04 8:16 Advait Dhamorikar
[not found] ` <00761132-75f3-41fd-b571-30b0cbe5565d@amd.com>
0 siblings, 1 reply; 9+ messages in thread
From: Advait Dhamorikar @ 2024-10-04 8:16 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, Xinhui.Pan, airlied, simona,
leo.liu, sathishkumar.sundararaju, saleemkhan.jamadar,
Veerabadhran.Gopalakrishnan, advaitdhamorikar, sonny.jiang
Cc: amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail
Fix shift-count-overflow when creating mask.
The expression's value may not be what the
programmer intended, because the expression is
evaluated using a narrower integer type.
Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
index 95e2796919fc..7df402c45f40 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
@@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
ring = &adev->jpeg.inst[i].ring_dec[j];
if (ring->sched.ready)
- mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
+ mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
}
}
*val = mask;
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
[not found] ` <00761132-75f3-41fd-b571-30b0cbe5565d@amd.com>
@ 2024-10-04 17:33 ` Shuah Khan
2024-10-04 18:00 ` Alex Deucher
1 sibling, 0 replies; 9+ messages in thread
From: Shuah Khan @ 2024-10-04 17:33 UTC (permalink / raw)
To: Sundararaju, Sathishkumar, Advait Dhamorikar, alexander.deucher,
christian.koenig, Xinhui.Pan, airlied, simona, leo.liu,
sathishkumar.sundararaju, saleemkhan.jamadar,
Veerabadhran.Gopalakrishnan, sonny.jiang
Cc: amd-gfx, dri-devel, linux-kernel, anupnewsmail, Shuah Khan
On 10/4/24 03:15, Sundararaju, Sathishkumar wrote:
>
> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
>
> Sorry Alex, I missed to note this yesterday.
>
>
> Regards,
> Sathish
Sathish,
Please don't post on top when responding to kernel emails
and patches. It makes it difficult to follow the discussions
>
>
> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
>> Fix shift-count-overflow when creating mask.
>> The expression's value may not be what the
>> programmer intended, because the expression is
>> evaluated using a narrower integer type.
>>
>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
>> Signed-off-by: Advait Dhamorikar<advaitdhamorikar@gmail.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> index 95e2796919fc..7df402c45f40 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
>> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>> ring = &adev->jpeg.inst[i].ring_dec[j];
>> if (ring->sched.ready)
>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>> }
>> }
>> *val = mask;
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
[not found] ` <00761132-75f3-41fd-b571-30b0cbe5565d@amd.com>
2024-10-04 17:33 ` Shuah Khan
@ 2024-10-04 18:00 ` Alex Deucher
2024-10-05 3:34 ` Sundararaju, Sathishkumar
1 sibling, 1 reply; 9+ messages in thread
From: Alex Deucher @ 2024-10-04 18:00 UTC (permalink / raw)
To: Sundararaju, Sathishkumar
Cc: Advait Dhamorikar, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, simona, leo.liu, sathishkumar.sundararaju,
saleemkhan.jamadar, Veerabadhran.Gopalakrishnan, sonny.jiang,
amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail
On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
<sasundar@amd.com> wrote:
>
>
> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
>
> Sorry Alex, I missed to note this yesterday.
I've dropped the patch. Please pick it up once it's fixed up appropriately.
Thanks,
Alex
>
>
> Regards,
> Sathish
>
>
> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
>
> Fix shift-count-overflow when creating mask.
> The expression's value may not be what the
> programmer intended, because the expression is
> evaluated using a narrower integer type.
>
> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> index 95e2796919fc..7df402c45f40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
> ring = &adev->jpeg.inst[i].ring_dec[j];
> if (ring->sched.ready)
> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> }
> }
> *val = mask;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
2024-10-04 18:00 ` Alex Deucher
@ 2024-10-05 3:34 ` Sundararaju, Sathishkumar
2024-10-05 7:05 ` Advait Dhamorikar
0 siblings, 1 reply; 9+ messages in thread
From: Sundararaju, Sathishkumar @ 2024-10-05 3:34 UTC (permalink / raw)
To: Alex Deucher
Cc: Advait Dhamorikar, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, simona, leo.liu, sathishkumar.sundararaju,
saleemkhan.jamadar, Veerabadhran.Gopalakrishnan, sonny.jiang,
amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail, Lazar,
Lijo
On 10/4/2024 11:30 PM, Alex Deucher wrote:
> On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
> <sasundar@amd.com> wrote:
>>
>> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
>>
>> Sorry Alex, I missed to note this yesterday.
> I've dropped the patch. Please pick it up once it's fixed up appropriately.
Thanks Alex.
Hi Advait,
Please collate the changes together with Lijo's suggestion as well,
"1ULL <<" instead of typecast, there are 3 occurrences of the error in
f0b19b84d391.
Regards,
Sathish
>
> Thanks,
>
> Alex
>
>>
>> Regards,
>> Sathish
>>
>>
>> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
>>
>> Fix shift-count-overflow when creating mask.
>> The expression's value may not be what the
>> programmer intended, because the expression is
>> evaluated using a narrower integer type.
>>
>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
>> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> index 95e2796919fc..7df402c45f40 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
>> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>> ring = &adev->jpeg.inst[i].ring_dec[j];
>> if (ring->sched.ready)
>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>> }
>> }
>> *val = mask;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
2024-10-05 3:34 ` Sundararaju, Sathishkumar
@ 2024-10-05 7:05 ` Advait Dhamorikar
2024-10-07 13:56 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Advait Dhamorikar @ 2024-10-05 7:05 UTC (permalink / raw)
To: Sundararaju, Sathishkumar
Cc: Alex Deucher, alexander.deucher, christian.koenig, Xinhui.Pan,
airlied, simona, leo.liu, sathishkumar.sundararaju,
saleemkhan.jamadar, Veerabadhran.Gopalakrishnan, sonny.jiang,
amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail, Lazar,
Lijo
Hi Sathish,
> Please collate the changes together with Lijo's suggestion as well,
> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> f0b19b84d391.
I could only observe two instances of this error in f0b19b84d391 at:
'mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;`
and `mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);`
There are a few instances where we can use 1U instead of int as
harvest_config uses unsigned int
(adev->jpeg.harvest_config & (1 << i)
However I think they should be fixed in a separate patch?
Thanks and regards,
Advait
On Sat, 5 Oct 2024 at 09:05, Sundararaju, Sathishkumar <sasundar@amd.com> wrote:
>
>
>
> On 10/4/2024 11:30 PM, Alex Deucher wrote:
> > On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
> > <sasundar@amd.com> wrote:
> >>
> >> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
> >>
> >> Sorry Alex, I missed to note this yesterday.
> > I've dropped the patch. Please pick it up once it's fixed up appropriately.
> Thanks Alex.
>
> Hi Advait,
> Please collate the changes together with Lijo's suggestion as well,
> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> f0b19b84d391.
>
> Regards,
> Sathish
> >
> > Thanks,
> >
> > Alex
> >
> >>
> >> Regards,
> >> Sathish
> >>
> >>
> >> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
> >>
> >> Fix shift-count-overflow when creating mask.
> >> The expression's value may not be what the
> >> programmer intended, because the expression is
> >> evaluated using a narrower integer type.
> >>
> >> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
> >> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >> index 95e2796919fc..7df402c45f40 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
> >> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
> >> ring = &adev->jpeg.inst[i].ring_dec[j];
> >> if (ring->sched.ready)
> >> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >> }
> >> }
> >> *val = mask;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
2024-10-05 7:05 ` Advait Dhamorikar
@ 2024-10-07 13:56 ` Christian König
2024-10-08 3:38 ` Advait Dhamorikar
0 siblings, 1 reply; 9+ messages in thread
From: Christian König @ 2024-10-07 13:56 UTC (permalink / raw)
To: Advait Dhamorikar, Sundararaju, Sathishkumar
Cc: Alex Deucher, alexander.deucher, christian.koenig, Xinhui.Pan,
airlied, simona, leo.liu, sathishkumar.sundararaju,
saleemkhan.jamadar, Veerabadhran.Gopalakrishnan, sonny.jiang,
amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail, Lazar,
Lijo
Am 05.10.24 um 09:05 schrieb Advait Dhamorikar:
> Hi Sathish,
>
>> Please collate the changes together with Lijo's suggestion as well,
>> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
>> f0b19b84d391.
> I could only observe two instances of this error in f0b19b84d391 at:
> 'mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;`
> and `mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);`
>
> There are a few instances where we can use 1U instead of int as
> harvest_config uses unsigned int
> (adev->jpeg.harvest_config & (1 << i)
> However I think they should be fixed in a separate patch?
No, all of this are numerical problems where not taken into account the
size of the destination type.
Saying that all of that are basically just style cleanups which doesn't
need to be back-ported in any way, so please drop the Fixes: tag.
And you should probably change the subject line to something like
"drm/amdgpu: cleanup shift coding style".
Regards,
Christian.
>
> Thanks and regards,
> Advait
>
> On Sat, 5 Oct 2024 at 09:05, Sundararaju, Sathishkumar <sasundar@amd.com> wrote:
>>
>>
>> On 10/4/2024 11:30 PM, Alex Deucher wrote:
>>> On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
>>> <sasundar@amd.com> wrote:
>>>> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
>>>>
>>>> Sorry Alex, I missed to note this yesterday.
>>> I've dropped the patch. Please pick it up once it's fixed up appropriately.
>> Thanks Alex.
>>
>> Hi Advait,
>> Please collate the changes together with Lijo's suggestion as well,
>> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
>> f0b19b84d391.
>>
>> Regards,
>> Sathish
>>> Thanks,
>>>
>>> Alex
>>>
>>>> Regards,
>>>> Sathish
>>>>
>>>>
>>>> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
>>>>
>>>> Fix shift-count-overflow when creating mask.
>>>> The expression's value may not be what the
>>>> programmer intended, because the expression is
>>>> evaluated using a narrower integer type.
>>>>
>>>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
>>>> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>> index 95e2796919fc..7df402c45f40 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
>>>> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>>>> ring = &adev->jpeg.inst[i].ring_dec[j];
>>>> if (ring->sched.ready)
>>>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>>>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>>>> }
>>>> }
>>>> *val = mask;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
2024-10-07 13:56 ` Christian König
@ 2024-10-08 3:38 ` Advait Dhamorikar
2024-10-08 6:56 ` Christian König
0 siblings, 1 reply; 9+ messages in thread
From: Advait Dhamorikar @ 2024-10-08 3:38 UTC (permalink / raw)
To: Christian König
Cc: Sundararaju, Sathishkumar, Alex Deucher, alexander.deucher,
christian.koenig, Xinhui.Pan, airlied, simona, leo.liu,
sathishkumar.sundararaju, saleemkhan.jamadar,
Veerabadhran.Gopalakrishnan, sonny.jiang, amd-gfx, dri-devel,
linux-kernel, skhan, anupnewsmail, Lazar, Lijo
Hi Christian,
I am not sure if I correctly understood what you meant, just to clarify
When you say this
>No, all of this are numerical problems where not taken into account the
>size of the destination type.
>Saying that all of that are basically just style cleanups which doesn't
>need to be back-ported in any way, so please drop the Fixes: tag.
>And you should probably change the subject line to something like
>"drm/amdgpu: cleanup shift coding style".
Are you just talking about this message?
>> There are a few instances where we can use 1U instead of int as
>> harvest_config uses unsigned int
>>(adev->jpeg.harvest_config & (1 << i)
>> However I think they should be fixed in a separate patch?
Or is it intended for the complete previous "Fix unintentional
overflow" patch as well?
And I should just send a v3 with the two changes?
Thanks and regards,
Advait
On Mon, 7 Oct 2024 at 19:26, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 05.10.24 um 09:05 schrieb Advait Dhamorikar:
> > Hi Sathish,
> >
> >> Please collate the changes together with Lijo's suggestion as well,
> >> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> >> f0b19b84d391.
> > I could only observe two instances of this error in f0b19b84d391 at:
> > 'mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;`
> > and `mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);`
> >
> > There are a few instances where we can use 1U instead of int as
> > harvest_config uses unsigned int
> > (adev->jpeg.harvest_config & (1 << i)
> > However I think they should be fixed in a separate patch?
>
> No, all of this are numerical problems where not taken into account the
> size of the destination type.
>
> Saying that all of that are basically just style cleanups which doesn't
> need to be back-ported in any way, so please drop the Fixes: tag.
>
> And you should probably change the subject line to something like
> "drm/amdgpu: cleanup shift coding style".
>
> Regards,
> Christian.
>
> >
> > Thanks and regards,
> > Advait
> >
> > On Sat, 5 Oct 2024 at 09:05, Sundararaju, Sathishkumar <sasundar@amd.com> wrote:
> >>
> >>
> >> On 10/4/2024 11:30 PM, Alex Deucher wrote:
> >>> On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
> >>> <sasundar@amd.com> wrote:
> >>>> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
> >>>>
> >>>> Sorry Alex, I missed to note this yesterday.
> >>> I've dropped the patch. Please pick it up once it's fixed up appropriately.
> >> Thanks Alex.
> >>
> >> Hi Advait,
> >> Please collate the changes together with Lijo's suggestion as well,
> >> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
> >> f0b19b84d391.
> >>
> >> Regards,
> >> Sathish
> >>> Thanks,
> >>>
> >>> Alex
> >>>
> >>>> Regards,
> >>>> Sathish
> >>>>
> >>>>
> >>>> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
> >>>>
> >>>> Fix shift-count-overflow when creating mask.
> >>>> The expression's value may not be what the
> >>>> programmer intended, because the expression is
> >>>> evaluated using a narrower integer type.
> >>>>
> >>>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
> >>>> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> >>>> ---
> >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> index 95e2796919fc..7df402c45f40 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
> >>>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
> >>>> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
> >>>> ring = &adev->jpeg.inst[i].ring_dec[j];
> >>>> if (ring->sched.ready)
> >>>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >>>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
> >>>> }
> >>>> }
> >>>> *val = mask;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-next] Fix unintentional integer overflow
2024-10-08 3:38 ` Advait Dhamorikar
@ 2024-10-08 6:56 ` Christian König
0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2024-10-08 6:56 UTC (permalink / raw)
To: Advait Dhamorikar, Christian König
Cc: Sundararaju, Sathishkumar, Alex Deucher, alexander.deucher,
Xinhui.Pan, airlied, simona, leo.liu, sathishkumar.sundararaju,
saleemkhan.jamadar, Veerabadhran.Gopalakrishnan, sonny.jiang,
amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail, Lazar,
Lijo
Am 08.10.24 um 05:38 schrieb Advait Dhamorikar:
> Hi Christian,
>
> I am not sure if I correctly understood what you meant, just to clarify
>
> When you say this
>> No, all of this are numerical problems where not taken into account the
>> size of the destination type.
>> Saying that all of that are basically just style cleanups which doesn't
>> need to be back-ported in any way, so please drop the Fixes: tag.
>> And you should probably change the subject line to something like
>> "drm/amdgpu: cleanup shift coding style".
> Are you just talking about this message?
>>> There are a few instances where we can use 1U instead of int as
>>> harvest_config uses unsigned int
>>> (adev->jpeg.harvest_config & (1 << i)
>>> However I think they should be fixed in a separate patch?
> Or is it intended for the complete previous "Fix unintentional
> overflow" patch as well?
My comment applies to all patches.
Those patches are not really fixing anything because the shift values
come from some BIOS table and if I remember correctly for example the
harvest config only contains two meaningful bits.
Fixing the warnings is nice to have, but not really necessary for
correctness. So the patches shouldn't be back-ported and don't need any
Fixes tags.
Regards,
Christian.
> And I should just send a v3 with the two changes?
>
> Thanks and regards,
> Advait
>
> On Mon, 7 Oct 2024 at 19:26, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 05.10.24 um 09:05 schrieb Advait Dhamorikar:
>>> Hi Sathish,
>>>
>>>> Please collate the changes together with Lijo's suggestion as well,
>>>> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
>>>> f0b19b84d391.
>>> I could only observe two instances of this error in f0b19b84d391 at:
>>> 'mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;`
>>> and `mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);`
>>>
>>> There are a few instances where we can use 1U instead of int as
>>> harvest_config uses unsigned int
>>> (adev->jpeg.harvest_config & (1 << i)
>>> However I think they should be fixed in a separate patch?
>> No, all of this are numerical problems where not taken into account the
>> size of the destination type.
>>
>> Saying that all of that are basically just style cleanups which doesn't
>> need to be back-ported in any way, so please drop the Fixes: tag.
>>
>> And you should probably change the subject line to something like
>> "drm/amdgpu: cleanup shift coding style".
>>
>> Regards,
>> Christian.
>>
>>> Thanks and regards,
>>> Advait
>>>
>>> On Sat, 5 Oct 2024 at 09:05, Sundararaju, Sathishkumar <sasundar@amd.com> wrote:
>>>>
>>>> On 10/4/2024 11:30 PM, Alex Deucher wrote:
>>>>> On Fri, Oct 4, 2024 at 5:15 AM Sundararaju, Sathishkumar
>>>>> <sasundar@amd.com> wrote:
>>>>>> All occurrences of this error fix should have been together in a single patch both in _get and _set callbacks corresponding to f0b19b84d391, please avoid separate patch for each occurrence.
>>>>>>
>>>>>> Sorry Alex, I missed to note this yesterday.
>>>>> I've dropped the patch. Please pick it up once it's fixed up appropriately.
>>>> Thanks Alex.
>>>>
>>>> Hi Advait,
>>>> Please collate the changes together with Lijo's suggestion as well,
>>>> "1ULL <<" instead of typecast, there are 3 occurrences of the error in
>>>> f0b19b84d391.
>>>>
>>>> Regards,
>>>> Sathish
>>>>> Thanks,
>>>>>
>>>>> Alex
>>>>>
>>>>>> Regards,
>>>>>> Sathish
>>>>>>
>>>>>>
>>>>>> On 10/4/2024 1:46 PM, Advait Dhamorikar wrote:
>>>>>>
>>>>>> Fix shift-count-overflow when creating mask.
>>>>>> The expression's value may not be what the
>>>>>> programmer intended, because the expression is
>>>>>> evaluated using a narrower integer type.
>>>>>>
>>>>>> Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
>>>>>> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>> index 95e2796919fc..7df402c45f40 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
>>>>>> @@ -388,7 +388,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_get(void *data, u64 *val)
>>>>>> for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
>>>>>> ring = &adev->jpeg.inst[i].ring_dec[j];
>>>>>> if (ring->sched.ready)
>>>>>> - mask |= 1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>>>>>> + mask |= (u64)1 << ((i * adev->jpeg.num_jpeg_rings) + j);
>>>>>> }
>>>>>> }
>>>>>> *val = mask;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-next] Fix unintentional integer overflow
@ 2024-10-03 7:14 Advait Dhamorikar
0 siblings, 0 replies; 9+ messages in thread
From: Advait Dhamorikar @ 2024-10-03 7:14 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, Xinhui.Pan, airlied, simona,
leo.liu, sathishkumar.sundararaju, saleemkhan.jamadar,
Veerabadhran.Gopalakrishnan, advaitdhamorikar, sonny.jiang
Cc: amd-gfx, dri-devel, linux-kernel, skhan, anupnewsmail
Fix overflow issue by casting uint8_t to uint64_t in JPEG
instance multiplication.
The expression's value may not be what the programmer intended,
because the expression is evaluated using
a narrow (i.e. few bits) integer type.
Fixes: f0b19b84d391 ("drm/amdgpu: add amdgpu_jpeg_sched_mask debugfs")
Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
index 95e2796919fc..b6f0435f56ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_jpeg.c
@@ -357,7 +357,7 @@ static int amdgpu_debugfs_jpeg_sched_mask_set(void *data, u64 val)
if (!adev)
return -ENODEV;
- mask = (1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;
+ mask = ((uint64_t)1 << (adev->jpeg.num_jpeg_inst * adev->jpeg.num_jpeg_rings)) - 1;
if ((val & mask) == 0)
return -EINVAL;
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-08 6:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-04 8:16 [PATCH-next] Fix unintentional integer overflow Advait Dhamorikar
[not found] ` <00761132-75f3-41fd-b571-30b0cbe5565d@amd.com>
2024-10-04 17:33 ` Shuah Khan
2024-10-04 18:00 ` Alex Deucher
2024-10-05 3:34 ` Sundararaju, Sathishkumar
2024-10-05 7:05 ` Advait Dhamorikar
2024-10-07 13:56 ` Christian König
2024-10-08 3:38 ` Advait Dhamorikar
2024-10-08 6:56 ` Christian König
-- strict thread matches above, loose matches on Subject: below --
2024-10-03 7:14 Advait Dhamorikar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®