[net-next,v3] hv_netvsc: Set device flags for properly indicating bonding in Hyper-V

Commit Message

Long Li Feb. 7, 2025, 9:55 p.m. UTC

From: Long Li <[e-mail]>

On Hyper-V platforms, a slave VF netdev always bonds to Netvsc and remains
as Netvsc's only active slave as long as the slave device is present. This
behavior is the same as a bonded device, but it's not user-configurable.

Some kernel APIs (e.g those in "include/linux/netdevice.h") check for
IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used in
a master/slave bonded setup. Netvsc's bonding setup with its slave device
falls into this category.

The security implication of this behavior is the same as if bonding is
configured from the user-mode for the bonding device. It's an invalid
configuration if a user attempts to configure different permissions or
to setup for different containers for netvsc and its slave device, as the
bonding is mandatory for slave device to function under Hyper-V/Azure
environment. The user must assume the kernel will always bond and
configure netvsc and its slave device to the same permission/constraint
as if they are used as a single device.

Make hv_netvsc properly indicate bonding with its slave and change the
relevant kernel API to reflect this bonding setup.

Signed-off-by: Long Li <[e-mail]>
---
Change log
v2: instead of re-using IFF_BOND, introduce permanent_bond in netdev
v3: rename permanent_bond to kernel_bond, add details in commit on security implications

 drivers/net/hyperv/netvsc_drv.c | 11 +++++++++++
 include/linux/netdevice.h       |  9 +++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski Feb. 12, 2025, 12:20 a.m. UTC | #1

On Fri,  7 Feb 2025 13:55:37 -0800 [e-mail] wrote:
> On Hyper-V platforms, a slave VF netdev always bonds to Netvsc and remains
> as Netvsc's only active slave as long as the slave device is present. This
> behavior is the same as a bonded device, but it's not user-configurable.
> 
> Some kernel APIs (e.g those in "include/linux/netdevice.h") check for
> IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used in
> a master/slave bonded setup. Netvsc's bonding setup with its slave device
> falls into this category.

Again, this is way too much of a hack. You're trying to make
netif_is_bond_master() return true for your franken-interfaces
with minimal effort.

Stephen Hemminger Feb. 12, 2025, 12:37 a.m. UTC | #2

On Tue, 11 Feb 2025 16:20:26 -0800
Jakub Kicinski <[e-mail]> wrote:

> On Fri,  7 Feb 2025 13:55:37 -0800 [e-mail] wrote:
> > On Hyper-V platforms, a slave VF netdev always bonds to Netvsc and remains
> > as Netvsc's only active slave as long as the slave device is present. This
> > behavior is the same as a bonded device, but it's not user-configurable.
> > 
> > Some kernel APIs (e.g those in "include/linux/netdevice.h") check for
> > IFF_MASTER, IFF_SLAVE and IFF_BONDING for determing if those are used in
> > a master/slave bonded setup. Netvsc's bonding setup with its slave device
> > falls into this category.  
> 
> Again, this is way too much of a hack. You're trying to make
> netif_is_bond_master() return true for your franken-interfaces
> with minimal effort. 

Agree but disagree as to reasoning.

The way bonding is handled in the kernel internal API's is ad-hoc.
Really a better solution is needed.

The real problem is in any code (other than the bonding driver itself)
looking at IFF_BONDING is broken. All that code won't work if used over team
or failover devices (luckily no one ever seems to use them).

Patch

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d6c4abfc3a28..035869b02fde 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2204,6 +2204,10 @@  static int netvsc_vf_join(struct net_device *vf_netdev,
 		goto rx_handler_failed;
 	}
 
+	vf_netdev->kernel_bond = 1;
+	ndev->kernel_bond = 1;
+	ndev->flags |= IFF_MASTER;
+
 	ret = netdev_master_upper_dev_link(vf_netdev, ndev,
 					   NULL, NULL, NULL);
 	if (ret != 0) {
@@ -2484,6 +2488,13 @@  static int netvsc_unregister_vf(struct net_device *vf_netdev)
 
 	reinit_completion(&net_device_ctx->vf_add);
 	netdev_rx_handler_unregister(vf_netdev);
+
+	/* Unlink the slave device and clear flag */
+	vf_netdev->kernel_bond = 0;
+	ndev->kernel_bond = 0;
+	vf_netdev->flags &= ~IFF_SLAVE;
+	ndev->flags &= ~IFF_MASTER;
+
 	netdev_upper_dev_unlink(vf_netdev, ndev);
 	RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
 	dev_put(vf_netdev);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ecc686409161..5befb0e23585 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1997,6 +1997,8 @@  enum netdev_reg_state {
  *	@change_proto_down: device supports setting carrier via IFLA_PROTO_DOWN
  *	@netns_local: interface can't change network namespaces
  *	@fcoe_mtu:	device supports maximum FCoE MTU, 2158 bytes
+ *	@kernel_bond:	device is auto bonded to another device without user
+ *			configuration
  *
  *	@net_notifier_list:	List of per-net netdev notifier block
  *				that follow this device when it is moved
@@ -2402,6 +2404,7 @@  struct net_device {
 	unsigned long		change_proto_down:1;
 	unsigned long		netns_local:1;
 	unsigned long		fcoe_mtu:1;
+	unsigned long		kernel_bond:1;
 
 	struct list_head	net_notifier_list;
 
@@ -5150,12 +5153,14 @@  static inline bool netif_is_macvlan_port(const struct net_device *dev)
 
 static inline bool netif_is_bond_master(const struct net_device *dev)
 {
-	return dev->flags & IFF_MASTER && dev->priv_flags & IFF_BONDING;
+	return dev->flags & IFF_MASTER &&
+		(dev->priv_flags & IFF_BONDING || dev->kernel_bond);
 }
 
 static inline bool netif_is_bond_slave(const struct net_device *dev)
 {
-	return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_BONDING;
+	return dev->flags & IFF_SLAVE &&
+		(dev->priv_flags & IFF_BONDING || dev->kernel_bond);
 }
 
 static inline bool netif_supports_nofcs(struct net_device *dev)