All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22  1:25 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22  1:25 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Daniel Vetter
  Cc: Gustavo A. R. Silva, dri-devel, amd-gfx, linux-kernel

The current codebase makes use of one-element arrays in the following
form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.

This issue was found with the help of Coccinelle and, audited and fixed
_manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
 drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
     uint8_t                             levelCount;
     uint8_t                             padding2;
     uint8_t                             padding3;
-    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
+    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
 };
 
 typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..7d81dde509dc9 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
 	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
 	u16 address = pi->state_table_start +
 		offsetof(NISLANDS_SMC_STATETABLE, driverState);
-	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
-		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+	NISLANDS_SMC_SWSTATE *smc_state;
+	u16 state_size = struct_size(smc_state, levels,
+			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
 	int ret;
-	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
 
+	smc_state = kzalloc(state_size, GFP_KERNEL);
 	if (smc_state == NULL)
 		return -ENOMEM;
 
-- 
2.26.2

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22  1:25 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22  1:25 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Daniel Vetter
  Cc: Gustavo A. R. Silva, dri-devel, amd-gfx, linux-kernel

The current codebase makes use of one-element arrays in the following
form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.

This issue was found with the help of Coccinelle and, audited and fixed
_manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
 drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
     uint8_t                             levelCount;
     uint8_t                             padding2;
     uint8_t                             padding3;
-    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
+    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
 };
 
 typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..7d81dde509dc9 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
 	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
 	u16 address = pi->state_table_start +
 		offsetof(NISLANDS_SMC_STATETABLE, driverState);
-	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
-		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+	NISLANDS_SMC_SWSTATE *smc_state;
+	u16 state_size = struct_size(smc_state, levels,
+			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
 	int ret;
-	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
 
+	smc_state = kzalloc(state_size, GFP_KERNEL);
 	if (smc_state == NULL)
 		return -ENOMEM;
 
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22  1:25 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22  1:25 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Daniel Vetter
  Cc: amd-gfx, dri-devel, linux-kernel, Gustavo A. R. Silva

The current codebase makes use of one-element arrays in the following
form:

struct something {
    int length;
    u8 data[1];
};

struct something *instance;

instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);

but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.

Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.

This issue was found with the help of Coccinelle and, audited and fixed
_manually_.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
 drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
     uint8_t                             levelCount;
     uint8_t                             padding2;
     uint8_t                             padding3;
-    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
+    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
 };
 
 typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..7d81dde509dc9 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
 	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
 	u16 address = pi->state_table_start +
 		offsetof(NISLANDS_SMC_STATETABLE, driverState);
-	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
-		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+	NISLANDS_SMC_SWSTATE *smc_state;
+	u16 state_size = struct_size(smc_state, levels,
+			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
 	int ret;
-	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
 
+	smc_state = kzalloc(state_size, GFP_KERNEL);
 	if (smc_state == NULL)
 		return -ENOMEM;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
  2020-05-22  1:25 ` Gustavo A. R. Silva
  (?)
@ 2020-05-22  7:00   ` Christian König
  -1 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2020-05-22  7:00 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Alex Deucher, David Airlie, Daniel Vetter
  Cc: Gustavo A. R. Silva, dri-devel, amd-gfx, linux-kernel

Am 22.05.20 um 03:25 schrieb Gustavo A. R. Silva:
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
>      int length;
>      u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
>          int stuff;
>          struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> size of struct NISLANDS_SMC_SWSTATE.
>
> This issue was found with the help of Coccinelle and, audited and fixed
> _manually_.
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FZero-Length.html&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=fQHnbXZpsgiMjHOr%2By0Uq12jpCYEYbdX5K26iNkwyeM%3D&amp;reserved=0
> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F21&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=gg20YupmqsaW%2Bg3VyJL%2BkjE3kFwnWF1RD1D2QP04OLk%3D&amp;reserved=0
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>   drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
>   drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> index 6b7d292b919f3..bc0be6818e218 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> @@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
>       uint8_t                             levelCount;
>       uint8_t                             padding2;
>       uint8_t                             padding3;
> -    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
> +    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
>   };
>   
>   typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
> diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
> index b57c37ddd164c..7d81dde509dc9 100644
> --- a/drivers/gpu/drm/radeon/ni_dpm.c
> +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
>   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
>   	u16 address = pi->state_table_start +
>   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> +	NISLANDS_SMC_SWSTATE *smc_state;
> +	u16 state_size = struct_size(smc_state, levels,
> +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);

