powerpc/ps3: Use struct_size() in kzalloc()
Subsystems (2)
PATCH 1/1 powerpc/ps3: Use struct_size() in kzalloc()
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
--- arch/powerpc/platforms/ps3/device-init.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
arch/powerpc/platforms/ps3/device-init.c +1 -3
@@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo, repo->dev_index, repo->dev_type, port, blk_size, num_blocks, num_regions); - p =kzalloc (sizeof (struct ps3_storage_device ) + - num_regions *sizeof (struct ps3_storage_region ), -GFP_KERNEL ); + p =kzalloc (struct_size (p, regions, num_regions),GFP_KERNEL );if (!p) { result = -ENOMEM ; goto fail_malloc;
Signature · 1 line
2.20.1
Geoff Levand
maintainerHi Gustavo,
Quoted 28 lines · Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:
struct foo { int stuff; void *entry[]; };instance =
kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);Instead of leaving these open-coded and prone to type mistakes, we can now use the new
struct_size()helper:instance =
kzalloc(struct_size(instance, entry, count), GFP_KERNEL);This code was detected with the help of Coccinelle.
Signed-off-by Gustavo A. R. Silva <[email protected]>--- arch/powerpc/platforms/ps3/device-init.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)arch/powerpc/platforms/ps3/device-init.c +0
-0
arch/powerpc/platforms/ps3/device-init.c static int ps3_setup_storage_dev(const struct ps3_repository_device *repo, · line 360
Quoted 10 lines
arch/powerpc/platforms/ps3/device-init.c +1
-3@@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo, repo->dev_index, repo->dev_type, port, blk_size, num_blocks, num_regions); - p =kzalloc (sizeof (struct ps3_storage_device ) + - num_regions *sizeof (struct ps3_storage_region ), -GFP_KERNEL ); + p =kzalloc (struct_size (p, regions, num_regions),GFP_KERNEL );if (!p) { result = -ENOMEM ; goto fail_malloc;
It seems to me the motivation for this change is just to have a code change. As I've said for other similar patches, I'm reluctant to accept such trivial changes to the PS3 code because it makes it harder for me to maintain the code in the long term. When I need to do a git bisect to track down a problem I generally have a set of debugging patches that I apply on top of the bisect. A change to the code like this creates the potential for a patch conflict that I then need to manually edit to resolve.
If this change was for relatively new code, or better, for a patch that was submitted but not yet merged, then my feelings would be different. I think it would be really useful to have something that scans patches in patchwork.
-Geoff
Hi Geoff,
Quoted 34 lines · Geoff Levand
Hi Gustavo,
Quoted 31 lines · Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:
struct foo { int stuff; void *entry[]; };instance =
kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);Instead of leaving these open-coded and prone to type mistakes, we can now use the new
struct_size()helper:instance =
kzalloc(struct_size(instance, entry, count), GFP_KERNEL);This code was detected with the help of Coccinelle.
Signed-off-by Gustavo A. R. Silva <[email protected]>--- arch/powerpc/platforms/ps3/device-init.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)arch/powerpc/platforms/ps3/device-init.c +0
-0@@ -354,9 +354,7 @@ static int ps3_setup_storage_dev(const struct ps3_repository_device *repo, repo->dev_index, repo->dev_type, port, blk_size, num_blocks, num_regions);
Quoted 16 lines
arch/powerpc/platforms/ps3/device-init.c static int ps3_setup_storage_dev(const struct ps3_repository_device *repo, · line 360
Quoted 7 lines
arch/powerpc/platforms/ps3/device-init.c +1
-3- p =kzalloc (sizeof (struct ps3_storage_device ) + - num_regions *sizeof (struct ps3_storage_region ), -GFP_KERNEL ); + p =kzalloc (struct_size (p, regions, num_regions),GFP_KERNEL );if (!p) { result = -ENOMEM ; goto fail_malloc;It seems to me the motivation for this change is just to have a code change. As I've said for other similar patches, I'm reluctant to accept such trivial changes to the PS3 code because it makes it harder for me to maintain the code in the long term. When I need to do a git bisect to track down a problem I generally have a set of debugging patches that I apply on top of the bisect. A change to the code like this creates the potential for a patch conflict that I then need to manually edit to resolve.
This patch is part of a treewide effort aimed at preventing potential integer overflows during allocation. As the commit log says, the intention is to promote the use of struct_size instead of an open-coded form that can be prone to mistakes. This macro is a defense-in-depth strategy against overflows and it is a good idea to use it as widely as possible.
I'm not stopping to see how old the code is. I'm only focusing on the particular context of the memory allocation to understand what is the name of the zero-sized array that should be used for struct_size(), and if this macro can accurately replace the open-coded form.
Quoted 5 lines
If this change was for relatively new code, or better, for a patch that was submitted but not yet merged, then my feelings would be different. I think it would be really useful to have something that scans patches in patchwork.
I understand your point.
Thanks
--
Gustavo
Michael Ellerman
AppliedQuoted 11 lines · Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:
struct foo { int stuff; void *entry[]; };instance =
kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Quoted 8 lines
Instead of leaving these open-coded and prone to type mistakes, we can now use the new
struct_size()helper:instance =
kzalloc(struct_size(instance, entry, count), GFP_KERNEL);This code was detected with the help of Coccinelle.
Signed-off-by Gustavo A. R. Silva <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/31367b9a01d6a3f4f77694bd44f547d6
cheers
Quoted 17 lines · Michael Ellerman
Quoted 16 lines · Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:
struct foo { int stuff; void *entry[]; };instance =
kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);Instead of leaving these open-coded and prone to type mistakes, we can now use the new
struct_size()helper:instance =
kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
Quoted 8 lines
Quoted 3 lines
This code was detected with the help of Coccinelle.
Signed-off-by Gustavo A. R. Silva <[email protected]>Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/31367b9a01d6a3f4f77694bd44f547d6
Thanks, Michael.
--
Gustavo