2019-06-03 03:24:46

by Young Xiao

Subject: [PATCH] ipvlan: Don't propagate IFF_ALLMULTI changes on down interfaces.

Clearing the IFF_ALLMULTI flag on a down interface could cause an allmulti
overflow on the underlying interface.

Attempting the set IFF_ALLMULTI on the underlying interface would cause an
error and the log message:

"allmulti touches root, set allmulti failed."

Signed-off-by: Young Xiao <[email protected]>
---
drivers/net/ipvlan/ipvlan_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index bbeb162..523bb83 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -242,8 +242,10 @@ static void ipvlan_change_rx_flags(struct net_device *dev, int change)
struct ipvl_dev *ipvlan = netdev_priv(dev);
struct net_device *phy_dev = ipvlan->phy_dev;

- if (change & IFF_ALLMULTI)
- dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI? 1 : -1);
+ if (dev->flags & IFF_UP) {
+ if (change & IFF_ALLMULTI)
+ dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
+ }
}

static void ipvlan_set_multicast_mac_filter(struct net_device *dev)
--
2.7.4

2019-06-03 07:55:46

by Xin Long

Subject: Re: [PATCH] ipvlan: Don't propagate IFF_ALLMULTI changes on down interfaces.

On Mon, Jun 3, 2019 at 11:22 AM Young Xiao <[email protected]> wrote:
>
> Clearing the IFF_ALLMULTI flag on a down interface could cause an allmulti
> overflow on the underlying interface.
>
> Attempting the set IFF_ALLMULTI on the underlying interface would cause an
> error and the log message:
>
> "allmulti touches root, set allmulti failed."
s/root/roof

I guess this patch was inspired by:

commit bbeb0eadcf9fe74fb2b9b1a6fea82cd538b1e556
Author: Peter Christensen <[email protected]>
Date: Thu May 8 11:15:37 2014 +0200

macvlan: Don't propagate IFF_ALLMULTI changes on down interfaces.

I could trigger this error on macvlan prior to this patch with:

# ip link add mymacvlan1 link eth2 type macvlan mode bridge
# ip link set mymacvlan1 up
# ip link set mymacvlan1 allmulticast on
# ip link set mymacvlan1 down
# ip link set mymacvlan1 allmulticast off
# ip link set mymacvlan1 allmulticast on

but not on ipvlan, could you?

macvlan had this problem, as lowerdev's allmulticast is cleared when doing
dev_stop and set when doing dev_open, which doesn't happen on ipvlan.

So I'd think this patch fixes nothing unless you want to add
dev_set_allmulti(1/-1) in ipvlan_open/stop(), but that's another topic.

did I miss something?

>
> Signed-off-by: Young Xiao <[email protected]>
> ---
> drivers/net/ipvlan/ipvlan_main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index bbeb162..523bb83 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -242,8 +242,10 @@ static void ipvlan_change_rx_flags(struct net_device *dev, int change)
> struct ipvl_dev *ipvlan = netdev_priv(dev);
> struct net_device *phy_dev = ipvlan->phy_dev;
>
> - if (change & IFF_ALLMULTI)
> - dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI? 1 : -1);
> + if (dev->flags & IFF_UP) {
> + if (change & IFF_ALLMULTI)
> + dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> + }
> }
>
> static void ipvlan_set_multicast_mac_filter(struct net_device *dev)
> --
> 2.7.4
>

2019-06-03 08:29:49

by Young Xiao

Subject: Re: [PATCH] ipvlan: Don't propagate IFF_ALLMULTI changes on down interfaces.

Based on your explanation, I found that the patch I submitted is useless.
Great thanks and sorry for the trouble.

On Mon, Jun 3, 2019 at 3:54 PM Xin Long <[email protected]> wrote:
>
> On Mon, Jun 3, 2019 at 11:22 AM Young Xiao <[email protected]> wrote:
> >
> > Clearing the IFF_ALLMULTI flag on a down interface could cause an allmulti
> > overflow on the underlying interface.
> >
> > Attempting the set IFF_ALLMULTI on the underlying interface would cause an
> > error and the log message:
> >
> > "allmulti touches root, set allmulti failed."
> s/root/roof
>
> I guess this patch was inspired by:
>
> commit bbeb0eadcf9fe74fb2b9b1a6fea82cd538b1e556
> Author: Peter Christensen <[email protected]>
> Date: Thu May 8 11:15:37 2014 +0200
>
> macvlan: Don't propagate IFF_ALLMULTI changes on down interfaces.
>
> I could trigger this error on macvlan prior to this patch with:
>
> # ip link add mymacvlan1 link eth2 type macvlan mode bridge
> # ip link set mymacvlan1 up
> # ip link set mymacvlan1 allmulticast on
> # ip link set mymacvlan1 down
> # ip link set mymacvlan1 allmulticast off
> # ip link set mymacvlan1 allmulticast on
>
> but not on ipvlan, could you?
>
> macvlan had this problem, as lowerdev's allmulticast is cleared when doing
> dev_stop and set when doing dev_open, which doesn't happen on ipvlan.
>
> So I'd think this patch fixes nothing unless you want to add
> dev_set_allmulti(1/-1) in ipvlan_open/stop(), but that's another topic.
>
> did I miss something?
>
> >
> > Signed-off-by: Young Xiao <[email protected]>
> > ---
> > drivers/net/ipvlan/ipvlan_main.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> > index bbeb162..523bb83 100644
> > --- a/drivers/net/ipvlan/ipvlan_main.c
> > +++ b/drivers/net/ipvlan/ipvlan_main.c
> > @@ -242,8 +242,10 @@ static void ipvlan_change_rx_flags(struct net_device *dev, int change)
> > struct ipvl_dev *ipvlan = netdev_priv(dev);
> > struct net_device *phy_dev = ipvlan->phy_dev;
> >
> > - if (change & IFF_ALLMULTI)
> > - dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI? 1 : -1);
> > + if (dev->flags & IFF_UP) {
> > + if (change & IFF_ALLMULTI)
> > + dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> > + }
> > }
> >
> > static void ipvlan_set_multicast_mac_filter(struct net_device *dev)
> > --
> > 2.7.4
> >