Messages in this thread |  | | Date | Sat, 23 Oct 2021 13:28:38 +0200 | From | Len Baker <> | Subject | Re: [PATCH] nvmet: prefer struct_size over open coded arithmetic |
| |
Hi Gustavo,
first of all, thanks for this review (and all others reviews as well) ;)
More below.
On Sun, Oct 17, 2021 at 12:23:57PM -0500, Gustavo A. R. Silva wrote: > On Sun, Oct 17, 2021 at 11:56:50AM +0200, Len Baker wrote: > > As noted in the "Deprecated Interfaces, Language Features, Attributes, > > and Conventions" documentation [1], size calculations (especially > > multiplication) should not be performed in memory allocator (or similar) > > function arguments due to the risk of them overflowing. This could lead > > to values wrapping around and a smaller allocation being made than the > > caller was expecting. Using those allocations could lead to linear > > overflows of heap memory and other misbehaviors. > > > > In this case this is not actually dynamic size: all the operands > > involved in the calculation are constant values. However it is better to > > refactor this anyway, just to keep the open-coded math idiom out of > > code. > > > > So, use the struct_size() helper to do the arithmetic instead of the > > argument "size + count * size" in the kmalloc() function. > > > > This code was detected with the help of Coccinelle and audited and fixed > > manually. > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > > > Signed-off-by: Len Baker <len.baker@gmx.com> > > --- > > Hi, > > > > this patch is built against the linux-next tree (tag next-20211015). > > You don't need to include these lines in every patch. Just add [next] > to the subject line, like this: > > [PATCH][next] nvmet: prefer struct_size over open coded arithmetic > > It should be clear enough for people that you are talking about > linux-next. And in case someone asks, then you proceed to clarify. :)
Ok, understood. Thanks for the advise.
> > Regards, > > Len > > > > drivers/nvme/target/admin-cmd.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c > > index aa6d84d8848e..4aa71625c86a 100644 > > --- a/drivers/nvme/target/admin-cmd.c > > +++ b/drivers/nvme/target/admin-cmd.c > > @@ -278,8 +278,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req) > > u16 status; > > > > status = NVME_SC_INTERNAL; > > - desc = kmalloc(sizeof(struct nvme_ana_group_desc) + > > - NVMET_MAX_NAMESPACES * sizeof(__le32), GFP_KERNEL); > > + desc = kmalloc(struct_size(desc, nsids, NVMET_MAX_NAMESPACES), > > + GFP_KERNEL); > > It might be worth exploring if the flexible array is actually needed, > once the allocation is always determined by NVMET_MAX_NAMESPACES. Maybe > it can be changed to the following and remove the dynamic allocation > entirely? > > struct nvme_ana_group_desc { > __le32 grpid; > __le32 nnsids; > __le64 chgcnt; > __u8 state; > __u8 rsvd17[15]; > __le32 nsids[NVMET_MAX_NAMESPACES]; > };
What's the size limit for dynamic allocation vs stack allocation? I think that NVMET_MAX_NAMESPACES * sizeof(__le32) = 1024 * 4 = 4096 bytes is big enough (but I don't know if it is the correct way to think).
However, due to the following comment in the NVMET_MAX_NAMESPACES macro definition:
/* * Nice round number that makes a list of nsids fit into a page. * Should become tunable at some point in the future. */ #define NVMET_MAX_NAMESPACES 1024
I think that it is better to use the dynamic allocation since in the future the struct size could be dynamic.
> > If the above is possible then (at least) these lines should be audited: > > drivers/nvme/host/multipath.c-551- if (WARN_ON_ONCE(offset > ctrl->ana_log_size - sizeof(*desc))) > > drivers/nvme/host/multipath.c-566- offset += sizeof(*desc); > drivers/nvme/host/multipath.c-567- if (WARN_ON_ONCE(offset > ctrl->ana_log_size - nsid_buf_size)) > > If the flexible array remains, then this line could use > flex_array_size(): > > drivers/nvme/host/multipath.c-555- nsid_buf_size = nr_nsids * sizeof(__le32);
Ok. I didn't see it. > > struct_size() could be used here, as well: > > drivers/nvme/host/multipath.c-847- ana_log_size = sizeof(struct nvme_ana_rsp_hdr) + > drivers/nvme/host/multipath.c:848: ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc) + > drivers/nvme/host/multipath.c-849- ctrl->max_namespaces * sizeof(__le32);
Sorry, but here it's not possible to use struct_size() due to
sizeof(struct nvme_ana_group_desc) + ctrl->max_namespaces * sizeof(__le32)
it's not one single element. The "sizeof(struct nvme_ana_group_desc)" is multiplied by "ctrl->nanagrpid" and then added "ctrl->max_namespaces * sizeof(__le32)".
> drivers/nvme/target/admin-cmd.c:267: return sizeof(struct nvme_ana_group_desc) + count * sizeof(__le32);
Ok. I forgot it. Apologies.
Again, thanks for your time and advises, Len
|  |