Thanks. Please use the following Fixes tag:
Fixes: bbeb0eadcf9f ("macvlan: Don't propagate IFF_ALLMULTI changes
on down interfaces.")
Signed-off-by: Prasad Koya [off-list ref]
On Fri, Oct 30, 2026 at 3:15 AM Simon Horman [off-list ref] wrote:quoted hunk ↗ jump to hunk
On Mon, Oct 26, 2026 at 07:46:35PM -0700, Prasad Koya wrote:
quoted
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.
quoted
Signed-off-by: Prasad Koya <redacted>
---
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@kernel.org>
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;
}