| Message ID | AS8PR02MB72374BD1B23728F2E3C3B1A18B022@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2 | expand |
On Sat, Apr 06, 2024 at 04:23:34PM +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). > > Also, avoid the open-coded arithmetic in the memory allocator functions > [2] using the "struct_size" macro. > > 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. > > Specifically, the first commit adds the flex member and the patches 2 and > 3 refactor the consumers of the "struct mana_cfg_rx_steer_req_v2". > > This code was detected with the help of Coccinelle, and audited and > modified manually. The Coccinelle script used to detect this code pattern > is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) + sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > 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> > --- > Changes in v3: > - Split the changes in various commits to simplify the acceptance process > (Leon Romanovsky). > > Changes in v2: > - Remove the "req_indir_tab" variable (Gustavo A. R. Silva). > - Update the commit message. > - Add the "__counted_by" attribute. > > Previous versions: > v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237974EF1B9BAFA618166C38B382@AS8PR02MB7237.eurprd02.prod.outlook.com/ > v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB723729C5A63F24C312FC9CD18B3F2@AS8PR02MB7237.eurprd02.prod.outlook.com/ > --- > Erick Archer (3): > net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 > RDMA/mana_ib: Prefer struct_size over open coded arithmetic > net: mana: Avoid open coded arithmetic Unfortunately, I still can't take RDMA patch alone without the netdev patches. Jakub, do you want shared branch for this series or should I take everything through RDMA tree as netdev part is small enough? Thanks
On Mon, 8 Apr 2024 14:07:30 +0300 Leon Romanovsky wrote: > Jakub, do you want shared branch for this series or should I take > everything through RDMA tree as netdev part is small enough? Shared branch would be good. Ed has some outstanding patches to refactor the ethtool RSS API.
On Mon, Apr 08, 2024 at 06:36:57PM -0700, Jakub Kicinski wrote: > On Mon, 8 Apr 2024 14:07:30 +0300 Leon Romanovsky wrote: > > Jakub, do you want shared branch for this series or should I take > > everything through RDMA tree as netdev part is small enough? > > Shared branch would be good. Ed has some outstanding patches > to refactor the ethtool RSS API. Great, I'll wait for a day or two to give people time to review and then I'll create a shared branch. Thanks
On 09/04/2024 02:36, Jakub Kicinski wrote: > On Mon, 8 Apr 2024 14:07:30 +0300 Leon Romanovsky wrote: >> Jakub, do you want shared branch for this series or should I take >> everything through RDMA tree as netdev part is small enough? > > Shared branch would be good. Ed has some outstanding patches > to refactor the ethtool RSS API. For the record I am extremely unlikely to have time to get those done this cycle :( Though in any case fwiw it doesn't look like this series touches anything that would conflict; mana doesn't appear to support custom RSS contexts and besides the changes are well away from the ethtool API handling. -e
On Tue, 9 Apr 2024 18:01:40 +0100 Edward Cree wrote: > > Shared branch would be good. Ed has some outstanding patches > > to refactor the ethtool RSS API. > > For the record I am extremely unlikely to have time to get those > done this cycle :( > Though in any case fwiw it doesn't look like this series touches > anything that would conflict; mana doesn't appear to support > custom RSS contexts and besides the changes are well away from > the ethtool API handling. Better safe than sorry, since the change applies cleanly on an -rc tag having it applied to both trees should be very little extra work.
On Tue, Apr 09, 2024 at 02:44:19PM -0700, Jakub Kicinski wrote: > On Tue, 9 Apr 2024 18:01:40 +0100 Edward Cree wrote: > > > Shared branch would be good. Ed has some outstanding patches > > > to refactor the ethtool RSS API. > > > > For the record I am extremely unlikely to have time to get those > > done this cycle :( > > Though in any case fwiw it doesn't look like this series touches > > anything that would conflict; mana doesn't appear to support > > custom RSS contexts and besides the changes are well away from > > the ethtool API handling. > > Better safe than sorry, since the change applies cleanly on an -rc tag > having it applied to both trees should be very little extra work. I prepared mana-ib-flex branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=mana-ib-flex and merge ti to our wip branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/leon-for-next&id=e537deecda03e0911e9406095ccd48bd42f328c7 Thanks
On Thu, 11 Apr 2024 13:58:39 +0300 Leon Romanovsky wrote: > I prepared mana-ib-flex branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=mana-ib-flex > and merge ti to our wip branch https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/leon-for-next&id=e537deecda03e0911e9406095ccd48bd42f328c7 Thanks!
Hello: This series was applied to netdev/net-next.git (main) by Leon Romanovsky <leon@kernel.org>: On Sat, 6 Apr 2024 16:23:34 +0200 you 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). > > [...] Here is the summary with links: - [v3,1/3] net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 https://git.kernel.org/netdev/net-next/c/bfec4e18f943 - [v3,2/3] RDMA/mana_ib: Prefer struct_size over open coded arithmetic https://git.kernel.org/netdev/net-next/c/29b8e13a8b4c - [v3,3/3] net: mana: Avoid open coded arithmetic https://git.kernel.org/netdev/net-next/c/a68292eb4316 You are awesome, thank you!
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). Also, avoid the open-coded arithmetic in the memory allocator functions [2] using the "struct_size" macro. 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. Specifically, the first commit adds the flex member and the patches 2 and 3 refactor the consumers of the "struct mana_cfg_rx_steer_req_v2". This code was detected with the help of Coccinelle, and audited and modified manually. The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) + sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) 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> --- Changes in v3: - Split the changes in various commits to simplify the acceptance process (Leon Romanovsky). Changes in v2: - Remove the "req_indir_tab" variable (Gustavo A. R. Silva). - Update the commit message. - Add the "__counted_by" attribute. Previous versions: v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237974EF1B9BAFA618166C38B382@AS8PR02MB7237.eurprd02.prod.outlook.com/ v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB723729C5A63F24C312FC9CD18B3F2@AS8PR02MB7237.eurprd02.prod.outlook.com/ --- Erick Archer (3): net: mana: Add flex array to struct mana_cfg_rx_steer_req_v2 RDMA/mana_ib: Prefer struct_size over open coded arithmetic net: mana: Avoid open coded arithmetic drivers/infiniband/hw/mana/qp.c | 12 +++++------- drivers/net/ethernet/microsoft/mana/mana_en.c | 14 ++++++-------- include/net/mana/mana.h | 1 + 3 files changed, 12 insertions(+), 15 deletions(-)