Probably better to use size_t instead of u16 here. With that fixed feel 
free to stick my rb on the patch.

Regards,
Christian.

>   	int ret;
> -	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
>   
> +	smc_state = kzalloc(state_size, GFP_KERNEL);
>   	if (smc_state == NULL)
>   		return -ENOMEM;
>   

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22  7:00   ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2020-05-22  7:00 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Alex Deucher, David Airlie, Daniel Vetter
  Cc: Gustavo A. R. Silva, dri-devel, amd-gfx, linux-kernel

Am 22.05.20 um 03:25 schrieb Gustavo A. R. Silva:
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
>      int length;
>      u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
>          int stuff;
>          struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> size of struct NISLANDS_SMC_SWSTATE.
>
> This issue was found with the help of Coccinelle and, audited and fixed
> _manually_.
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FZero-Length.html&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=fQHnbXZpsgiMjHOr%2By0Uq12jpCYEYbdX5K26iNkwyeM%3D&amp;reserved=0
> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F21&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=gg20YupmqsaW%2Bg3VyJL%2BkjE3kFwnWF1RD1D2QP04OLk%3D&amp;reserved=0
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>   drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
>   drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> index 6b7d292b919f3..bc0be6818e218 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> @@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
>       uint8_t                             levelCount;
>       uint8_t                             padding2;
>       uint8_t                             padding3;
> -    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
> +    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
>   };
>   
>   typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
> diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
> index b57c37ddd164c..7d81dde509dc9 100644
> --- a/drivers/gpu/drm/radeon/ni_dpm.c
> +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
>   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
>   	u16 address = pi->state_table_start +
>   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> +	NISLANDS_SMC_SWSTATE *smc_state;
> +	u16 state_size = struct_size(smc_state, levels,
> +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);

Probably better to use size_t instead of u16 here. With that fixed feel 
free to stick my rb on the patch.

Regards,
Christian.

>   	int ret;
> -	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
>   
> +	smc_state = kzalloc(state_size, GFP_KERNEL);
>   	if (smc_state == NULL)
>   		return -ENOMEM;
>   

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22  7:00   ` Christian König
  0 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2020-05-22  7:00 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Alex Deucher, David Airlie, Daniel Vetter
  Cc: amd-gfx, dri-devel, linux-kernel, Gustavo A. R. Silva

Am 22.05.20 um 03:25 schrieb Gustavo A. R. Silva:
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
>      int length;
>      u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
>          int stuff;
>          struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> size of struct NISLANDS_SMC_SWSTATE.
>
> This issue was found with the help of Coccinelle and, audited and fixed
> _manually_.
>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FZero-Length.html&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=fQHnbXZpsgiMjHOr%2By0Uq12jpCYEYbdX5K26iNkwyeM%3D&amp;reserved=0
> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F21&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745&amp;sdata=gg20YupmqsaW%2Bg3VyJL%2BkjE3kFwnWF1RD1D2QP04OLk%3D&amp;reserved=0
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>   drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
>   drivers/gpu/drm/radeon/ni_dpm.c     | 7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> index 6b7d292b919f3..bc0be6818e218 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> @@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
>       uint8_t                             levelCount;
>       uint8_t                             padding2;
>       uint8_t                             padding3;
> -    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[1];
> +    NISLANDS_SMC_HW_PERFORMANCE_LEVEL   levels[];
>   };
>   
>   typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
> diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
> index b57c37ddd164c..7d81dde509dc9 100644
> --- a/drivers/gpu/drm/radeon/ni_dpm.c
> +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
>   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
>   	u16 address = pi->state_table_start +
>   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> +	NISLANDS_SMC_SWSTATE *smc_state;
> +	u16 state_size = struct_size(smc_state, levels,
> +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);

Probably better to use size_t instead of u16 here. With that fixed feel 
free to stick my rb on the patch.

Regards,
Christian.

>   	int ret;
> -	NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
>   
> +	smc_state = kzalloc(state_size, GFP_KERNEL);
>   	if (smc_state == NULL)
>   		return -ENOMEM;
>   


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
  2020-05-22  7:00   ` Christian König
  (?)
