[PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[Haiyang Zhang]

The existing code moves VF to the same namespace as the synthetic device
during netvsc_register_vf(). But, if the synthetic device is moved to a
new namespace after the VF registration, the VF won't be moved together.

To make the behavior more consistent, add a namespace check to netvsc_open(),
and move the VF if it is not in the same namespace.

Cc: stable@vger.kernel.org
Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 153b97f8ec0d..9caade092524 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -134,6 +134,19 @@ static int netvsc_open(struct net_device *net)
 	}
 
 	if (vf_netdev) {
+		if (!net_eq(dev_net(net), dev_net(vf_netdev))) {
+			ret = dev_change_net_namespace(vf_netdev, dev_net(net),
+						       "eth%d");
+			if (ret)
+				netdev_err(vf_netdev,
+					"Cannot move to same namespace as %s: %d\n",
+					net->name, ret);
+			else
+				netdev_info(vf_netdev,
+					"Moved VF to namespace with: %s\n",
+					net->name);
+		}
+
 		/* Setting synthetic device up transparently sets
 		 * slave as up. If open fails, then slave will be
 		 * still be offline (and not used).
-- 
2.34.1

Re: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[Paolo Abeni]

On 9/27/24 22:54, Haiyang Zhang wrote:
> The existing code moves VF to the same namespace as the synthetic device
> during netvsc_register_vf(). But, if the synthetic device is moved to a
> new namespace after the VF registration, the VF won't be moved together.
> 
> To make the behavior more consistent, add a namespace check to netvsc_open(),
> and move the VF if it is not in the same namespace.
> 
> Cc: stable@vger.kernel.org
> Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

This looks strange to me. Skimming over the code it looks like that with 
VF you really don't mean a Virtual Function...

Looking at the blamed commit, it looks like that having both the 
synthetic and the "VF" device in different namespaces is an intended 
use-case. This change would make such scenario more difficult and could 
possibly break existing use-cases.

Why do you think it will be more consistent? If the user moved the 
synthetic device in another netns, possibly/likely the user intended to 
keep both devices separated.

Thanks,

Paolo

RE: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[Haiyang Zhang]

> -----Original Message-----
> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Thursday, October 3, 2024 5:35 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; edumazet@google.com; kuba@kernel.org;
> stephen@networkplumber.org; davem@davemloft.net; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open
> 
> On 9/27/24 22:54, Haiyang Zhang wrote:
> > The existing code moves VF to the same namespace as the synthetic
> device
> > during netvsc_register_vf(). But, if the synthetic device is moved to a
> > new namespace after the VF registration, the VF won't be moved
> together.
> >
> > To make the behavior more consistent, add a namespace check to
> netvsc_open(),
> > and move the VF if it is not in the same namespace.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc
> device")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> This looks strange to me. Skimming over the code it looks like that with
> VF you really don't mean a Virtual Function...

Thanks for the review.
"VF": I mean "Virtual Function" NIC.

> Looking at the blamed commit, it looks like that having both the
> synthetic and the "VF" device in different namespaces is an intended
> use-case. This change would make such scenario more difficult and could
> possibly break existing use-cases.

On Hyper-V / Azure hosts, the synthetic NIC (master) and VF NIC (slave)
are transparently bonded, and apps should only interact with the
synthetic NIC (master). Using them at two different namespaces is not
an intended use case.

We have published documents explaining this:
"The synthetic and VF interfaces have the same MAC address. Together, 
they constitute a single NIC from the standpoint of other network entities 
that exchange packets with the virtual NIC in the VM. "
"IP addresses are assigned only to the synthetic interface."
https://learn.microsoft.com/en-us/azure/virtual-network/accelerated-networking-how-it-works
 
> Why do you think it will be more consistent? If the user moved the
> synthetic device in another netns, possibly/likely the user intended to
> keep both devices separated.

Consider two Cases:
Case 1):
	- Synthetic NIC is offered.
	- Run command to move synthetic NIC
		ip link set <synthetic NIC> netns <new namespace>
	- VF NIC is offered.

Case 2):
	- Synthetic NIC is offered.
	- VF NIC is offered.
	- Run command to move synthetic NIC
		ip link set <synthetic NIC> netns <new namespace>

The previous patch: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device")
automatically moves the VF to the new namespace in Case (1), but not
in Case (2).

With this patch, VF will be automatically moved to the new namespace
also in the Case (2). So, the behaviors of Case 1 & 2 become consistent.
This will make our customers easier to find and check if VF NIC is
running, and its stat data.

Thanks,
- Haiyang

Re: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[Stephen Hemminger]

On Thu, 3 Oct 2024 11:34:49 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> On 9/27/24 22:54, Haiyang Zhang wrote:
> > The existing code moves VF to the same namespace as the synthetic device
> > during netvsc_register_vf(). But, if the synthetic device is moved to a
> > new namespace after the VF registration, the VF won't be moved together.
> > 
> > To make the behavior more consistent, add a namespace check to netvsc_open(),
> > and move the VF if it is not in the same namespace.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc device")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>  
> 
> This looks strange to me. Skimming over the code it looks like that with 
> VF you really don't mean a Virtual Function...

In Hyper-V/Azure, there is a feature called "Accelerated Networking" where
a Virtual Function (VF) is associated with the synthetic network interface.
The VF may be added/removed by hypervisor while network is running and driver
needs to follow and track that.

> 
> Looking at the blamed commit, it looks like that having both the 
> synthetic and the "VF" device in different namespaces is an intended 
> use-case. This change would make such scenario more difficult and could 
> possibly break existing use-cases.

That commit was trying to solve the case where a network interface
was isolated at boot. The VF device shows up after the
synthetic device has been registered.


> Why do you think it will be more consistent? If the user moved the 
> synthetic device in another netns, possibly/likely the user intended to 
> keep both devices separated.

Splitting the two across namespaces is not useful. The VF is a secondary
device and doing anything directly on the VF will not give good results.
Linux does not have a way to hide or lock out network devices, if it did
the VF would be so marked.

This patch is trying to handle the case where userspace moves
the synthetic network device and the VF is left in wrong namespace.

Moving the device when brought up is not the best solution. Probably better to
do it when the network device is moved to another namespace.
Which is visible in driver as NETDEV_REGISTER event.
The driver already handles this (for VF events) in netvsc_netdev_event()
it would just have to look at events on the netvsc device as well.

RE: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open

[Haiyang Zhang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, October 3, 2024 11:49 AM
> To: Paolo Abeni <pabeni@redhat.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> wei.liu@kernel.org; Dexuan Cui <decui@microsoft.com>;
> edumazet@google.com; kuba@kernel.org; davem@davemloft.net; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH net] hv_netvsc: Fix VF namespace also in netvsc_open
> 
> On Thu, 3 Oct 2024 11:34:49 +0200
> Paolo Abeni <pabeni@redhat.com> wrote:
> 
> > On 9/27/24 22:54, Haiyang Zhang wrote:
> > > The existing code moves VF to the same namespace as the synthetic
> device
> > > during netvsc_register_vf(). But, if the synthetic device is moved to
> a
> > > new namespace after the VF registration, the VF won't be moved
> together.
> > >
> > > To make the behavior more consistent, add a namespace check to
> netvsc_open(),
> > > and move the VF if it is not in the same namespace.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: c0a41b887ce6 ("hv_netvsc: move VF to same namespace as netvsc
> device")
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > This looks strange to me. Skimming over the code it looks like that
> with
> > VF you really don't mean a Virtual Function...
> 
> In Hyper-V/Azure, there is a feature called "Accelerated Networking"
> where
> a Virtual Function (VF) is associated with the synthetic network
> interface.
> The VF may be added/removed by hypervisor while network is running and
> driver
> needs to follow and track that.
> 
> >
> > Looking at the blamed commit, it looks like that having both the
> > synthetic and the "VF" device in different namespaces is an intended
> > use-case. This change would make such scenario more difficult and could
> > possibly break existing use-cases.
> 
> That commit was trying to solve the case where a network interface
> was isolated at boot. The VF device shows up after the
> synthetic device has been registered.
> 
> 
> > Why do you think it will be more consistent? If the user moved the
> > synthetic device in another netns, possibly/likely the user intended to
> > keep both devices separated.
> 
> Splitting the two across namespaces is not useful. The VF is a secondary
> device and doing anything directly on the VF will not give good results.
> Linux does not have a way to hide or lock out network devices, if it did
> the VF would be so marked.
> 
> This patch is trying to handle the case where userspace moves
> the synthetic network device and the VF is left in wrong namespace.
> 
> Moving the device when brought up is not the best solution. Probably
> better to
> do it when the network device is moved to another namespace.
> Which is visible in driver as NETDEV_REGISTER event.
> The driver already handles this (for VF events) in netvsc_netdev_event()
> it would just have to look at events on the netvsc device as well.
Thank you for the suggestion. I will look into this idea: let the netvsc_netdev_event() 
to monitor the NETDEV_REGISTER from netvsc devices.
This will come from __dev_change_net_namespace -> call_netdevice_notifiers(NETDEV_REGISTER, dev).

- Haiyang