| Message ID | 20190610210613.GA21989@embeddedor (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2b90cb223320a93b1be6c2616efe6f9ff14d8b28 |
| Headers | show |
| Series | libnvdimm, region: Use struct_size() in kzalloc() | expand |
Hi all, Friendly ping: Who can take this, please? Thanks -- Gustavo On 6/10/19 4:06 PM, Gustavo A. R. Silva wrote: > 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 nd_region { > ... > struct nd_mapping mapping[0]; > }; > > instance = kzalloc(sizeof(struct nd_region) + sizeof(struct nd_mapping) * > 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, mapping, count), GFP_KERNEL); > > This code was detected with the help of Coccinelle. > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/nvdimm/region_devs.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index b4ef7d9ff22e..88becc87e234 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -1027,10 +1027,9 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, > } > region_buf = ndbr; > } else { > - nd_region = kzalloc(sizeof(struct nd_region) > - + sizeof(struct nd_mapping) > - * ndr_desc->num_mappings, > - GFP_KERNEL); > + nd_region = kzalloc(struct_size(nd_region, mapping, > + ndr_desc->num_mappings), > + GFP_KERNEL); > region_buf = nd_region; > } > >
On Mon, 2019-06-10 at 16:06 -0500, Gustavo A. R. Silva wrote: > 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 nd_region { > ... > struct nd_mapping mapping[0]; > }; > > instance = kzalloc(sizeof(struct nd_region) + sizeof(struct > nd_mapping) * > 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, mapping, count), GFP_KERNEL); > > This code was detected with the help of Coccinelle. > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/nvdimm/region_devs.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvdimm/region_devs.c > b/drivers/nvdimm/region_devs.c > index b4ef7d9ff22e..88becc87e234 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -1027,10 +1027,9 @@ static struct nd_region > *nd_region_create(struct nvdimm_bus *nvdimm_bus, > } > region_buf = ndbr; > } else { > - nd_region = kzalloc(sizeof(struct nd_region) > - + sizeof(struct nd_mapping) > - * ndr_desc->num_mappings, > - GFP_KERNEL); > + nd_region = kzalloc(struct_size(nd_region, mapping, > + ndr_desc->num_mappings), > + GFP_KERNEL); > region_buf = nd_region; > } > Hi Gustavo, The patch looks good to me, however it looks like it might've missed some instances where this replacement can be performed? One is just a few lines below from the above, in the 'else' block[1]. Additionally, maybe the Coccinelle script can be augmented to catch devm_kzalloc instances as well - there is one of those in this file[2]. Thanks, -Vishal [1]: https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96
Hi Vishal, On 8/28/19 1:51 PM, Verma, Vishal L wrote: [..] > > Hi Gustavo, > > The patch looks good to me, however it looks like it might've missed > some instances where this replacement can be performed? > struct_size() does not apply to those scenarios. See below... > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030 struct_size() only applies to structures of the following kind: struct foo { int stuff; struct boo entry[]; }; and this scenario includes two different structures: struct nd_region { ... struct nd_mapping mapping[0]; }; struct nd_blk_region { ... struct nd_region nd_region; }; > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96 > In this scenario struct_size() does not apply directly because of the following logic before the call to devm_kzalloc(): size_t flush_data_size = sizeof(void *); [..] for (i = 0; i < nd_region->ndr_mappings; i++) { [..] /* at least one null hint slot per-dimm for the "no-hint" case */ flush_data_size += sizeof(void *); [..] flush_data_size += nvdimm->num_flush * sizeof(void *); } Thanks -- Gustavo
On Wed, 2019-08-28 at 14:36 -0500, Gustavo A. R. Silva wrote: > struct_size() does not apply to those scenarios. See below... > > > [1]: > > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030 > > struct_size() only applies to structures of the following kind: > > struct foo { > int stuff; > struct boo entry[]; > }; > > and this scenario includes two different structures: > > struct nd_region { > ... > struct nd_mapping mapping[0]; > }; > > struct nd_blk_region { > ... > struct nd_region nd_region; > }; Yep - I neglected to actually look at the structures involved - you're right, it doesn't apply here. > > > [2]: > > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96 > > > > In this scenario struct_size() does not apply directly because of the > following > logic before the call to devm_kzalloc(): Agreed, I missed that the calculation was more involved here. Thanks for the clarifications, you can add: Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
On Wed, Aug 28, 2019 at 01:30:24PM -0500, Gustavo A. R. Silva wrote: > Hi all, > > Friendly ping: > > Who can take this, please? > > Thanks > -- > Gustavo > > On 6/10/19 4:06 PM, Gustavo A. R. Silva wrote: > > 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 nd_region { > > ... > > struct nd_mapping mapping[0]; > > }; > > > > instance = kzalloc(sizeof(struct nd_region) + sizeof(struct nd_mapping) * > > 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, mapping, count), GFP_KERNEL); > > > > This code was detected with the help of Coccinelle. > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> FWIW, Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > --- > > drivers/nvdimm/region_devs.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > > index b4ef7d9ff22e..88becc87e234 100644 > > --- a/drivers/nvdimm/region_devs.c > > +++ b/drivers/nvdimm/region_devs.c > > @@ -1027,10 +1027,9 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, > > } > > region_buf = ndbr; > > } else { > > - nd_region = kzalloc(sizeof(struct nd_region) > > - + sizeof(struct nd_mapping) > > - * ndr_desc->num_mappings, > > - GFP_KERNEL); > > + nd_region = kzalloc(struct_size(nd_region, mapping, > > + ndr_desc->num_mappings), > > + GFP_KERNEL); > > region_buf = nd_region; > > } > > > >
On Wed, Aug 28, 2019 at 1:24 PM Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > On Wed, 2019-08-28 at 14:36 -0500, Gustavo A. R. Silva wrote: > > > struct_size() does not apply to those scenarios. See below... > > > > > [1]: > > > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n1030 > > > > struct_size() only applies to structures of the following kind: > > > > struct foo { > > int stuff; > > struct boo entry[]; > > }; > > > > and this scenario includes two different structures: > > > > struct nd_region { > > ... > > struct nd_mapping mapping[0]; > > }; > > > > struct nd_blk_region { > > ... > > struct nd_region nd_region; > > }; > > Yep - I neglected to actually look at the structures involved - you're > right, it doesn't apply here. > > > > > > [2]: > > > https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tree/drivers/nvdimm/region_devs.c#n96 > > > > > > > In this scenario struct_size() does not apply directly because of the > > following > > logic before the call to devm_kzalloc(): > > Agreed, I missed that the calculation was more involved here. > > Thanks for the clarifications, you can add: > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> Thanks, applied.
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c index b4ef7d9ff22e..88becc87e234 100644 --- a/drivers/nvdimm/region_devs.c +++ b/drivers/nvdimm/region_devs.c @@ -1027,10 +1027,9 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, } region_buf = ndbr; } else { - nd_region = kzalloc(sizeof(struct nd_region) - + sizeof(struct nd_mapping) - * ndr_desc->num_mappings, - GFP_KERNEL); + nd_region = kzalloc(struct_size(nd_region, mapping, + ndr_desc->num_mappings), + GFP_KERNEL); region_buf = nd_region; }
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 nd_region { ... struct nd_mapping mapping[0]; }; instance = kzalloc(sizeof(struct nd_region) + sizeof(struct nd_mapping) * 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, mapping, count), GFP_KERNEL); This code was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/nvdimm/region_devs.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)