[PATCH net 0/3] fix ipvlan/macvlan link event handing

[PATCH net 0/3] fix ipvlan/macvlan link event handing

[Hangbin Liu]

When setting the lower-layer link up/down, the ipvlan/macvlan device
synchronizes its state via netif_stacked_transfer_operstate(), which
only checks the carrier state. However, setting the link down does not
necessarily change the carrier state for virtual interfaces like bonding.
This causes the ipvlan/macvlan state to become out of sync with the
lower-layer link state. Fix this by explicitly changing the IFF_UP flag,
similar to how VLAN handles it.

Before the patch:
  # ./rtnetlink.sh -t "kci_test_vlan kci_test_ipvlan kci_test_macvlan"
  PASS: vlan link state correct
  FAIL: ipvlan link state incorrect
  FAIL: macvlan link state incorrect

After the patch set:
  # ./rtnetlink.sh -t "kci_test_vlan kci_test_ipvlan kci_test_macvlan"
  PASS: vlan link state correct
  PASS: ipvlan link state correct
  PASS: macvlan link state correct

Hangbin Liu (3):
  ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling
  macvlan: fix NETDEV_UP/NETDEV_DOWN event handling
  selftests/rtnetlink.sh: add vlan/ipvlan/macvlan link state test

 drivers/net/ipvlan/ipvlan_main.c         | 20 +++++++-
 drivers/net/macvlan.c                    | 20 ++++++++
 tools/testing/selftests/net/rtnetlink.sh | 64 ++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 1 deletion(-)

-- 
2.46.0

[PATCH net 1/3] ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Hangbin Liu]

When setting the lower-layer link up/down, the ipvlan device synchronizes
its state via netif_stacked_transfer_operstate(), which only checks the
carrier state. However, setting the link down does not necessarily change
the carrier state for virtual interfaces like bonding. This causes the
ipvlan state to become out of sync with the lower-layer link state.

If the lower link and ipvlan are in the same namespace, this issue is
hidden because ip link show checks the link state in IFLA_LINK and has
a m_flag to control the state, displaying M-DOWN in the flags. However,
if the ipvlan and the lower link are in different namespaces, this
information is not available, and the ipvlan link state remains unchanged.
For example:

  1. Add an ipvlan over bond0.
  2. Move the ipvlan to a separate namespace and bring it up.
  3. Set bond0 link down.
  4. The ipvlan remains up.

This issue affects containers and pods, causing them to display an
incorrect link state for ipvlan. Fix this by explicitly changing the
IFF_UP flag, similar to how VLAN handles it.

