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

Message-ID:
 <SA1PR21MB1335C5554F769454AAEDE1C8BF7D2@SA1PR21MB1335.namprd21.prod.outlook.com>
Date: Tue, 30 Jan 2024 20:13:21 +0000
From: Dexuan Cui <decui@...rosoft.com>
To: Shradha Gupta <shradhagupta@...ux.microsoft.com>, KY Srinivasan
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...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>
CC: 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

> From: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> Sent: Monday, January 29, 2024 11:19 PM
>  [...]
> If hv_netvsc driver is removed and reloaded, the NET_DEVICE_REGISTER

s/removed/unloaded/
unloaded looks more accurate to me :-)

> [...]
> Tested-on: Ubuntu22
> Testcases: LISA testsuites
> 	   verify_reload_hyperv_modules, perf_tcp_ntttcp_sriov
IMO the 3 lines can be removed: this bug is not specific to Ubuntu, and the
test case names don't provide extra value to help understand the issue
here and they might cause more questions unnecessarily, e.g. what's LISA,
what exactly do the test cases do.

> +/* Macros to define the context of vf registration */
> +#define VF_REG_IN_PROBE		1
> +#define VF_REG_IN_RECV_CBACK	2

I think VF_REG_IN_NOTIFIER is a better name?
RECV_CBALL looks inaccurate to me.

> @@ -2205,8 +2209,11 @@ static int netvsc_vf_join(struct net_device
> *vf_netdev,
>  			   ndev->name, ret);
>  		goto upper_link_failed;
>  	}
> -
> -	schedule_delayed_work(&ndev_ctx->vf_takeover,
> VF_TAKEOVER_INT);
> +	/* If this registration is called from probe context vf_takeover
> +	 * is taken care of later in probe itself.
I suspect "later in probe itself" is not accurate.
If 'context' is VF_REG_IN_PROBE, I suppose what happens here is:
after netvsc_probe() finishes, the netvsc interface becomes available,
so the user space will ifup it, and netvsc_open() will UP the VF interface,
and netvsc_netdev_event() is called for the VF with event ==
NETDEV_POST_INIT (?) and NETDEV_CHANGE, and the data path is
switched to the VF.

If my understanding is correct, I think in the case of 'context' ==
VF_REG_IN_PROBE, I suspect the "Align MTU of VF with master"
and the "sync address list from ndev to VF" in __netvsc_vf_setup() are
omitted? If so, should this be fixed? e.g. Not sure if the below is an issue or not:
1) a VF is bound to a NetVSC interface, and a user sets the MTUs to 1024.
2) rmmod hv_netvsc
3) modprobe hv_netvsc
4) the netvsc interface uses MTU=1500 (the default), and the VF still uses 1024.

> @@ -2597,6 +2604,34 @@ static int netvsc_probe(struct hv_device *dev,
>  	}
> 
>  	list_add(&net_device_ctx->list, &netvsc_dev_list);
> +
> +	/* When the hv_netvsc driver is removed and readded, the

s/removed and readded/unloaded and reloaded/

> +	 * NET_DEVICE_REGISTER for the vf device is replayed before
> probe
> +	 * is complete. This is because register_netdevice_notifier() gets
> +	 * registered before vmbus_driver_register() so that callback func
> +	 * is set before probe and we don't miss events like
> NETDEV_POST_INIT
> +	 * So, in this section we try to register each matching

Looks like there are spaces at the end of the line. We can move up a few words
from the next line :-)

s/each matching/the matching/
A NetVSC interface has only 1 matching VF device.

> +	 * vf device that is present as a netdevice, knowing that it's register

s/it's/its/

> +	 * call is not processed in the netvsc_netdev_notifier(as probing is
> +	 * progress and get_netvsc_byslot fails).
> +	 */
> +	for_each_netdev(dev_net(net), vf_netdev) {
> +		if (vf_netdev->netdev_ops == &device_ops)
> +			continue;
> +
> +		if (vf_netdev->type != ARPHRD_ETHER)
> +			continue;
> +
> +		if (is_vlan_dev(vf_netdev))
> +			continue;
> +
> +		if (netif_is_bond_master(vf_netdev))
> +			continue;

The code above is duplicated from netvsc_netdev_event().
Can we add a new helper function is_matching_vf() to avoid the duplication?

> +		netvsc_prepare_bonding(vf_netdev);
> +		netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);
> +		__netvsc_vf_setup(net, vf_netdev);

add a "break;' ?

> +	}
>  	rtnl_unlock();