2024-04-06 15:04:34

by Erick Archer

[permalink] [raw]
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.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 <[email protected]>
---
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



2024-04-08 19:57:11

by Long Li

[permalink] [raw]
Subject: RE: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2

> 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 <[email protected]>

Reviewed-by: Long Li <[email protected]>


2024-04-08 21:35:23

by Justin Stitt

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2

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 <[email protected]>

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 <[email protected]>

> ---
> 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

2024-04-08 21:43:32

by Justin Stitt

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2

On Mon, Apr 8, 2024 at 2:35 PM Justin Stitt <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
> > ---
> > 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