[PATCH net-next 0/3] net: Fix protodown with macvlan

[PATCH net-next 0/3] net: Fix protodown with macvlan

[Ido Schimmel]

When protodown is enabled on a macvlan, two bugs cause the macvlan to
incorrectly report an UP operational state:

1. Toggling protodown on and then off on the macvlan while the lower
device has no carrier causes the macvlan to report UP instead of
LOWERLAYERDOWN, since netif_change_proto_down() unconditionally turns
the carrier on.

2. Toggling the lower device's carrier while protodown is enabled on the
macvlan causes the macvlan to inherit the UP operational state,
effectively bypassing the protodown mechanism.

Patch #1 solves the first problem by introducing a new NDO that allows
different drivers to react differently to protodown being cleared. For
macvlan it is implemented by transferring the operational state from the
lower device. For vxlan (the other driver that supports protodown) it
continues to be implemented by turning on the carrier.

Patch #2 prevents a macvlan with protodown enabled from inheriting the
operational state of its lower device.

Patch #3 adds a selftest covering both bugs and the basic protodown
functionality.

Targeting at net-next since these are not regressions (i.e., never
worked).

Ido Schimmel (3):
  net: Do not unconditionally turn on carrier when clearing protodown
  macvlan: Do not transfer operational state when protodown is enabled
  selftests: net: Add protodown tests

 .../networking/net_cachelines/net_device.rst  |   1 -
 drivers/net/macvlan.c                         |  12 +-
 drivers/net/vxlan/vxlan_core.c                |   8 +-
 include/linux/netdevice.h                     |   6 +-
 net/core/dev.c                                |   4 +-
 net/core/rtnetlink.c                          |   2 +-
 tools/testing/selftests/net/Makefile          |   1 +
 tools/testing/selftests/net/protodown.sh      | 160 ++++++++++++++++++
 8 files changed, 185 insertions(+), 9 deletions(-)
 create mode 100755 tools/testing/selftests/net/protodown.sh

-- 
2.53.0

[PATCH net-next 1/3] net: Do not unconditionally turn on carrier when clearing protodown

[Ido Schimmel]

The protodown functionality is currently supported by vxlan and macvlan
and it allows to keep an interface down by turning off its carrier:

 # ip link add name dummy1 up type dummy
 # ip link add name macvlan1 up link dummy1 type macvlan mode bridge
 # ip link set dev macvlan1 protodown on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  DOWN           0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>

When protodown is cleared, the core unconditionally enables carrier on
the interface:

 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

This is wrong as it means that a macvlan can end up with a carrier when
its lower device does not have a carrier:

 # ip link set dev dummy1 carrier off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  LOWERLAYERDOWN 0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev macvlan1 protodown on
 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

Solve this by adding a new NDO that allows different drivers to react
differently to protodown being cleared. Change the core to invoke the
NDO and implement it for vxlan and macvlan. Maintain the current
behavior for the former, but for macvlan implement the NDO by
transferring the operational state from its lower device.

Output with the patch:

 # ip link add name dummy1 up type dummy
 # ip link add name macvlan1 up link dummy1 type macvlan mode bridge
 # ip link set dev dummy1 carrier off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  LOWERLAYERDOWN 0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev macvlan1 protodown on
 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  LOWERLAYERDOWN 0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev dummy1 carrier on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>
 # ip link set dev macvlan1 protodown on
 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

Make the NDO only about clearing protodown to avoid suspicion that this
is about re-introducing the NDO that was removed by commit 2106efda785b
("net: remove .ndo_change_proto_down").

Remove net_device::change_proto_down since we can now use the presence
of the new NDO as an indication that changing protodown is supported.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/net_cachelines/net_device.rst | 1 -
 drivers/net/macvlan.c                                  | 7 ++++++-
 drivers/net/vxlan/vxlan_core.c                         | 8 +++++++-
 include/linux/netdevice.h                              | 6 ++++--
 net/core/dev.c                                         | 4 ++--
 net/core/rtnetlink.c                                   | 2 +-
 6 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
