* [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2
[not found] <20240406142337.16241-1-erick.archer@outlook.com>
@ 2024-04-06 14:23 ` Erick Archer
2024-04-08 19:42 ` Long Li
2024-04-08 21:34 ` Justin Stitt
2024-04-06 14:23 ` [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic Erick Archer
2024-04-06 14:23 ` [PATCH v3 3/3] net: mana: Avoid " Erick Archer
2 siblings, 2 replies; 9+ messages in thread
From: Erick Archer @ 2024-04-06 14:23 UTC (permalink / raw)
To: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov
Cc: Erick Archer, linux-rdma, linux-hyperv, netdev, linux-kernel,
linux-hardening, llvm
The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
trailing elements. Specifically, it uses a "mana_handle_t" array. So,
use the preferred way in the kernel declaring a flexible array [1].
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).
This is a previous step to refactor the two consumers of this structure.
drivers/infiniband/hw/mana/qp.c
drivers/net/ethernet/microsoft/mana/mana_en.c
The ultimate goal is to avoid the open-coded arithmetic in the memory
allocator functions [2] using the "struct_size" macro.
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
include/net/mana/mana.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index 4eeedf14711b..561f6719fb4e 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -670,6 +670,7 @@ struct mana_cfg_rx_steer_req_v2 {
u8 hashkey[MANA_HASH_KEY_SIZE];
u8 cqe_coalescing_enable;
u8 reserved2[7];
+ mana_handle_t indir_tab[] __counted_by(num_indir_entries);
}; /* HW DATA */
struct mana_cfg_rx_steer_resp {
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2
2024-04-06 14:23 ` [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 Erick Archer
@ 2024-04-08 19:42 ` Long Li
2024-04-08 21:34 ` Justin Stitt
1 sibling, 0 replies; 9+ messages in thread
From: Long Li @ 2024-04-08 19:42 UTC (permalink / raw)
To: Erick Archer, Ajay Sharma, KY Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov
Cc: linux-rdma, linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
> Subject: [PATCH v3 1/3] net: mana: Add flex array to struct
> mana_cfg_rx_steer_req_v2
>
> The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of trailing
> elements. Specifically, it uses a "mana_handle_t" array. So, use the preferred way
> in the kernel declaring a flexible array [1].
>
> At the same time, prepare for the coming implementation by GCC and Clang of
> the __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
>
> This is a previous step to refactor the two consumers of this structure.
>
> drivers/infiniband/hw/mana/qp.c
> drivers/net/ethernet/microsoft/mana/mana_en.c
>
> The ultimate goal is to avoid the open-coded arithmetic in the memory allocator
> functions [2] using the "struct_size" macro.
>
> Link:
> https://www.ker/
> nel.org%2Fdoc%2Fhtml%2Fnext%2Fprocess%2Fdeprecated.html%23zero-length-
> and-one-element-
> arrays&data=05%7C02%7Clongli%40microsoft.com%7Ce75134553ebf4bca87bd0
> 8dc564acf8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63848012
> 6558204741%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=%2B8k08
> SWrKXJiDfQ2cal65b1K1sElRB8x0oA5EFeUqbw%3D&reserved=0 [1]
> Link:
> https://www.ker/
> nel.org%2Fdoc%2Fhtml%2Fnext%2Fprocess%2Fdeprecated.html%23open-coded-
> arithmetic-in-allocator-
> arguments&data=05%7C02%7Clongli%40microsoft.com%7Ce75134553ebf4bca8
> 7bd08dc564acf8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6384
> 80126558211762%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=h0
> wsUICWnJwn1Nd5fr%2B0z8SXZIqXQrNWKTEbVlB%2BNI0%3D&reserved=0 [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
Reviewed-by: Long Li <longli@microsoft.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2
2024-04-06 14:23 ` [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 Erick Archer
2024-04-08 19:42 ` Long Li
@ 2024-04-08 21:34 ` Justin Stitt
2024-04-08 21:43 ` Justin Stitt
1 sibling, 1 reply; 9+ messages in thread
From: Justin Stitt @ 2024-04-08 21:34 UTC (permalink / raw)
To: Erick Archer
Cc: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov, linux-rdma,
linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
Hi,
On Sat, Apr 06, 2024 at 04:23:35PM +0200, Erick Archer wrote:
> The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
> trailing elements. Specifically, it uses a "mana_handle_t" array. So,
> use the preferred way in the kernel declaring a flexible array [1].
>
> At the same time, prepare for the coming implementation by GCC and Clang
> of the __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> strcpy/memcpy-family functions).
>
> This is a previous step to refactor the two consumers of this structure.
>
> drivers/infiniband/hw/mana/qp.c
> drivers/net/ethernet/microsoft/mana/mana_en.c
>
> The ultimate goal is to avoid the open-coded arithmetic in the memory
> allocator functions [2] using the "struct_size" macro.
>
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
I think this could have all been one patch, I found myself jumping
around the three patches here piecing together context.
Reviewed-by: Justin Stitt <justinstitt@google.com>
> ---
> include/net/mana/mana.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 4eeedf14711b..561f6719fb4e 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -670,6 +670,7 @@ struct mana_cfg_rx_steer_req_v2 {
> u8 hashkey[MANA_HASH_KEY_SIZE];
> u8 cqe_coalescing_enable;
> u8 reserved2[7];
> + mana_handle_t indir_tab[] __counted_by(num_indir_entries);
> }; /* HW DATA */
>
> struct mana_cfg_rx_steer_resp {
> --
> 2.25.1
>
Thanks
Justin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2
2024-04-08 21:34 ` Justin Stitt
@ 2024-04-08 21:43 ` Justin Stitt
0 siblings, 0 replies; 9+ messages in thread
From: Justin Stitt @ 2024-04-08 21:43 UTC (permalink / raw)
To: Erick Archer
Cc: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov, linux-rdma,
linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
On Mon, Apr 8, 2024 at 2:35 PM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi,
>
> On Sat, Apr 06, 2024 at 04:23:35PM +0200, Erick Archer wrote:
> > The "struct mana_cfg_rx_steer_req_v2" uses a dynamically sized set of
> > trailing elements. Specifically, it uses a "mana_handle_t" array. So,
> > use the preferred way in the kernel declaring a flexible array [1].
> >
> > At the same time, prepare for the coming implementation by GCC and Clang
> > of the __counted_by attribute. Flexible array members annotated with
> > __counted_by can have their accesses bounds-checked at run-time via
> > CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> > strcpy/memcpy-family functions).
> >
> > This is a previous step to refactor the two consumers of this structure.
> >
> > drivers/infiniband/hw/mana/qp.c
> > drivers/net/ethernet/microsoft/mana/mana_en.c
> >
> > The ultimate goal is to avoid the open-coded arithmetic in the memory
> > allocator functions [2] using the "struct_size" macro.
> >
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> > Signed-off-by: Erick Archer <erick.archer@outlook.com>
>
> I think this could have all been one patch, I found myself jumping
> around the three patches here piecing together context.
I now see Leon said to combine them in v2. Whoops, sorry to give
conflicting feedback.
>
> Reviewed-by: Justin Stitt <justinstitt@google.com>
>
> > ---
> > include/net/mana/mana.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> > index 4eeedf14711b..561f6719fb4e 100644
> > --- a/include/net/mana/mana.h
> > +++ b/include/net/mana/mana.h
> > @@ -670,6 +670,7 @@ struct mana_cfg_rx_steer_req_v2 {
> > u8 hashkey[MANA_HASH_KEY_SIZE];
> > u8 cqe_coalescing_enable;
> > u8 reserved2[7];
> > + mana_handle_t indir_tab[] __counted_by(num_indir_entries);
> > }; /* HW DATA */
> >
> > struct mana_cfg_rx_steer_resp {
> > --
> > 2.25.1
> >
>
> Thanks
> Justin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic
[not found] <20240406142337.16241-1-erick.archer@outlook.com>
2024-04-06 14:23 ` [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 Erick Archer
@ 2024-04-06 14:23 ` Erick Archer
2024-04-08 19:42 ` Long Li
2024-04-08 21:38 ` Justin Stitt
2024-04-06 14:23 ` [PATCH v3 3/3] net: mana: Avoid " Erick Archer
2 siblings, 2 replies; 9+ messages in thread
From: Erick Archer @ 2024-04-06 14:23 UTC (permalink / raw)
To: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov
Cc: Erick Archer, linux-rdma, linux-hyperv, netdev, linux-kernel,
linux-hardening, llvm
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
and this structure ends in a flexible array:
struct mana_cfg_rx_steer_req_v2 {
[...]
mana_handle_t indir_tab[] __counted_by(num_indir_entries);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kzalloc() function.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member. This new structure member also allow
us to remove the "req_indir_tab" variable since it is no longer needed.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
drivers/infiniband/hw/mana/qp.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index ef0a6dc664d0..4cd8f8afe80d 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -15,15 +15,13 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
struct mana_port_context *mpc = netdev_priv(ndev);
struct mana_cfg_rx_steer_req_v2 *req;
struct mana_cfg_rx_steer_resp resp = {};
- mana_handle_t *req_indir_tab;
struct gdma_context *gc;
u32 req_buf_size;
int i, err;
gc = mdev_to_gc(dev);
- req_buf_size =
- sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
+ req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -44,20 +42,20 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
req->rss_enable = true;
req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+ indir_tab);
req->update_indir_tab = true;
req->cqe_coalescing_enable = 1;
- req_indir_tab = (mana_handle_t *)(req + 1);
/* The ind table passed to the hardware must have
* MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
* ind_table to MANA_INDIRECT_TABLE_SIZE if required
*/
ibdev_dbg(&dev->ib_dev, "ind table size %u\n", 1 << log_ind_tbl_size);
for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) {
- req_indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
+ req->indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
ibdev_dbg(&dev->ib_dev, "index %u handle 0x%llx\n", i,
- req_indir_tab[i]);
+ req->indir_tab[i]);
}
req->update_hashkey = true;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic
2024-04-06 14:23 ` [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic Erick Archer
@ 2024-04-08 19:42 ` Long Li
2024-04-08 21:38 ` Justin Stitt
1 sibling, 0 replies; 9+ messages in thread
From: Long Li @ 2024-04-08 19:42 UTC (permalink / raw)
To: Erick Archer, Ajay Sharma, KY Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov
Cc: linux-rdma, linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
> Subject: [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded
> arithmetic
>
> This is an effort to get rid of all multiplications from allocation functions in order to
> prevent integer overflows [1][2].
>
> As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
> and this structure ends in a flexible array:
>
> struct mana_cfg_rx_steer_req_v2 {
> [...]
> mana_handle_t indir_tab[] __counted_by(num_indir_entries); };
>
> the preferred way in the kernel is to use the struct_size() helper to do the
> arithmetic instead of the calculation "size + size * count" in the kzalloc() function.
>
> Moreover, use the "offsetof" helper to get the indirect table offset instead of the
> "sizeof" operator and avoid the open-coded arithmetic in pointers using the new
> flex member. This new structure member also allow us to remove the
> "req_indir_tab" variable since it is no longer needed.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and modified
> manually.
>
> Link:
> https://www.ker/
> nel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fdeprecated.html%23open-
> coded-arithmetic-in-allocator-
> arguments&data=05%7C02%7Clongli%40microsoft.com%7Cfcf2a410393a429633
> ca08dc56506b01%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63848
> 0150654917952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=2zSek
> zsyXsS1s9xw%2FwaSEl3h4s6AeiykFG4KiJLzXOc%3D&reserved=0 [1]
> Link:
> https://github.co/
> m%2FKSPP%2Flinux%2Fissues%2F160&data=05%7C02%7Clongli%40microsoft.co
> m%7Cfcf2a410393a429633ca08dc56506b01%7C72f988bf86f141af91ab2d7cd01
> 1db47%7C1%7C0%7C638480150654924997%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 0%7C%7C%7C&sdata=4pzQWVWVcIaeS07VgXY1I6%2FS%2FEFejUD4qv1D2Ouwf
> pA%3D&reserved=0 [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
Reviewed-by: Long Li <longli@microsoft.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic
2024-04-06 14:23 ` [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic Erick Archer
2024-04-08 19:42 ` Long Li
@ 2024-04-08 21:38 ` Justin Stitt
1 sibling, 0 replies; 9+ messages in thread
From: Justin Stitt @ 2024-04-08 21:38 UTC (permalink / raw)
To: Erick Archer
Cc: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov, linux-rdma,
linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
Hi,
On Sat, Apr 06, 2024 at 04:23:36PM +0200, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
> and this structure ends in a flexible array:
>
> struct mana_cfg_rx_steer_req_v2 {
> [...]
> mana_handle_t indir_tab[] __counted_by(num_indir_entries);
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + size * count" in
> the kzalloc() function.
>
> Moreover, use the "offsetof" helper to get the indirect table offset
> instead of the "sizeof" operator and avoid the open-coded arithmetic in
> pointers using the new flex member. This new structure member also allow
> us to remove the "req_indir_tab" variable since it is no longer needed.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
Reviewed-by: Justin Stitt <justinstitt@google.com>
> ---
> drivers/infiniband/hw/mana/qp.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index ef0a6dc664d0..4cd8f8afe80d 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -15,15 +15,13 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
> struct mana_port_context *mpc = netdev_priv(ndev);
> struct mana_cfg_rx_steer_req_v2 *req;
> struct mana_cfg_rx_steer_resp resp = {};
> - mana_handle_t *req_indir_tab;
> struct gdma_context *gc;
> u32 req_buf_size;
> int i, err;
>
> gc = mdev_to_gc(dev);
>
> - req_buf_size =
> - sizeof(*req) + sizeof(mana_handle_t) * MANA_INDIRECT_TABLE_SIZE;
> + req_buf_size = struct_size(req, indir_tab, MANA_INDIRECT_TABLE_SIZE);
> req = kzalloc(req_buf_size, GFP_KERNEL);
> if (!req)
> return -ENOMEM;
> @@ -44,20 +42,20 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
> req->rss_enable = true;
>
> req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
> - req->indir_tab_offset = sizeof(*req);
> + req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
> + indir_tab);
> req->update_indir_tab = true;
> req->cqe_coalescing_enable = 1;
>
> - req_indir_tab = (mana_handle_t *)(req + 1);
> /* The ind table passed to the hardware must have
> * MANA_INDIRECT_TABLE_SIZE entries. Adjust the verb
> * ind_table to MANA_INDIRECT_TABLE_SIZE if required
> */
> ibdev_dbg(&dev->ib_dev, "ind table size %u\n", 1 << log_ind_tbl_size);
> for (i = 0; i < MANA_INDIRECT_TABLE_SIZE; i++) {
> - req_indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
> + req->indir_tab[i] = ind_table[i % (1 << log_ind_tbl_size)];
> ibdev_dbg(&dev->ib_dev, "index %u handle 0x%llx\n", i,
> - req_indir_tab[i]);
> + req->indir_tab[i]);
> }
>
> req->update_hashkey = true;
> --
> 2.25.1
>
Thanks
Justin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] net: mana: Avoid open coded arithmetic
[not found] <20240406142337.16241-1-erick.archer@outlook.com>
2024-04-06 14:23 ` [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 Erick Archer
2024-04-06 14:23 ` [PATCH v3 2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic Erick Archer
@ 2024-04-06 14:23 ` Erick Archer
2024-04-08 21:45 ` Justin Stitt
2 siblings, 1 reply; 9+ messages in thread
From: Erick Archer @ 2024-04-06 14:23 UTC (permalink / raw)
To: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov
Cc: Erick Archer, linux-rdma, linux-hyperv, netdev, linux-kernel,
linux-hardening, llvm
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].
As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
and this structure ends in a flexible array:
struct mana_cfg_rx_steer_req_v2 {
[...]
mana_handle_t indir_tab[] __counted_by(num_indir_entries);
};
the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + size * count" in
the kzalloc() function.
Moreover, use the "offsetof" helper to get the indirect table offset
instead of the "sizeof" operator and avoid the open-coded arithmetic in
pointers using the new flex member. This new structure member also allow
us to remove the "req_indir_tab" variable since it is no longer needed.
Now, it is also possible to use the "flex_array_size" helper to compute
the size of these trailing elements in the "memcpy" function.
This way, the code is more readable and safer.
This code was detected with the help of Coccinelle, and audited and
modified manually.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
Link: https://github.com/KSPP/linux/issues/160 [2]
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index d8af5e7e15b4..f2fae659bf3b 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1058,11 +1058,10 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
struct mana_cfg_rx_steer_req_v2 *req;
struct mana_cfg_rx_steer_resp resp = {};
struct net_device *ndev = apc->ndev;
- mana_handle_t *req_indir_tab;
u32 req_buf_size;
int err;
- req_buf_size = sizeof(*req) + sizeof(mana_handle_t) * num_entries;
+ req_buf_size = struct_size(req, indir_tab, num_entries);
req = kzalloc(req_buf_size, GFP_KERNEL);
if (!req)
return -ENOMEM;
@@ -1074,7 +1073,8 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
req->vport = apc->port_handle;
req->num_indir_entries = num_entries;
- req->indir_tab_offset = sizeof(*req);
+ req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
+ indir_tab);
req->rx_enable = rx;
req->rss_enable = apc->rss_state;
req->update_default_rxobj = update_default_rxobj;
@@ -1086,11 +1086,9 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
if (update_key)
memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
- if (update_tab) {
- req_indir_tab = (mana_handle_t *)(req + 1);
- memcpy(req_indir_tab, apc->rxobj_table,
- req->num_indir_entries * sizeof(mana_handle_t));
- }
+ if (update_tab)
+ memcpy(req->indir_tab, apc->rxobj_table,
+ flex_array_size(req, indir_tab, req->num_indir_entries));
err = mana_send_request(apc->ac, req, req_buf_size, &resp,
sizeof(resp));
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] net: mana: Avoid open coded arithmetic
2024-04-06 14:23 ` [PATCH v3 3/3] net: mana: Avoid " Erick Archer
@ 2024-04-08 21:45 ` Justin Stitt
0 siblings, 0 replies; 9+ messages in thread
From: Justin Stitt @ 2024-04-08 21:45 UTC (permalink / raw)
To: Erick Archer
Cc: Long Li, Ajay Sharma, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Jason Gunthorpe,
Leon Romanovsky, Shradha Gupta, Konstantin Taranov, linux-rdma,
linux-hyperv, netdev, linux-kernel, linux-hardening, llvm
Hi,
On Sat, Apr 06, 2024 at 04:23:37PM +0200, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "req" variable is a pointer to "struct mana_cfg_rx_steer_req_v2"
> and this structure ends in a flexible array:
>
> struct mana_cfg_rx_steer_req_v2 {
> [...]
> mana_handle_t indir_tab[] __counted_by(num_indir_entries);
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + size * count" in
> the kzalloc() function.
>
> Moreover, use the "offsetof" helper to get the indirect table offset
> instead of the "sizeof" operator and avoid the open-coded arithmetic in
> pointers using the new flex member. This new structure member also allow
> us to remove the "req_indir_tab" variable since it is no longer needed.
>
> Now, it is also possible to use the "flex_array_size" helper to compute
> the size of these trailing elements in the "memcpy" function.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
Reviewed-by: Justin Stitt <justinstitt@google.com>
> ---
> drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index d8af5e7e15b4..f2fae659bf3b 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1058,11 +1058,10 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
> struct mana_cfg_rx_steer_req_v2 *req;
> struct mana_cfg_rx_steer_resp resp = {};
> struct net_device *ndev = apc->ndev;
> - mana_handle_t *req_indir_tab;
> u32 req_buf_size;
> int err;
>
> - req_buf_size = sizeof(*req) + sizeof(mana_handle_t) * num_entries;
> + req_buf_size = struct_size(req, indir_tab, num_entries);
> req = kzalloc(req_buf_size, GFP_KERNEL);
> if (!req)
> return -ENOMEM;
> @@ -1074,7 +1073,8 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
>
> req->vport = apc->port_handle;
> req->num_indir_entries = num_entries;
> - req->indir_tab_offset = sizeof(*req);
> + req->indir_tab_offset = offsetof(struct mana_cfg_rx_steer_req_v2,
> + indir_tab);
> req->rx_enable = rx;
> req->rss_enable = apc->rss_state;
> req->update_default_rxobj = update_default_rxobj;
> @@ -1086,11 +1086,9 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
> if (update_key)
> memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
>
> - if (update_tab) {
> - req_indir_tab = (mana_handle_t *)(req + 1);
> - memcpy(req_indir_tab, apc->rxobj_table,
> - req->num_indir_entries * sizeof(mana_handle_t));
> - }
> + if (update_tab)
> + memcpy(req->indir_tab, apc->rxobj_table,
> + flex_array_size(req, indir_tab, req->num_indir_entries));
>
> err = mana_send_request(apc->ac, req, req_buf_size, &resp,
> sizeof(resp));
> --
> 2.25.1
>
Thanks
Justin
^ permalink raw reply [flat|nested] 9+ messages in thread