netdev - RE: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for Multi Vports on Bare metal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <MN0PR21MB3437196CFAF4574776862F6ACA99A@MN0PR21MB3437.namprd21.prod.outlook.com>
Date: Thu, 22 May 2025 14:51:11 +0000
From: Haiyang Zhang <haiyangz@...rosoft.com>
To: Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Dexuan Cui
	<decui@...rosoft.com>, "stephen@...workplumber.org"
	<stephen@...workplumber.org>, KY Srinivasan <kys@...rosoft.com>, Paul
 Rosswurm <paulros@...rosoft.com>, "olaf@...fle.de" <olaf@...fle.de>,
	"vkuznets@...hat.com" <vkuznets@...hat.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "leon@...nel.org" <leon@...nel.org>, Long Li
	<longli@...rosoft.com>, "ssengar@...ux.microsoft.com"
	<ssengar@...ux.microsoft.com>, "linux-rdma@...r.kernel.org"
	<linux-rdma@...r.kernel.org>, "daniel@...earbox.net" <daniel@...earbox.net>,
	"john.fastabend@...il.com" <john.fastabend@...il.com>, "bpf@...r.kernel.org"
	<bpf@...r.kernel.org>, "ast@...nel.org" <ast@...nel.org>, "hawk@...nel.org"
	<hawk@...nel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"shradhagupta@...ux.microsoft.com" <shradhagupta@...ux.microsoft.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, Konstantin Taranov
	<kotaranov@...rosoft.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for
 Multi Vports on Bare metal



> -----Original Message-----
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Thursday, May 22, 2025 9:44 AM
> To: Simon Horman <horms@...nel.org>; Haiyang Zhang
> <haiyangz@...rosoft.com>

> >>>>  static int mana_query_device_cfg(struct mana_context *ac, u32
> >>> proto_major_ver,
> >>>>  				 u32 proto_minor_ver, u32 proto_micro_ver,
> >>>> -				 u16 *max_num_vports)
> >>>> +				 u16 *max_num_vports, u8 *bm_hostmode)
> >>>>  {
> >>>>  	struct gdma_context *gc = ac->gdma_dev->gdma_context;
> >>>>  	struct mana_query_device_cfg_resp resp = {};
> >>>> @@ -932,7 +932,7 @@ static int mana_query_device_cfg(struct
> mana_context
> >>> *ac, u32 proto_major_ver,
> >>>>  	mana_gd_init_req_hdr(&req.hdr, MANA_QUERY_DEV_CONFIG,
> >>>>  			     sizeof(req), sizeof(resp));
> >>>>
> >>>> -	req.hdr.resp.msg_version = GDMA_MESSAGE_V2;
> >>>> +	req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
> >>>>
> >>>>  	req.proto_major_ver = proto_major_ver;
> >>>>  	req.proto_minor_ver = proto_minor_ver;
> >>>
> >>>> @@ -956,11 +956,16 @@ static int mana_query_device_cfg(struct
> >>> mana_context *ac, u32 proto_major_ver,
> >>>>
> >>>>  	*max_num_vports = resp.max_num_vports;
> >>>>
> >>>> -	if (resp.hdr.response.msg_version == GDMA_MESSAGE_V2)
> >>>> +	if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V2)
> >>>>  		gc->adapter_mtu = resp.adapter_mtu;
> >>>>  	else
> >>>>  		gc->adapter_mtu = ETH_FRAME_LEN;
> >>>>
> >>>> +	if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
> >>>> +		*bm_hostmode = resp.bm_hostmode;
> >>>> +	else
> >>>> +		*bm_hostmode = 0;
> >>>
> >>> Hi,
> >>>
> >>> Perhaps not strictly related to this patch, but I see
> >>> that mana_verify_resp_hdr() is called a few lines above.
> >>> And that verifies a minimum msg_version. But I do not see
> >>> any verification of the maximum msg_version supported by the code.
> >>>
> >>> I am concerned about a hypothetical scenario where, say the as yet
> unknown
> >>> version 5 is sent as the version, and the above behaviour is used,
> while
> >>> not being correct.
> >>>
> >>> Could you shed some light on this?
> >>>
> >>
> >> In driver, we specify the expected reply msg version is v3 here:
> >> req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
> >>
> >> If the HW side is upgraded, it won't send reply msg version higher
> >> than expected, which may break the driver.
> >
> > Thanks,
> >
> > If I understand things correctly the HW side will honour the
> > req.hdr.resp.msg_version and thus the SW won't receive anything
> > it doesn't expect. Is that right?
> 
> @Haiyang, if Simon's interpretation is correct, please change the
> version checking in the driver from:
> 
> 	if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
> 
> to
> 	if (resp.hdr.response.msg_version == GDMA_MESSAGE_V3)
> 
> As the current code is misleading.

Simon:
Yes, you are right. So newer HW can support older driver, and vice
versa.

Paolo:
The MANA protocol doesn't remove any existing fields during upgrades.

So (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3) will continue
to work in the future. If we change it to 
(resp.hdr.response.msg_version == GDMA_MESSAGE_V3), 
we will have to remember to update it to something like:
(resp.hdr.response.msg_version >= GDMA_MESSAGE_V3 &&
 resp.hdr.response.msg_version <= GDMA_MESSAGE_V5), 
if the version is upgraded to v5 in the future. And keep on updating
the checks on existing fields every time when the version is
upgraded.

So, can I keep the ">=" condition, to avoid future bug if anyone
forget to update checks on all existing fields?

Thanks,
- Haiyang