Received-SPF: pass (google.com: domain of linux-kernel+bounces-126756-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 1 Apr 2024 16:47:40 +0200
From: Erick Archer <erick.archer@outlook.com>
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Erick Archer <erick.archer@outlook.com>, Long Li <longli@microsoft.com>,
	Ajay Sharma <sharmaajay@microsoft.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, Leon Romanovsky <leon@kernel.org>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Shradha Gupta <shradhagupta@linux.microsoft.com>,
	Konstantin Taranov <kotaranov@microsoft.com>,
	Kees Cook <keescook@chromium.org>, linux-rdma@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
	netdev@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] RDMA/mana_ib: Add flex array to struct
 mana_cfg_rx_steer_req_v2
Message-ID:
 <AS8PR02MB72377480DEF6222736BF72AE8B3F2@AS8PR02MB7237.eurprd02.prod.outlook.com>
References: <AS8PR02MB7237974EF1B9BAFA618166C38B382@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <1be10e9c-e521-4974-b2ed-808a2e3a1a9d@embeddedor.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1be10e9c-e521-4974-b2ed-808a2e3a1a9d@embeddedor.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: AS8PR02MB7237:EE_|VI1PR02MB6096:EE_
X-MS-Office365-Filtering-Correlation-Id: 01ebd0ac-8329-409b-a8c5-08dc525ab50e
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	tvEMghliWOOj9OKltKDKXiEz6xku36p2ZIo0eDLDBm0unkSyilvVuJzHrqnJTorLF7xUmtD2mnflJQvAvLGClRmkaW/R/quG/0YpPzoyPoIgj5GlPuWP1G7z9IEJdVvtcJUNQqGOeXEb4Ir++unb0vNBxMx9iyl/Q4Gfu11+473iKlvqGo3cXlAmYpXeRKHNwBARaxLoZ4Yj0V/98o8OLHl/id7Q56EzvMxf6pWx2Ct9tzgixznGFNuYUmpdTqdDa34QzHEWiOvtvwcBWpC8rIpLqjyStNo7uhJ2vE/7S1YCavQIRBc0Z/5yHXTWc6e8NiNtB/DBSFSJlbt+m/Hyn7rRMBd+Kmadk405oAxMs6cHMOsG9zxdL8pFnYhinmGk5Cu29p5Xqa/R8pNK2bPeB1fCtY7KtktYIzI8RznShSVaxDNiwb///0RSPJZDJ9Xva9/HM0krYx6UEfii1giUZPo8Viri7a1o+R/s6q6INHNDpIwHEU1KcQ/V6as8QGQBLdS8GLCxNgAn5gdsfx3kqAL9rfXGhPzTgEpT2i5Xw0JYB4QBPxvw2pNl8JSbisHFXx+MrWqx41yjm6fnj47AxAoe2aBqrgitoJEEuYQ8ogKL/W3H/+Guz3ToTTOi3/j8GV9m39SeHxoZemU7nfqqU9cMbeYbQp4dMqR1BEc0UOsaqyziSEmuR/JOdMsR2Spu
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?eNx3sH1829DKsdOreYhrpU/HscpM1pPmhXlaC8Mld6AwG1IYbm7kXfHP1Ic4?=
 =?us-ascii?Q?+FvCfdeSjmTAqJMnk6cejmSceQkpPIOvtS5PGdGUNso3Kz8INENGTgAFmB9N?=
 =?us-ascii?Q?tb11Kdg9r2Vuv9uHvP/Ww91ZjYxOkJC3ZVbMdUUk01WLlOm3TYcSD3DRrY3l?=
 =?us-ascii?Q?MTvBTrNXUEFEkVebBpcj0Fh+WpRYIhQXeBuqsWfNC02vtG82Q0cr1ugD/ZGY?=
 =?us-ascii?Q?VpVg4Rw/BucVmiFT/cPHkYDsdN+CqdXPHToCGbnAxSqTF6dzSIDJlw94axTN?=
 =?us-ascii?Q?g/3m33zB9RILgHhcclKMyPZKls8RVn+myzjPo0xPH+hdyWHu7BVlkbbuew2P?=
 =?us-ascii?Q?AJWopAXnQIl3PvLsW4Ts6VXKutUTyq2WTBQLdYpI2NeFY4g57dxUp8czeZXh?=
 =?us-ascii?Q?4LEfSLQN9l6tjMpNS0KkMtol/c9wpWfYz28sgFGJ1Yay5BYgeeiSE9QOzfx6?=
 =?us-ascii?Q?GYWslCqJj7dRXa/2oD/WFcKwBgqlRADGvzu8ViLSBK94DnhOTHWxrlmR1tk9?=
 =?us-ascii?Q?vrlraUjZv/kyxHAERnTqH9cjGLXua7I8Dmy7SyDbaCsXvCesdh2Aad+DBrEo?=
 =?us-ascii?Q?Wb2I7xJ62+KXv29FQ1HuFelZaal/T6qxGrLOYFHQpvYqTdwR2LPep05kSaV7?=
 =?us-ascii?Q?BWPweTVHeGPeUDkA0epTYrHaZwOy2WAtI3cknG3E0IlhLOheqLKkIoFi0N2P?=
 =?us-ascii?Q?yEX3IdsxUTtR7cmSlwaWFn3Mt+qBL5PySgWAHBpMsmQtRVlyw5By4Cm9arRs?=
 =?us-ascii?Q?K5tEQ21DthBQtsODRo2XAZofWCCvoOjlVVAeU5gn0nkQseiXGHwfG2SvkyHC?=
 =?us-ascii?Q?RXYcWML/y9UmdVU6e3ZVg37WkZliqWH5YytTVwWAPwssc3kK1QgslZINxMQw?=
 =?us-ascii?Q?zERt715pgrxDWELMOKXqKkM5VeF/vOjEhTWykCC1cc9WgJ21b69bod0oJMiP?=
 =?us-ascii?Q?MyxSfZaAua6sqDfmDOuBS0PcldIXjEOmvCVd/O6qVm5s5BfMGnHY+A6p6hsr?=
 =?us-ascii?Q?CYHU+CwovhrdVd3/KEWCYeT5/5QRu1tcXYYyosdP+VPiy0Q08cuMjtulKoLs?=
 =?us-ascii?Q?bOxGF55TImcxYtM4QM19RpYtqTncCp82SLPP4M0sAzXPv1hJhOtI3+sKH2OV?=
 =?us-ascii?Q?GrTCXCoGbmv88rbczhHZWwr2oYX7nB4PcEykhOYV4Cj3/f7MVGhCdcKA/gSL?=
 =?us-ascii?Q?xTQCOS1Xf2tCCT0Y9+yX9fRrsw4mIq/K23QB+Qt8z+HkPhbSvrrHDOEUbiAi?=
 =?us-ascii?Q?0FvrI46z9NfghMbn5wmn?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 01ebd0ac-8329-409b-a8c5-08dc525ab50e