index 1c19bb7705df..267af4957b0d 100644
--- a/Documentation/networking/net_cachelines/net_device.rst
+++ b/Documentation/networking/net_cachelines/net_device.rst
@@ -167,7 +167,6 @@ struct lock_class_key*              qdisc_tx_busylock
 bool                                proto_down
 unsigned:1                          wol_enabled
 unsigned_long:1                     see_all_hwtstamp_requests
-unsigned_long:1                     change_proto_down
 unsigned_long:1                     netns_immutable
 unsigned_long:1                     fcoe_mtu
 struct list_head                    net_notifier_list
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index c40fa331836b..61effa295c49 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -904,6 +904,11 @@ static int macvlan_hwtstamp_set(struct net_device *dev,
 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
 }
 
+static void macvlan_clear_proto_down(struct net_device *dev)
+{
+	netif_stacked_transfer_operstate(macvlan_dev_real_dev(dev), dev);
+}
+
 /*
  * macvlan network devices have devices nesting below it and are a special
  * "super class" of normal network devices; split their locks off into a
@@ -1211,6 +1216,7 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_hwtstamp_get	= macvlan_hwtstamp_get,
 	.ndo_hwtstamp_set	= macvlan_hwtstamp_set,
+	.ndo_clear_proto_down	= macvlan_clear_proto_down,
 };
 
 static void macvlan_dev_free(struct net_device *dev)
@@ -1230,7 +1236,6 @@ void macvlan_common_setup(struct net_device *dev)
 	dev->priv_flags	       &= ~IFF_TX_SKB_SHARING;
 	netif_keep_dst(dev);
 	dev->priv_flags	       |= IFF_UNICAST_FLT;
-	dev->change_proto_down	= true;
 	dev->netdev_ops		= &macvlan_netdev_ops;
 	dev->needs_free_netdev	= true;
 	dev->priv_destructor	= macvlan_dev_free;
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index e88798497503..a3ed9fbd60a6 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -3271,6 +3271,11 @@ static int vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 	return 0;
 }
 
+static void vxlan_clear_proto_down(struct net_device *dev)
+{
+	netif_carrier_on(dev);
+}
+
 static const struct net_device_ops vxlan_netdev_ether_ops = {
 	.ndo_init		= vxlan_init,
 	.ndo_uninit		= vxlan_uninit,
@@ -3292,6 +3297,7 @@ static const struct net_device_ops vxlan_netdev_ether_ops = {
 	.ndo_mdb_dump		= vxlan_mdb_dump,
 	.ndo_mdb_get		= vxlan_mdb_get,
 	.ndo_fill_metadata_dst	= vxlan_fill_metadata_dst,
+	.ndo_clear_proto_down	= vxlan_clear_proto_down,
 };
 
 static const struct net_device_ops vxlan_netdev_raw_ops = {
@@ -3302,6 +3308,7 @@ static const struct net_device_ops vxlan_netdev_raw_ops = {
 	.ndo_start_xmit		= vxlan_xmit,
 	.ndo_change_mtu		= vxlan_change_mtu,
 	.ndo_fill_metadata_dst	= vxlan_fill_metadata_dst,
+	.ndo_clear_proto_down	= vxlan_clear_proto_down,
 };
 
 /* Info for udev, that this is a virtual tunnel endpoint */
@@ -3368,7 +3375,6 @@ static void vxlan_setup(struct net_device *dev)
 
 	netif_keep_dst(dev);
 	dev->priv_flags |= IFF_NO_QUEUE;
-	dev->change_proto_down = true;
 	dev->lltx = true;
 
 	/* MTU range: 68 - 65535 */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0e1e581efc5a..3757dd6604b8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1432,6 +1432,9 @@ struct netdev_net_notifier {
  *			   struct kernel_hwtstamp_config *kernel_config,
  *			   struct netlink_ext_ack *extack);
  *	Change the hardware timestamping parameters for NIC device.
