This loop in the error handling code should start a "i - 1" and end at
"i == 0". Currently it starts a "i" and ends at "i == 1". The result
is that it removes one attribute that wasn't created yet, and leaks the
zeroeth attribute.
Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index b75362bf0742..ee4a8e44fbeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1931,7 +1931,7 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
uint32_t mask)
{
int ret = 0;
- uint32_t i = 0;
+ int i;
for (i = 0; i < counts; i++) {
ret = amdgpu_device_attr_create(adev, &attrs[i], mask);
@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
return 0;
failed:
- for (; i > 0; i--) {
+ while (--i >= 0)
amdgpu_device_attr_remove(adev, &attrs[i]);
- }
return ret;
}
--
2.26.2
Am 20.05.20 um 14:00 schrieb Dan Carpenter:
> This loop in the error handling code should start a "i - 1" and end at
> "i == 0". Currently it starts a "i" and ends at "i == 1". The result
> is that it removes one attribute that wasn't created yet, and leaks the
> zeroeth attribute.
>
> Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index b75362bf0742..ee4a8e44fbeb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1931,7 +1931,7 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
> uint32_t mask)
> {
> int ret = 0;
> - uint32_t i = 0;
> + int i;
>
> for (i = 0; i < counts; i++) {
> ret = amdgpu_device_attr_create(adev, &attrs[i], mask);
> @@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
> return 0;
>
> failed:
> - for (; i > 0; i--) {
> + while (--i >= 0)
As far as I know the common idiom for this is while (i--) which even
works without changing the type of i to signed.
Christian.
> amdgpu_device_attr_remove(adev, &attrs[i]);
> - }
>
> return ret;
> }
On Wed, May 20, 2020 at 02:05:19PM +0200, Christian K?nig wrote:
> Am 20.05.20 um 14:00 schrieb Dan Carpenter:
> > This loop in the error handling code should start a "i - 1" and end at
> > "i == 0". Currently it starts a "i" and ends at "i == 1". The result
> > is that it removes one attribute that wasn't created yet, and leaks the
> > zeroeth attribute.
> >
> > Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > index b75362bf0742..ee4a8e44fbeb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > @@ -1931,7 +1931,7 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
> > uint32_t mask)
> > {
> > int ret = 0;
> > - uint32_t i = 0;
> > + int i;
> > for (i = 0; i < counts; i++) {
> > ret = amdgpu_device_attr_create(adev, &attrs[i], mask);
> > @@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
> > return 0;
> > failed:
> > - for (; i > 0; i--) {
> > + while (--i >= 0)
>
> As far as I know the common idiom for this is while (i--) which even works
> without changing the type of i to signed.
It's about 50/50, one way or the other. To me --i >= 0 seems far more
readable.
I've been trying to figure out which tool tells people to make iterators
unsigned so I can help them avoid it. :/ I understand how in theory
iterators could go above INT_MAX but if we're going above INT_MAX then
probably we should use a 64 bit type. There are very few times where 2
billion iterations is not enough but in those situations probably 4
billion is not enough either. So unsigned int iterators never or seldom
solve real life bugs but they regularly cause them.
regards,
dan carpenter
This loop in the error handling code should start a "i - 1" and end at
"i == 0". Currently it starts a "i" and ends at "i == 1". The result
is that it removes one attribute that wasn't created yet, and leaks the
zeroeth attribute.
Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: style change
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index b75362bf0742..e809534fabd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
return 0;
failed:
- for (; i > 0; i--) {
+ while (i--)
amdgpu_device_attr_remove(adev, &attrs[i]);
- }
return ret;
}
"off by on"
or
"off by one"
?
M
>-----Original Message-----
>From: dri-devel <[email protected]> On Behalf Of Dan
>Carpenter
>Sent: Wednesday, May 20, 2020 9:08 AM
>To: Alex Deucher <[email protected]>; Kevin Wang
><[email protected]>
>Cc: David Airlie <[email protected]>; [email protected]; linux-
>[email protected]; [email protected]; Hawking Zhang
><[email protected]>; Rui Huang <[email protected]>; dri-
>[email protected]; Evan Quan <[email protected]>; Kenneth
>Feng <[email protected]>; Christian K?nig
><[email protected]>; Yintian Tao <[email protected]>
>Subject: [PATCH v2] drm/amdgpu: off by on in
>amdgpu_device_attr_create_groups() error handling
>
>This loop in the error handling code should start a "i - 1" and end at
>"i == 0". Currently it starts a "i" and ends at "i == 1". The result
>is that it removes one attribute that wasn't created yet, and leaks the
>zeroeth attribute.
>
>Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
>Signed-off-by: Dan Carpenter <[email protected]>
>---
>v2: style change
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>index b75362bf0742..e809534fabd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct
>amdgpu_device *adev,
> return 0;
>
> failed:
>- for (; i > 0; i--) {
>+ while (i--)
> amdgpu_device_attr_remove(adev, &attrs[i]);
>- }
>
> return ret;
> }
>_______________________________________________
>dri-devel mailing list
>[email protected]
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
This loop in the error handling code should start a "i - 1" and end at
"i == 0". Currently it starts a "i" and ends at "i == 1". The result
is that it removes one attribute that wasn't created yet, and leaks the
zeroeth attribute.
Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: style change
v3: Fix embarrassing typo in the subject
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index b75362bf0742..e809534fabd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct amdgpu_device *adev,
return 0;
failed:
- for (; i > 0; i--) {
+ while (i--)
amdgpu_device_attr_remove(adev, &attrs[i]);
- }
return ret;
}
>-----Original Message-----
>From: Dan Carpenter <[email protected]>
>Sent: Wednesday, May 20, 2020 11:26 AM
>To: Alex Deucher <[email protected]>; Kevin Wang
><[email protected]>; Ruhl, Michael J <[email protected]>
>Cc: Christian König <[email protected]>; David Airlie
><[email protected]>; Daniel Vetter <[email protected]>; Evan Quan
><[email protected]>; Rui Huang <[email protected]>; Kenneth Feng
><[email protected]>; Yintian Tao <[email protected]>; Hawking Zhang
><[email protected]>; [email protected]; dri-
>[email protected]; [email protected]; kernel-
>[email protected]
>Subject: [PATCH v3] drm/amdgpu: off by one in
>amdgpu_device_attr_create_groups() error handling
>
>This loop in the error handling code should start a "i - 1" and end at
>"i == 0". Currently it starts a "i" and ends at "i == 1". The result
>is that it removes one attribute that wasn't created yet, and leaks the
>zeroeth attribute.
>
>Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
>Signed-off-by: Dan Carpenter <[email protected]>
>---
>v2: style change
>v3: Fix embarrassing typo in the subject
????
Acked-by: Michael J. Ruhl <[email protected]>
m
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>index b75362bf0742..e809534fabd4 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>@@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct
>amdgpu_device *adev,
> return 0;
>
> failed:
>- for (; i > 0; i--) {
>+ while (i--)
> amdgpu_device_attr_remove(adev, &attrs[i]);
>- }
>
> return ret;
> }
Am 20.05.20 um 17:31 schrieb Ruhl, Michael J:
>> -----Original Message-----
>> From: Dan Carpenter <[email protected]>
>> Sent: Wednesday, May 20, 2020 11:26 AM
>> To: Alex Deucher <[email protected]>; Kevin Wang
>> <[email protected]>; Ruhl, Michael J <[email protected]>
>> Cc: Christian König <[email protected]>; David Airlie
>> <[email protected]>; Daniel Vetter <[email protected]>; Evan Quan
>> <[email protected]>; Rui Huang <[email protected]>; Kenneth Feng
>> <[email protected]>; Yintian Tao <[email protected]>; Hawking Zhang
>> <[email protected]>; [email protected]; dri-
>> [email protected]; [email protected]; kernel-
>> [email protected]
>> Subject: [PATCH v3] drm/amdgpu: off by one in
>> amdgpu_device_attr_create_groups() error handling
>>
>> This loop in the error handling code should start a "i - 1" and end at
>> "i == 0". Currently it starts a "i" and ends at "i == 1". The result
>> is that it removes one attribute that wasn't created yet, and leaks the
>> zeroeth attribute.
>>
>> Fixes: 4e01847c38f7 ("drm/amdgpu: optimize amdgpu device attribute code")
>> Signed-off-by: Dan Carpenter <[email protected]>
>> ---
>> v2: style change
>> v3: Fix embarrassing typo in the subject
> ????
>
> Acked-by: Michael J. Ruhl <[email protected]>
Reviewed-by: Christian König <[email protected]>
>
> m
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> index b75362bf0742..e809534fabd4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> @@ -1942,9 +1942,8 @@ static int amdgpu_device_attr_create_groups(struct
>> amdgpu_device *adev,
>> return 0;
>>
>> failed:
>> - for (; i > 0; i--) {
>> + while (i--)
>> amdgpu_device_attr_remove(adev, &attrs[i]);
>> - }
>>
>> return ret;
>> }