Fixes: 57fb346cc7d0 ("ipvlan: Add handling of NETDEV_UP events")
Fixes: 229783970838 ("ipvlan: handle NETDEV_DOWN event")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/ipvlan/ipvlan_main.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 0ed2fd833a5d..2abe6ddc4d15 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -730,7 +730,7 @@ static int ipvlan_device_event(struct notifier_block *unused,
 	struct ipvl_dev *ipvlan, *next;
 	struct ipvl_port *port;
 	LIST_HEAD(lst_kill);
-	int err;
+	int flags, err;
 
 	if (!netif_is_ipvlan_port(dev))
 		return NOTIFY_DONE;
@@ -739,7 +739,25 @@ static int ipvlan_device_event(struct notifier_block *unused,
 
 	switch (event) {
 	case NETDEV_UP:
+		list_for_each_entry(ipvlan, &port->ipvlans, pnode) {
+			flags = ipvlan->dev->flags;
+			if (flags & IFF_UP)
+				continue;
+			dev_change_flags(ipvlan->dev, flags | IFF_UP, extack);
+			netif_stacked_transfer_operstate(ipvlan->phy_dev,
+							 ipvlan->dev);
+		}
+		break;
 	case NETDEV_DOWN:
+		list_for_each_entry(ipvlan, &port->ipvlans, pnode) {
+			flags = ipvlan->dev->flags;
+			if (!(flags & IFF_UP))
+				continue;
+			dev_close(ipvlan->dev);
+			netif_stacked_transfer_operstate(ipvlan->phy_dev,
+							 ipvlan->dev);
+		}
+		break;
 	case NETDEV_CHANGE:
 		list_for_each_entry(ipvlan, &port->ipvlans, pnode)
 			netif_stacked_transfer_operstate(ipvlan->phy_dev,
-- 
2.46.0

Re: [PATCH net 1/3] ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Sabrina Dubroca]

Hello Hangbin,

2025-04-03, 08:58:55 +0000, Hangbin Liu wrote:
> When setting the lower-layer link up/down, the ipvlan device synchronizes
> its state via netif_stacked_transfer_operstate(), which only checks the
> carrier state. However, setting the link down does not necessarily change
> the carrier state for virtual interfaces like bonding. This causes the
> ipvlan state to become out of sync with the lower-layer link state.
> 
> If the lower link and ipvlan are in the same namespace, this issue is
> hidden because ip link show checks the link state in IFLA_LINK and has
> a m_flag to control the state, displaying M-DOWN in the flags. However,
> if the ipvlan and the lower link are in different namespaces, this
> information is not available, and the ipvlan link state remains unchanged.

Is the issue with the actual behavior (sending/receiving packets,
etc), or just in how it's displayed by iproute?

> For example:
> 
>   1. Add an ipvlan over bond0.
>   2. Move the ipvlan to a separate namespace and bring it up.
>   3. Set bond0 link down.
>   4. The ipvlan remains up.
> 
> This issue affects containers and pods, causing them to display an
> incorrect link state for ipvlan. Fix this by explicitly changing the
> IFF_UP flag, similar to how VLAN handles it.

I'm not sure this change of behavior can be done anymore. And I'm not
convinced vlan's behavior is better (commit 5e7565930524 ("vlan:
support "loose binding" to the underlying network device") describes
why it's not always wanted). IMO it makes sense to have admin state
separate from link state.

If you want a consistent behavior, the admin should also not be
allowed to set the link UP again while its lower device is not, like
VLAN does:

static int vlan_dev_open(struct net_device *dev)
{
	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
	struct net_device *real_dev = vlan->real_dev;
	int err;

	if (!(real_dev->flags & IFF_UP) &&
	    !(vlan->flags & VLAN_FLAG_LOOSE_BINDING))
		return -ENETDOWN;


(but that would almost certainly break someone's scripts)

-- 
Sabrina

Re: [PATCH net 1/3] ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Hangbin Liu]

Hi Sabrina,
On Thu, Apr 03, 2025 at 12:28:54PM +0200, Sabrina Dubroca wrote:
> Hello Hangbin,
> 
> 2025-04-03, 08:58:55 +0000, Hangbin Liu wrote:
> > When setting the lower-layer link up/down, the ipvlan device synchronizes
> > its state via netif_stacked_transfer_operstate(), which only checks the
> > carrier state. However, setting the link down does not necessarily change
> > the carrier state for virtual interfaces like bonding. This causes the
> > ipvlan state to become out of sync with the lower-layer link state.
> > 
> > If the lower link and ipvlan are in the same namespace, this issue is
> > hidden because ip link show checks the link state in IFLA_LINK and has
> > a m_flag to control the state, displaying M-DOWN in the flags. However,
> > if the ipvlan and the lower link are in different namespaces, this
> > information is not available, and the ipvlan link state remains unchanged.
> 
> Is the issue with the actual behavior (sending/receiving packets,
> etc), or just in how it's displayed by iproute?

The upper link in netns up while lower link down will cause the traffic break
in the pod.

> 
> > For example:
> > 
> >   1. Add an ipvlan over bond0.
> >   2. Move the ipvlan to a separate namespace and bring it up.
> >   3. Set bond0 link down.
> >   4. The ipvlan remains up.
> > 
> > This issue affects containers and pods, causing them to display an
> > incorrect link state for ipvlan. Fix this by explicitly changing the
> > IFF_UP flag, similar to how VLAN handles it.
> 
> I'm not sure this change of behavior can be done anymore. And I'm not
> convinced vlan's behavior is better (commit 5e7565930524 ("vlan:
> support "loose binding" to the underlying network device") describes
> why it's not always wanted). IMO it makes sense to have admin state
> separate from link state.

Thanks for the comments, that's also what I am worried. I have send
a question email[1] 2 months ago but not reply yet. So I post this
patch and welcome any feedback.

[1]https://lore.kernel.org/netdev/Z67lt5v6vrltiRyG@fedora/
> 
> If you want a consistent behavior, the admin should also not be
> allowed to set the link UP again while its lower device is not, like
> VLAN does:
> 
> static int vlan_dev_open(struct net_device *dev)
> {
> 	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
> 	struct net_device *real_dev = vlan->real_dev;
> 	int err;
> 
> 	if (!(real_dev->flags & IFF_UP) &&
> 	    !(vlan->flags & VLAN_FLAG_LOOSE_BINDING))
> 		return -ENETDOWN;
> 
> 
> (but that would almost certainly break someone's scripts)

Yes, so let's wait for others feedback first.

Thanks
Hangbin

Re: [PATCH net 1/3] ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Sabrina Dubroca]

2025-04-03, 13:09:02 +0000, Hangbin Liu wrote:
> Hi Sabrina,
> On Thu, Apr 03, 2025 at 12:28:54PM +0200, Sabrina Dubroca wrote:
> > Hello Hangbin,
> > 
> > 2025-04-03, 08:58:55 +0000, Hangbin Liu wrote:
> > > When setting the lower-layer link up/down, the ipvlan device synchronizes
> > > its state via netif_stacked_transfer_operstate(), which only checks the
> > > carrier state. However, setting the link down does not necessarily change
> > > the carrier state for virtual interfaces like bonding. This causes the
> > > ipvlan state to become out of sync with the lower-layer link state.
> > > 
> > > If the lower link and ipvlan are in the same namespace, this issue is
> > > hidden because ip link show checks the link state in IFLA_LINK and has
> > > a m_flag to control the state, displaying M-DOWN in the flags. However,
> > > if the ipvlan and the lower link are in different namespaces, this
> > > information is not available, and the ipvlan link state remains unchanged.
> > 
> > Is the issue with the actual behavior (sending/receiving packets,
> > etc), or just in how it's displayed by iproute?
> 
> The upper link in netns up while lower link down will cause the traffic break
> in the pod.

That seems like the correct behavior based on the actual (not
displayed) state of the links.


I wonder if netif_stacked_transfer_operstate should consider the admin
state of the lower device as well as link state:

@@ -10724,7 +10724,7 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
 	else
 		netif_testing_off(dev);
 
-	if (netif_carrier_ok(rootdev))
+	if (netif_carrier_ok(rootdev) && rootdev->flags & IFF_UP)
 		netif_carrier_on(dev);
 	else
 		netif_carrier_off(dev);


but I haven't looked at all the consequences and possible side
effects.

-- 
Sabrina

Re: [PATCH net 1/3] ipvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Hangbin Liu]

On Thu, Apr 03, 2025 at 05:00:14PM +0200, Sabrina Dubroca wrote:
> 2025-04-03, 13:09:02 +0000, Hangbin Liu wrote:
> > Hi Sabrina,
> > On Thu, Apr 03, 2025 at 12:28:54PM +0200, Sabrina Dubroca wrote:
> > > Hello Hangbin,
> > > 
> > > 2025-04-03, 08:58:55 +0000, Hangbin Liu wrote:
> > > > When setting the lower-layer link up/down, the ipvlan device synchronizes
> > > > its state via netif_stacked_transfer_operstate(), which only checks the
> > > > carrier state. However, setting the link down does not necessarily change
> > > > the carrier state for virtual interfaces like bonding. This causes the
> > > > ipvlan state to become out of sync with the lower-layer link state.
> > > > 
> > > > If the lower link and ipvlan are in the same namespace, this issue is
> > > > hidden because ip link show checks the link state in IFLA_LINK and has
> > > > a m_flag to control the state, displaying M-DOWN in the flags. However,
> > > > if the ipvlan and the lower link are in different namespaces, this
> > > > information is not available, and the ipvlan link state remains unchanged.
> > > 
> > > Is the issue with the actual behavior (sending/receiving packets,
> > > etc), or just in how it's displayed by iproute?
> > 
> > The upper link in netns up while lower link down will cause the traffic break
> > in the pod.
> 
> That seems like the correct behavior based on the actual (not
> displayed) state of the links.

Hmm, since this behavior is controversial, do you think if we should
drop this until some users request?

> 
> 
> I wonder if netif_stacked_transfer_operstate should consider the admin
> state of the lower device as well as link state:
> 
> @@ -10724,7 +10724,7 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>  	else
>  		netif_testing_off(dev);
>  
> -	if (netif_carrier_ok(rootdev))
> +	if (netif_carrier_ok(rootdev) && rootdev->flags & IFF_UP)
>  		netif_carrier_on(dev);
>  	else
>  		netif_carrier_off(dev);
> 
> 
> but I haven't looked at all the consequences and possible side
> effects.

I'm not sure. Only sync link carrier seems reasonable too.

Thanks
Hangbin

[PATCH net 2/3] macvlan: fix NETDEV_UP/NETDEV_DOWN event handling

[Hangbin Liu]

Setting the link down does not necessarily change the carrier state for
virtual interfaces like bonding. Therefore, handling the device up/down
event via netif_stacked_transfer_operstate() is not sufficient.

If the lower link and macvlan are in different namespaces, the upper link
state may become out of sync. Fix this by updating the handling logic to be
similar to VLAN.

Fixes: de7d244d0a35 ("macvlan: make operstate and carrier more accurate")
Fixes: 80fd2d6ca546 ("macvlan: Change status when lower device goes down")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/macvlan.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d0dfa6bca6cc..f254cda14dac 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1781,10 +1781,12 @@ static void update_port_bc_queue_len(struct macvlan_port *port)
 static int macvlan_device_event(struct notifier_block *unused,
 				unsigned long event, void *ptr)
 {
+	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct macvlan_dev *vlan, *next;
 	struct macvlan_port *port;
 	LIST_HEAD(list_kill);
+	int flags;
 
 	if (!netif_is_macvlan_port(dev))
 		return NOTIFY_DONE;
@@ -1793,7 +1795,25 @@ static int macvlan_device_event(struct notifier_block *unused,
 
 	switch (event) {
 	case NETDEV_UP:
+		list_for_each_entry(vlan, &port->vlans, list) {
+			flags = vlan->dev->flags;
+			if (flags & IFF_UP)
+				continue;
+			dev_change_flags(vlan->dev, flags | IFF_UP, extack);
+			netif_stacked_transfer_operstate(vlan->lowerdev,
+							 vlan->dev);
+		}
+		break;
 	case NETDEV_DOWN:
+		list_for_each_entry(vlan, &port->vlans, list) {
+			flags = vlan->dev->flags;
+			if (!(flags & IFF_UP))
+				continue;
+			dev_close(vlan->dev);
+			netif_stacked_transfer_operstate(vlan->lowerdev,
+							 vlan->dev);
+		}
+		break;
 	case NETDEV_CHANGE:
 		list_for_each_entry(vlan, &port->vlans, list)
 			netif_stacked_transfer_operstate(vlan->lowerdev,
-- 
2.46.0

[PATCH net 3/3] selftests/rtnetlink.sh: add vlan/ipvlan/macvlan link state test

[Hangbin Liu]

Add tests to create vlan/ipvlan/macvlan over a bond interface and move
them to a separate network namespace. Verify that the upper link state
correctly reflects the lower-layer link state.

 # ./rtnetlink.sh -t "kci_test_vlan kci_test_ipvlan kci_test_macvlan"
 PASS: vlan link state correct
 PASS: ipvlan link state correct
 PASS: macvlan link state correct

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 2e8243a65b50..de2f5bed8777 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -30,6 +30,9 @@ ALL_TESTS="
 	kci_test_address_proto
 	kci_test_enslave_bonding
 	kci_test_mngtmpaddr
+	kci_test_vlan
+	kci_test_ipvlan
+	kci_test_macvlan
 "
 
 devdummy="test-dummy0"
@@ -1334,6 +1337,67 @@ kci_test_mngtmpaddr()
 	return $ret
 }
 
+kci_test_link_state()
+{
+	local test_link=$(mktemp -u test_link-XXX)
+	local bond=$(mktemp -u bond-XXX)
+	local link_param=$2
+	local link_type=$1
+	local ret=0
+
+	setup_ns testns
+	if [ $? -ne 0 ]; then
+		end_test "SKIP ${link_type} netns tests: cannot add net namespace $testns"
+		return $ksft_skip
+	fi
+
+	# 1. Test link state over bond
+	run_cmd ip link add dev $bond type bond mode active-backup miimon 10
+	run_cmd ip link set dev ${devdummy} down
+	run_cmd ip link set dev ${devdummy} master $bond
+	run_cmd ip link set dev ${bond} up
+	run_cmd ip link add link ${bond} name ${test_link} type ${link_type} ${link_param}
+	run_cmd ip link set ${test_link} up
+	run_cmd ip link set ${devdummy} down
+	run_cmd_grep_fail "LOWER_UP" ip link show dev ${test_link}
+	run_cmd ip link set ${devdummy} up
+	run_cmd_grep "LOWER_UP" ip link show dev ${test_link}
+
+	# 2. Test link state over bond in netns
+	run_cmd ip link set ${test_link} netns ${testns}
+	run_cmd ip -n ${testns} link set ${test_link} up
+	run_cmd ip link set ${devdummy} down
+	run_cmd_grep_fail "LOWER_UP" ip -n ${testns} link show dev ${test_link}
+	run_cmd ip link set ${devdummy} up
+	run_cmd_grep "LOWER_UP" ip -n ${testns} link show dev ${test_link}
+	ip -n ${testns} link del ${test_link}
+
+	if [ $ret -ne 0 ]; then
+		end_test "FAIL: ${link_type} link state incorrect"
+	else
+		end_test "PASS: ${link_type} link state correct"
+	fi
+
+	ip netns del "$testns"
+	ip link del dev ${bond}
+	return $ret
+}
+
+kci_test_vlan()
+{
+	kci_test_link_state "vlan" "id 2"
+}
+
+kci_test_ipvlan()
+{
+	kci_test_link_state "ipvlan" "mode l2"
+}
+
+kci_test_macvlan()
+{
+	kci_test_link_state "macvlan" "mode bridge"
+}
+
 kci_test_rtnl()
 {
 	local current_test
-- 
2.46.0