Re: [PATCH net] macvlan: fix allmulti reference counting in passthru mode

[Simon Horman]

On Mon, Oct 26, 2026 at 07:46:35PM -0700, Prasad Koya wrote:
> Enabling allmulticast on a down passthru macvlan, bringing it up,
> then toggling allmulticast off and on can underflow the lower device's
> allmulti reference count.
> 
> macvlan_change_rx_flags() only propagates IFF_ALLMULTI changes while
> the macvlan is up. However, the passthru path in macvlan_open() skips
> taking an allmulti reference when IFF_ALLMULTI is already set.
> Clearing allmulticast after opening the device therefore decrements
> a reference that was never taken. With no other allmulti users, the
> unsigned counter wraps from zero to UINT_MAX. The next increment
> fails with -EOVERFLOW and reports "allmulti touches roof".
> 

Hi Prasad,

A Fixes tag should go here.
(No blank line between it and your Signed-off-by line).

There is no need to repost just to address this.
Replying with the Fixes tag should be sufficient.

> Signed-off-by: Prasad Koya <prasad@xxxxxxxxxx>
> ---
> Reproducer:
> 
>   #!/bin/bash
>   set -euo pipefail
> 
>   ns=allmulti-test
>   host=am-veth0
>   peer=am-veth1
>   mv=am-mv0
> 
>   cleanup()
>   {
>       ip netns del "$ns" 2>/dev/null || true
>   }
>   trap cleanup EXIT
> 
>   ip netns add "$ns"
>   ip link add "$host" type veth peer name "$peer"
>   ip link set "$peer" netns "$ns"
> 
>   ip link set "$host" up
>   ip -n "$ns" link set lo up
>   ip -n "$ns" link set "$peer" up
> 
>   ip -n "$ns" link add link "$peer" name "$mv" type macvlan mode passthru
> 
>   ip -n "$ns" link set dev "$mv" allmulticast on
>   ip -n "$ns" link set dev "$mv" up
>   ip -n "$ns" link set dev "$mv" allmulticast off
>   ip -n "$ns" link set dev "$mv" allmulticast on
> 
>  drivers/net/macvlan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index afad90b9222a..e20d587453d1 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -643,6 +643,14 @@ static int macvlan_open(struct net_device *dev)
>  			if (err < 0)
>  				goto out;
>  		}
> +		if (dev->flags & IFF_ALLMULTI) {
> +			err = dev_set_allmulti(lowerdev, 1);
> +			if (err < 0) {
> +				if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
> +					dev_set_promiscuity(lowerdev, -1);
> +				goto out;
> +			}
> +		}
>  		goto hash_add;
>  	}
>  
> @@ -713,6 +721,8 @@ static int macvlan_stop(struct net_device *dev)
>  	dev_mc_unsync(lowerdev, dev);
>  
>  	if (macvlan_passthru(vlan->port)) {
> +		if (dev->flags & IFF_ALLMULTI)
> +			dev_set_allmulti(lowerdev, -1);
>  		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
>  			dev_set_promiscuity(lowerdev, -1);
>  		goto hash_del;
> 
> base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
> -- 
> 2.43.0
> 

I'm unsure if it matters, but the order of initialisation of multicast and
promiscuity differs here from the non-pass-through case.

Overall I think the approach you have taken here is appropriate
as a minimal fix for net.

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>


But I do see that there is growing duplication of code in the
pass-through and non-pass-through paths. And I believe that
is due to the approach taken by the existing code of using
goto to control flow (other than for cleanup in error paths).

In the longer term I think it would be nice to address that.
I took a stab at that. Perhaps I'm overthinking things.
But, FWIIW, this is what I cam up with (compile tested only).

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index a48c9e662fe9..f2a3cf540e4e 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -631,24 +631,31 @@ static const struct header_ops macvlan_hard_header_ops = {
 	.parse_protocol	= eth_header_parse_protocol,
 };
 
-static int macvlan_open(struct net_device *dev)
+static int macvlan_should_set_promisc(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
-	struct net_device *lowerdev = vlan->lowerdev;
-	int err;
 
-	if (macvlan_passthru(vlan->port)) {
-		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC)) {
-			err = dev_set_promiscuity(lowerdev, 1);
-			if (err < 0)
-				goto out;
-		}
-		goto hash_add;
-	}
+	if (macvlan_passthru(vlan->port))
+		return !(vlan->flags & MACVLAN_FLAG_NOPROMISC);
+
+	return dev->flags & IFF_PROMISC;
+}
+
+/* Initialise acceleration or add unicast address.
+ * Skipped for pass-through ports.
+ */
+static int macvlan_init_vlan(struct net_device *dev)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev;
+
+	if (macvlan_passthru(vlan->port))
+		return 0;
 