X-MS-Exchange-CrossTenant-AuthSource: AS8PR02MB7237.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 14:47:51.9632
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR02MB6096

Hi Gustavo,

On Sun, Mar 31, 2024 at 08:53:07PM -0600, Gustavo A. R. Silva wrote:
> 
> 
> On 31/03/24 09:04, 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].
> > 
> > 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.
> > 
> > Now, it is also possible to use the "flex_array_size" helper to compute
> > the size of these trailing elements in the "memcpy" function.
> > 
> > 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>
> > ---
> >   drivers/infiniband/hw/mana/qp.c               | 8 ++++----
> >   drivers/net/ethernet/microsoft/mana/mana_en.c | 9 +++++----
> >   include/net/mana/mana.h                       | 1 +
> >   3 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> > index 6e7627745c95..c2a39db8ef92 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -22,8 +22,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
> >   	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,11 +43,12 @@ 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);
> > +	req_indir_tab = req->indir_tab;
> 
> It seems that `req_indir_tab` can be removed, and `req->indir_tab` be directly used.

It seems reasonable to me. Thanks for looking at this.

Regards,
Erick

> 
> >   	/* 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
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 59287c6e6cee..04aa096c6cc4 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -1062,7 +1062,7 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
> >   	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 +1074,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;
> > @@ -1087,9 +1088,9 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
> >   		memcpy(&req->hashkey, apc->hashkey, MANA_HASH_KEY_SIZE);
> >   	if (update_tab) {
> > -		req_indir_tab = (mana_handle_t *)(req + 1);
> > +		req_indir_tab = req->indir_tab;
> 
> Ditto.
> 
> Thanks
> --
> Gustavo
> 
> >   		memcpy(req_indir_tab, apc->rxobj_table,
> > -		       req->num_indir_entries * sizeof(mana_handle_t));
> > +		       flex_array_size(req, indir_tab, req->num_indir_entries));
> >   	}
> >   	err = mana_send_request(apc->ac, req, req_buf_size, &resp,
> > diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> > index 76147feb0d10..20ffcae29e1e 100644
> > --- a/include/net/mana/mana.h
> > +++ b/include/net/mana/mana.h
> > @@ -671,6 +671,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[];
> >   }; /* HW DATA */
> >   struct mana_cfg_rx_steer_resp {