+ *
+ * void (*ndo_clear_proto_down)(struct net_device *dev);
+ *	Enable carrier after dev->proto_down was cleared.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1683,6 +1686,7 @@ struct net_device_ops {
 	int			(*ndo_hwtstamp_set)(struct net_device *dev,
 						    struct kernel_hwtstamp_config *kernel_config,
 						    struct netlink_ext_ack *extack);
+	void			(*ndo_clear_proto_down)(struct net_device *dev);
 
 #if IS_ENABLED(CONFIG_NET_SHAPER)
 	/**
@@ -2062,7 +2066,6 @@ enum netdev_reg_state {
  *			ndo_hwtstamp_set() for all timestamp requests
  *			regardless of source, even if those aren't
  *			HWTSTAMP_SOURCE_NETDEV
- *	@change_proto_down: device supports setting carrier via IFLA_PROTO_DOWN
  *	@netns_immutable: interface can't change network namespaces
  *	@fcoe_mtu:	device supports maximum FCoE MTU, 2158 bytes
  *
@@ -2476,7 +2479,6 @@ struct net_device {
 
 	/* priv_flags_slow, ungrouped to save space */
 	unsigned long		see_all_hwtstamp_requests:1;
-	unsigned long		change_proto_down:1;
 	unsigned long		netns_immutable:1;
 	unsigned long		fcoe_mtu:1;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 06c195906231..ff66762592e4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10143,14 +10143,14 @@ EXPORT_SYMBOL(netdev_port_same_parent_id);
 
 int netif_change_proto_down(struct net_device *dev, bool proto_down)
 {
-	if (!dev->change_proto_down)
+	if (!dev->netdev_ops->ndo_clear_proto_down)
 		return -EOPNOTSUPP;
 	if (!netif_device_present(dev))
 		return -ENODEV;
 	if (proto_down)
 		netif_carrier_off(dev);
 	else
-		netif_carrier_on(dev);
+		dev->netdev_ops->ndo_clear_proto_down(dev);
 	WRITE_ONCE(dev->proto_down, proto_down);
 	return 0;
 }
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b613bb6e07df..30951e5a9555 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3009,7 +3009,7 @@ static int do_set_proto_down(struct net_device *dev,
 	bool proto_down;
 	int err;
 
-	if (!dev->change_proto_down) {
+	if (!dev->netdev_ops->ndo_clear_proto_down) {
 		NL_SET_ERR_MSG(extack,  "Protodown not supported by device");
 		return -EOPNOTSUPP;
 	}
-- 
2.53.0

[PATCH net-next 2/3] macvlan: Do not transfer operational state when protodown is enabled

[Ido Schimmel]

The protodown functionality allows user space to keep a macvlan down by
turning off its carrier:

 # ip link add name dummy1 up type dummy
 # ip link add name macvlan1 up link dummy1 type macvlan mode bridge
 # ip link set dev macvlan1 protodown on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  DOWN           0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>

Different applications can set different protodown reasons, which
prevents an application from bringing up a macvlan as long as others
want it down:

 # ip link set dev macvlan1 protodown_reason 1 on
 # ip link set dev macvlan1 protodown_reason 2 on
 # ip link set dev macvlan1 protodown off
 Error: Cannot clear protodown, active reasons.
 # ip link set dev macvlan1 protodown_reason 2 off
 # ip link set dev macvlan1 protodown off
 Error: Cannot clear protodown, active reasons.
 # ip link set dev macvlan1 protodown_reason 1 off
 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

Unfortunately, this mechanism is not very useful when the macvlan can be
brought up by toggling the carrier of its lower device:

 # ip link set dev macvlan1 protodown on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  DOWN           0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev dummy1 carrier off
 # ip link set dev dummy1 carrier on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

Obviously, this is not the intended behavior and it is unlikely to be
relied on by anyone. In fact, it is a problem for applications like FRR
that use protodown with macvlan on top of a bridge as part of Virtual
Router Redundancy Protocol (VRRP).

Solve this by not transferring the operational state from the lower
device to the macvlan if protodown is enabled on the macvlan. Note that
READ_ONCE() is not needed as RTNL is held. Also note that vxlan (the
other driver that supports protodown) does not suffer from this problem.

Output with the patch:

 # ip link add name dummy1 up type dummy
 # ip link add name macvlan1 up link dummy1 type macvlan mode bridge
 # ip link set dev macvlan1 protodown on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  DOWN           0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev dummy1 carrier off
 # ip link set dev dummy1 carrier on
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  DOWN           0a:5c:a3:05:c7:86 <NO-CARRIER,BROADCAST,MULTICAST,UP>
 # ip link set dev macvlan1 protodown off
 $ ip -br link show dev macvlan1
 macvlan1@dummy1  UP             0a:5c:a3:05:c7:86 <BROADCAST,MULTICAST,UP,LOWER_UP>

Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/macvlan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 61effa295c49..50c0bc8a38db 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1822,9 +1822,12 @@ static int macvlan_device_event(struct notifier_block *unused,
 	case NETDEV_UP:
 	case NETDEV_DOWN:
 	case NETDEV_CHANGE:
-		list_for_each_entry(vlan, &port->vlans, list)
+		list_for_each_entry(vlan, &port->vlans, list) {
+			if (vlan->dev->proto_down)
+				continue;
 			netif_stacked_transfer_operstate(vlan->lowerdev,
 							 vlan->dev);
+		}
 		break;
 	case NETDEV_FEAT_CHANGE:
 		list_for_each_entry(vlan, &port->vlans, list) {
-- 
2.53.0

[PATCH net-next 3/3] selftests: net: Add protodown tests

[Ido Schimmel]

Add a selftest for the protodown mechanism using a macvlan device on
top of a dummy device.

Four test cases are included:

1. Basic protodown toggling: Verify that setting protodown results in
   DOWN operational state and clearing it restores UP.

2. Protodown reasons: Verify that protodown cannot be cleared while
   there are active protodown reasons, but can be cleared once all
   reasons are removed.

3. Operational state inheritance: Verify that toggling the lower
   device's carrier while protodown is on does not cause the macvlan to
   inherit the UP operational state.

4. Lower layer down: Verify that toggling protodown while the lower
   device has no carrier does not cause the macvlan to transition to UP
   operational state.

Note that the last two test cases fail without "macvlan: Do not transfer
operational state when protodown is enabled" and "net: Add
.ndo_clear_proto_down":

 # ./protodown.sh
 TEST: Basic protodown on/off                                        [ OK ]
 TEST: Protodown reasons                                             [ OK ]
 TEST: Inheriting operational state with protodown                   [FAIL]
         Macvlan operational state is not DOWN despite protodown
 TEST: Protodown with lower layer down                               [FAIL]
         Macvlan is not LOWERLAYERDOWN after clearing protodown

Assisted-by: Claude:claude-opus-4-6
Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 tools/testing/selftests/net/Makefile     |   1 +
 tools/testing/selftests/net/protodown.sh | 160 +++++++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100755 tools/testing/selftests/net/protodown.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index a275ed584026..70e0ab43dc5d 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -69,6 +69,7 @@ TEST_PROGS := \
 	nl_netdev.py \
 	nl_nlctrl.py \
 	pmtu.sh \
+	protodown.sh \
 	psock_snd.sh \
 	reuseaddr_ports_exhausted.sh \
 	reuseport_addr_any.sh \
diff --git a/tools/testing/selftests/net/protodown.sh b/tools/testing/selftests/net/protodown.sh
new file mode 100755
index 000000000000..4ad795d3d011
--- /dev/null
+++ b/tools/testing/selftests/net/protodown.sh
@@ -0,0 +1,160 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test the "protodown" mechanism. Verify basic protodown toggling, protodown
+# reasons, operational state inheritance when the lower device carrier changes,
+# and correct operational state when the lower device has no carrier.
+
+# shellcheck disable=SC1091,SC2034,SC2154,SC2317
+source lib.sh
+
+require_command jq
+
+ALL_TESTS="
+	protodown_basic
+	protodown_reasons
+	protodown_inherit_operstate
+	protodown_lower_layer_down
+"
+
+operstate_get()
+{
+	local ns=$1; shift
+	local dev=$1; shift
+
+	ip -n "$ns" -j link show dev "$dev" | jq -r '.[].operstate'
+}
+
+operstate_check()
+{
+	local ns=$1; shift
+	local dev=$1; shift
+	local expected=$1; shift
+
+	local current
+	current=$(operstate_get "$ns" "$dev")
+
+	[ "$current" = "$expected" ]
+}
+
+setup_prepare()
+{
+	setup_ns NS
+	defer cleanup_all_ns
+
+	ip -n "$NS" link add name dummy0 type dummy
+	ip -n "$NS" link set dev dummy0 up
+
+	ip -n "$NS" link add name macvlan0 link dummy0 type macvlan mode bridge
+	ip -n "$NS" link set dev macvlan0 up
+}
+
+protodown_basic()
+{
+	RET=0
+
+	ip -n "$NS" link set dev macvlan0 protodown on
+	check_err $? "Failed to set protodown on"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 DOWN
+	check_err $? "Operational state is not DOWN after setting protodown"
+
+	ip -n "$NS" link set dev macvlan0 protodown off
+	check_err $? "Failed to set protodown off"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 UP
+	check_err $? "Operational state is not UP after clearing protodown"
+
+	log_test "Basic protodown on/off"
+}
+
+protodown_reasons()
+{
+	RET=0
+
+	ip -n "$NS" link set dev macvlan0 protodown on
+
+	ip -n "$NS" link set dev macvlan0 protodown_reason 0 on
+	check_err $? "Failed to set protodown reason bit 0"
+
+	# Cannot clear protodown while reasons are active.
+	ip -n "$NS" link set dev macvlan0 protodown off 2>/dev/null
+	check_fail $? "Clearing protodown succeeded with active reasons"
+
+	ip -n "$NS" link set dev macvlan0 protodown_reason 0 off
+	check_err $? "Failed to clear protodown reason bit 0"
+
+	# Can clear protodown when no reasons are active.
+	ip -n "$NS" link set dev macvlan0 protodown off
+	check_err $? "Failed to clear protodown with no active reasons"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 UP
+	check_err $? "Operational state is not UP after clearing protodown"
+
+	log_test "Protodown reasons"
+}
+
+protodown_inherit_operstate()
+{
+	RET=0
+
+	ip -n "$NS" link set dev macvlan0 protodown on
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 DOWN
+	check_err $? "Operational state is not DOWN after setting protodown"
+
+	# Toggle carrier on the lower device. The macvlan should stay DOWN
+	# because protodown is on.
+	ip -n "$NS" link set dev dummy0 carrier off
+	ip -n "$NS" link set dev dummy0 carrier on
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" dummy0 UP
+	check_err $? "Lower device is not UP after carrier on"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 DOWN
+	check_err $? "Macvlan operational state is not DOWN despite protodown"
+
+	# Clear protodown and verify the macvlan comes back up.
+	ip -n "$NS" link set dev macvlan0 protodown off
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 UP
+	check_err $? "Operational state is not UP after clearing protodown"
+
+	log_test "Inheriting operational state with protodown"
+}
+
+protodown_lower_layer_down()
+{
+	RET=0
+
+	# Bring the lower device carrier down first.
+	ip -n "$NS" link set dev dummy0 carrier off
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 LOWERLAYERDOWN
+	check_err $? "Macvlan is not LOWERLAYERDOWN with lower carrier off"
+
+	# Toggle protodown on and off while lower has no carrier. The macvlan
+	# should not transition to UP.
+	ip -n "$NS" link set dev macvlan0 protodown on
+	check_err $? "Failed to set protodown on"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 LOWERLAYERDOWN
+	check_err $? "Macvlan is not LOWERLAYERDOWN after setting protodown"
+
+	ip -n "$NS" link set dev macvlan0 protodown off
+	check_err $? "Failed to set protodown off"
+
+	busywait "$BUSYWAIT_TIMEOUT" operstate_check "$NS" macvlan0 LOWERLAYERDOWN
+	check_err $? "Macvlan is not LOWERLAYERDOWN after clearing protodown"
+
+	log_test "Protodown with lower layer down"
+
+	ip -n "$NS" link set dev macvlan0 protodown off
+	ip -n "$NS" link set dev dummy0 carrier on
+}
+
+trap defer_scopes_cleanup EXIT
+setup_prepare
+tests_run
+
+exit "$EXIT_STATUS"
-- 
2.53.0

Re: [PATCH net-next 1/3] net: Do not unconditionally turn on carrier when clearing protodown

[Jakub Kicinski]

On Wed, 29 Apr 2026 15:46:22 +0300 Ido Schimmel wrote:
> Solve this by adding a new NDO that allows different drivers to react
> differently to protodown being cleared. Change the core to invoke the
> NDO and implement it for vxlan and macvlan. Maintain the current
> behavior for the former, but for macvlan implement the NDO by
> transferring the operational state from its lower device.

Maybe just me but an NDO for this seems like an overkill.
Isn't our range of options for the callback either follow
lower or carrier on? Do you expect more complex logic?

Re: [PATCH net-next 2/3] macvlan: Do not transfer operational state when protodown is enabled

[Jakub Kicinski]

On Wed, 29 Apr 2026 15:46:23 +0300 Ido Schimmel wrote:
> -		list_for_each_entry(vlan, &port->vlans, list)
> +		list_for_each_entry(vlan, &port->vlans, list) {
> +			if (vlan->dev->proto_down)
> +				continue;
>  			netif_stacked_transfer_operstate(vlan->lowerdev,
>  							 vlan->dev);

Doesn't feel particularly macvlan-specific?
Other simple upper devs don't support protodown
but when they do presumably they'll have to add
this exact condition, too, so why not add it in
netif_stacked_transfer_operstate()?

Re: [PATCH net-next 1/3] net: Do not unconditionally turn on carrier when clearing protodown

[Ido Schimmel]

On Fri, May 01, 2026 at 06:08:43PM -0700, Jakub Kicinski wrote:
> On Wed, 29 Apr 2026 15:46:22 +0300 Ido Schimmel wrote:
> > Solve this by adding a new NDO that allows different drivers to react
> > differently to protodown being cleared. Change the core to invoke the
> > NDO and implement it for vxlan and macvlan. Maintain the current
> > behavior for the former, but for macvlan implement the NDO by
> > transferring the operational state from its lower device.
> 
> Maybe just me but an NDO for this seems like an overkill.
> Isn't our range of options for the callback either follow
> lower or carrier on? Do you expect more complex logic?

Thanks for reviewing. I don't expect more complex logic, but resolving
the linked device via the driver is more straightforward (unless I'm
missing something).

There are drivers that only implement ndo_get_iflink(), but don't
implement get_link_net(), so it's unclear in which netns we need to look
up the device using the ifindex we got from ndo_get_iflink(). Netdevsim
is one example. There are also some drivers that only implement
get_link_net() such as vxlan.

The popular drivers such as vlan, macvlan and veth obviously implement
both.

I added a helper [1] that resolves the linked device if these operations
are present and otherwise returns the device itself. Similar to
dev_get_iflink() which only gives us the ifindex.

Another option is to add a new NDO that gives us a pointer to the linked
device.

[1] https://github.com/idosch/linux/commit/34c57bceb1a7e52da0f973914f8a4dd08b7f9072.patch

Re: [PATCH net-next 2/3] macvlan: Do not transfer operational state when protodown is enabled

[Ido Schimmel]

On Fri, May 01, 2026 at 06:09:48PM -0700, Jakub Kicinski wrote:
> On Wed, 29 Apr 2026 15:46:23 +0300 Ido Schimmel wrote:
> > -		list_for_each_entry(vlan, &port->vlans, list)
> > +		list_for_each_entry(vlan, &port->vlans, list) {
> > +			if (vlan->dev->proto_down)
> > +				continue;
> >  			netif_stacked_transfer_operstate(vlan->lowerdev,
> >  							 vlan->dev);
> 
> Doesn't feel particularly macvlan-specific?
> Other simple upper devs don't support protodown
> but when they do presumably they'll have to add
> this exact condition, too, so why not add it in
> netif_stacked_transfer_operstate()?

It seemed more consistent with patch #1 that invokes
netif_stacked_transfer_operstate() from the driver. I can move it to the
core in v2 [1] assuming we go for a solution that doesn't involve the
driver at all.

[1] https://github.com/idosch/linux/commit/1d86acbf0affc9004fad79820bbb2172aeb305a4.patch

Re: [PATCH net-next 1/3] net: Do not unconditionally turn on carrier when clearing protodown

[Jakub Kicinski]

On Sun, 3 May 2026 21:08:03 +0300 Ido Schimmel wrote:
> There are drivers that only implement ndo_get_iflink(), but don't
> implement get_link_net(), so it's unclear in which netns we need to look
> up the device using the ifindex we got from ndo_get_iflink(). Netdevsim
> is one example. There are also some drivers that only implement
> get_link_net() such as vxlan.
> 
> The popular drivers such as vlan, macvlan and veth obviously implement
> both.

It's probably unintentional omission for the less popular devices.
Easy to forget to add an NDO (which is also the basis for my initial
comments).

> I added a helper [1] that resolves the linked device if these operations
> are present and otherwise returns the device itself. Similar to
> dev_get_iflink() which only gives us the ifindex.

The linked diff LGTM, thanks!

> Another option is to add a new NDO that gives us a pointer to the linked
> device.

Re: [PATCH net-next 2/3] macvlan: Do not transfer operational state when protodown is enabled

[Jakub Kicinski]

On Sun, 3 May 2026 21:19:34 +0300 Ido Schimmel wrote:
> On Fri, May 01, 2026 at 06:09:48PM -0700, Jakub Kicinski wrote:
> > On Wed, 29 Apr 2026 15:46:23 +0300 Ido Schimmel wrote:  
> > > -		list_for_each_entry(vlan, &port->vlans, list)
> > > +		list_for_each_entry(vlan, &port->vlans, list) {
> > > +			if (vlan->dev->proto_down)
> > > +				continue;
> > >  			netif_stacked_transfer_operstate(vlan->lowerdev,
> > >  							 vlan->dev);  
> > 
> > Doesn't feel particularly macvlan-specific?
> > Other simple upper devs don't support protodown
> > but when they do presumably they'll have to add
> > this exact condition, too, so why not add it in
> > netif_stacked_transfer_operstate()?  
> 
> It seemed more consistent with patch #1 that invokes
> netif_stacked_transfer_operstate() from the driver. I can move it to the
> core in v2 [1] assuming we go for a solution that doesn't involve the
> driver at all.
> 
> [1] https://github.com/idosch/linux/commit/1d86acbf0affc9004fad79820bbb2172aeb305a4.patch

👍