@ 2020-05-22 17:27     ` Gustavo A. R. Silva
  -1 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22 17:27 UTC (permalink / raw)
  To: Christian König
  Cc: Gustavo A. R. Silva, David Airlie, linux-kernel, dri-devel,
	amd-gfx, Daniel Vetter, Alex Deucher

On Fri, May 22, 2020 at 09:00:09AM +0200, Christian König wrote:
> > +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> > @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
> >   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
> >   	u16 address = pi->state_table_start +
> >   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> > -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> > -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> > +	NISLANDS_SMC_SWSTATE *smc_state;
> > +	u16 state_size = struct_size(smc_state, levels,
> > +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
> 
> Probably better to use size_t instead of u16 here. With that fixed feel free
> to stick my rb on the patch.
> 

Sure thing. I'll send v2, shortly.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22 17:27     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22 17:27 UTC (permalink / raw)
  To: Christian König
  Cc: Gustavo A. R. Silva, David Airlie, linux-kernel, dri-devel,
	amd-gfx, Alex Deucher

On Fri, May 22, 2020 at 09:00:09AM +0200, Christian König wrote:
> > +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> > @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
> >   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
> >   	u16 address = pi->state_table_start +
> >   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> > -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> > -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> > +	NISLANDS_SMC_SWSTATE *smc_state;
> > +	u16 state_size = struct_size(smc_state, levels,
> > +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
> 
> Probably better to use size_t instead of u16 here. With that fixed feel free
> to stick my rb on the patch.
> 

Sure thing. I'll send v2, shortly.

Thanks, Christian.
--
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
@ 2020-05-22 17:27     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-22 17:27 UTC (permalink / raw)
  To: Christian König
  Cc: Alex Deucher, David Airlie, Daniel Vetter, amd-gfx, dri-devel,
	linux-kernel, Gustavo A. R. Silva

On Fri, May 22, 2020 at 09:00:09AM +0200, Christian König wrote:
> > +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> > @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device *rdev,
> >   	struct rv7xx_power_info *pi = rv770_get_pi(rdev);
> >   	u16 address = pi->state_table_start +
> >   		offsetof(NISLANDS_SMC_STATETABLE, driverState);
> > -	u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> > -		((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> > +	NISLANDS_SMC_SWSTATE *smc_state;
> > +	u16 state_size = struct_size(smc_state, levels,
> > +			NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
> 
> Probably better to use size_t instead of u16 here. With that fixed feel free
> to stick my rb on the patch.
> 

Sure thing. I'll send v2, shortly.

Thanks, Christian.
--
Gustavo

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-22 17:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-22  1:25 [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper Gustavo A. R. Silva
2020-05-22  1:25 ` Gustavo A. R. Silva
2020-05-22  1:25 ` Gustavo A. R. Silva
2020-05-22  7:00 ` Christian König
2020-05-22  7:00   ` Christian König
2020-05-22  7:00   ` Christian König
2020-05-22 17:27   ` Gustavo A. R. Silva
2020-05-22 17:27     ` Gustavo A. R. Silva
2020-05-22 17:27     ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.