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

[Prasad Koya]

  Thanks. Please use the following Fixes tag:

  Fixes: bbeb0eadcf9f ("macvlan: Don't propagate IFF_ALLMULTI changes
on down interfaces.")
  Signed-off-by: Prasad Koya <prasad@xxxxxxxxxx>


On Fri, Oct 30, 2026 at 3:15 AM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> 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;
>  }