-	err = -EADDRINUSE;
 	if (macvlan_addr_busy(vlan->port, dev->dev_addr))
-		goto out;
+		return -EADDRINUSE;
+
+	lowerdev = vlan->lowerdev;
 
 	/* Attempt to populate accel_priv which is used to offload the L2
 	 * forwarding requests for unicast packets.
@@ -661,40 +668,71 @@ static int macvlan_open(struct net_device *dev)
 	 * populated we must add the unicast address to the lower device.
 	 */
 	if (IS_ERR_OR_NULL(vlan->accel_priv)) {
+		int err;
+
 		vlan->accel_priv = NULL;
 		err = dev_uc_add(lowerdev, dev->dev_addr);
 		if (err < 0)
-			goto out;
+			return err;
+	}
+
+	return 0;
+}
+
+/* Clean up acceleration or remove unicast address.
+ * Skipped for pass-through ports.
+ */
+static void macvlan_uninit_vlan(struct net_device *dev)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev;
+
+	if (macvlan_passthru(vlan->port))
+		return;
+
+	lowerdev = vlan->lowerdev;
+
+	if (vlan->accel_priv) {
+		lowerdev->netdev_ops->ndo_dfwd_del_station(lowerdev,
+							   vlan->accel_priv);
+		vlan->accel_priv = NULL;
+	} else {
+		dev_uc_del(lowerdev, dev->dev_addr);
 	}
+}
+
+static int macvlan_open(struct net_device *dev)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev;
+	int err;
+
+	err = macvlan_init_vlan(dev);
+	if (err)
+		return err;
+
+	lowerdev = vlan->lowerdev;
 
 	if (dev->flags & IFF_ALLMULTI) {
 		err = dev_set_allmulti(lowerdev, 1);
 		if (err < 0)
-			goto del_unicast;
+			goto uninit;
 	}
 
-	if (dev->flags & IFF_PROMISC) {
+	if (macvlan_should_set_promisc(dev)) {
 		err = dev_set_promiscuity(lowerdev, 1);
 		if (err < 0)
 			goto clear_multi;
 	}
 
-hash_add:
 	macvlan_hash_add(vlan);
 	return 0;
 
 clear_multi:
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(lowerdev, -1);
-del_unicast:
-	if (vlan->accel_priv) {
-		lowerdev->netdev_ops->ndo_dfwd_del_station(lowerdev,
-							   vlan->accel_priv);
-		vlan->accel_priv = NULL;
-	} else {
-		dev_uc_del(lowerdev, dev->dev_addr);
-	}
-out:
+uninit:
+	macvlan_uninit_vlan(dev);
 	return err;
 }
 
@@ -712,21 +750,15 @@ static int macvlan_stop(struct net_device *dev)
 	dev_uc_unsync(lowerdev, dev);
 	dev_mc_unsync(lowerdev, dev);
 
-	if (macvlan_passthru(vlan->port)) {
-		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
-			dev_set_promiscuity(lowerdev, -1);
-		goto hash_del;
-	}
-
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(lowerdev, -1);
 
-	if (dev->flags & IFF_PROMISC)
+	if (macvlan_should_set_promisc(dev))
 		dev_set_promiscuity(lowerdev, -1);
 
-	dev_uc_del(lowerdev, dev->dev_addr);
+	if (!macvlan_passthru(vlan->port))
+		dev_uc_del(lowerdev, dev->dev_addr);
 
-hash_del:
 	macvlan_hash_del(vlan, !dev->dismantle);
 	return 0;
 }