linux-kernel - RE: [PATCH] hv_netvsc:Register VF in netvsc_probe if NET_DEVICE

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]

Date: Wed, 31 Jan 2024 20:05:39 +0000
From: Haiyang Zhang <haiyangz@...rosoft.com>
To: Dexuan Cui <decui@...rosoft.com>, Shradha Gupta
	<shradhagupta@...ux.microsoft.com>
CC: KY Srinivasan <kys@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Wojciech Drewek
	<wojciech.drewek@...el.com>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Shradha Gupta <shradhagupta@...rosoft.com>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] hv_netvsc:Register VF in netvsc_probe if
 NET_DEVICE_REGISTER missed



> -----Original Message-----
> From: Dexuan Cui <decui@...rosoft.com>
> Sent: Wednesday, January 31, 2024 12:40 PM
> To: Haiyang Zhang <haiyangz@...rosoft.com>; Shradha Gupta
> <shradhagupta@...ux.microsoft.com>
> Cc: KY Srinivasan <kys@...rosoft.com>; Wei Liu <wei.liu@...nel.org>;
> David S. Miller <davem@...emloft.net>; Eric Dumazet
> <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Wojciech Drewek <wojciech.drewek@...el.com>; linux-
> hyperv@...r.kernel.org; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; Shradha Gupta <shradhagupta@...rosoft.com>;
> stable@...r.kernel.org
> Subject: RE: [PATCH] hv_netvsc:Register VF in netvsc_probe if
> NET_DEVICE_REGISTER missed
> 
> > From: Haiyang Zhang <haiyangz@...rosoft.com>
> > Sent: Wednesday, January 31, 2024 8:46 AM
> >  [...]
> > > From: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> > > Sent: Wednesday, January 31, 2024 2:54 AM
> > > > [...]
> > > > > +		netvsc_prepare_bonding(vf_netdev);
> > > > > +		netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);
> > > > > +		__netvsc_vf_setup(net, vf_netdev);
> > > >
> > > > add a "break;' ?
> > > With MANA devices and multiport support there, the individual ports
> are
> > > also net_devices.
> > > Wouldn't this be needed for such scenario(where we have multiple mana
> > > port net devices) to
> > > register them all?
> >
> > Each device has separate probe() call, so only one VF will match in one
> > netvsc_probe().
> >
> > netvsc_prepare_bonding() &  netvsc_register_vf() have
> > get_netvsc_byslot(vf_netdev), but __netvsc_vf_setup() doesn't have. So,
> > in case of multi-Vfs, this code will run "this" netvsc NIC with
> multiple VFs by
> > __netvsc_vf_setup() which isn't correct.
> >
> > You need to add the following lines before
> > netvsc_prepare_bonding(vf_netdev)
> > in netvsc_probe() to skip non-matching VFs:
> >
> > if (net != get_netvsc_byslot(vf_netdev))
> > 	continue;
> 
> Haiyang is correct.
> I think it's still good to add a "break;", e.g. my understanding is
> something
> like the below (this is untested):
> 
> +static struct net_device *get_matching_netvsc_dev(net_device
> *event_ndev)
> +{
> +       /* Skip NetVSC interfaces */
> +       if (event_ndev->netdev_ops == &device_ops)
> +               return NULL;
> +
> +       /* Avoid non-Ethernet type devices */
> +       if (event_ndev->type != ARPHRD_ETHER)
> +               return NULL;
> +
> +       /* Avoid Vlan dev with same MAC registering as VF */
> +       if (is_vlan_dev(event_ndev))
> +               return NULL;
> +
> +       /* Avoid Bonding master dev with same MAC registering as VF */
> +       if (netif_is_bond_master(event_ndev))
> +               return NULL;
> +
> +       return get_netvsc_byslot(event_ndev);
> +}

Looks good. 
But, like you said before, the four if's can be moved into a new function,
and shared by two callers: netvsc_probe() & netvsc_netdev_event().


> 
> +	for_each_netdev(dev_net(net), vf_netdev) {
> + 		if (get_matching_netvsc_dev(event_dev) != net)
> +			continue;
> +
> +		netvsc_prepare_bonding(vf_netdev);
> +		netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);
> +		__netvsc_vf_setup(net, vf_netdev);
> +
> +		break;
> +	}
> 
> We can also use get_matching_netvsc_dev() in netvsc_netdev_event().

netvsc_netdev_event() >> netvsc_unregister_vf() uses get_netvsc_byref(vf_netdev)
instead of get_netvsc_byslot().
So probably just re-factoring the four if's is better.

Thanks,
-Haiyang