| Message ID | 20190202142935.3090-1-rdong.ge@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: slave: Don't propagate flag changes on down slave interfaces | expand |
Le 2/2/19 à 6:29 AM, Rundong Ge a écrit : > The unbalance of master's promiscuity or allmulti will happen after ifdown > and ifup a slave interface which is in a bridge. > > When we ifdown a slave interface , both the 'dsa_slave_close' and > 'dsa_slave_change_rx_flags' will clear the master's flags. The flags > of master will be decrease twice. > In the other hand, if we ifup the slave interface again, since the > slave's flags were cleared the 'dsa_slave_open' won't set the master's > flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if' > will set the master's flags. The flags of master is increase once. > > Only propagating flag changes when a slave interface is up makes > sure this does not happen. The 'vlan_dev_change_rx_flags' had the > same problem and was fixed, and changes here follows that fix. VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears to do the same thing that you are proposing, so this looks fine to me. Since that is a bugfix, we should probably add: Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support") ? > > Signed-off-by: Rundong Ge <rdong.ge@gmail.com> > --- > net/dsa/slave.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index a3fcc1d..b5e4482 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -140,11 +140,14 @@ static int dsa_slave_close(struct net_device *dev) > static void dsa_slave_change_rx_flags(struct net_device *dev, int change) > { > struct net_device *master = dsa_slave_to_master(dev); > - > - if (change & IFF_ALLMULTI) > - dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1); > - if (change & IFF_PROMISC) > - dev_set_promiscuity(master, dev->flags & IFF_PROMISC ? 1 : -1); > + if (dev->flags & IFF_UP) { > + if (change & IFF_ALLMULTI) > + dev_set_allmulti(master, > + dev->flags & IFF_ALLMULTI ? 1 : -1); > + if (change & IFF_PROMISC) > + dev_set_promiscuity(master, > + dev->flags & IFF_PROMISC ? 1 : -1); > + } > } > > static void dsa_slave_set_rx_mode(struct net_device *dev) >
On Sat, Feb 02, 2019 at 09:05:11AM -0800, Florian Fainelli wrote: > Le 2/2/19 à 6:29 AM, Rundong Ge a écrit : > > The unbalance of master's promiscuity or allmulti will happen after ifdown > > and ifup a slave interface which is in a bridge. > > > > When we ifdown a slave interface , both the 'dsa_slave_close' and > > 'dsa_slave_change_rx_flags' will clear the master's flags. The flags > > of master will be decrease twice. > > In the other hand, if we ifup the slave interface again, since the > > slave's flags were cleared the 'dsa_slave_open' won't set the master's > > flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if' > > will set the master's flags. The flags of master is increase once. > > > > Only propagating flag changes when a slave interface is up makes > > sure this does not happen. The 'vlan_dev_change_rx_flags' had the > > same problem and was fixed, and changes here follows that fix. > > VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears > to do the same thing that you are proposing, so this looks fine to me. > Since that is a bugfix, we should probably add: Hi Rundong, Florian I've seen issues with tcpdump causing the promiscuous count to go negative. I wounder if this will fix that at well? Andrew
Hi Andrew Sorry for the previous mails due to my error mail format setting on browser. I have reproduced the tcpdump issue in my env, but not sure if it is the same issue as you mentioned. Ifdown a slave interface which is under tcpdump capturing will also decrease the master's promiscuous twice. So when I do: a) tcpdump -i ethx b) ifdown ethx c) ifup ethx d) goto a for several times the promiscuous count of master will go negative. Raydon Andrew Lunn <andrew@lunn.ch> 于2019年2月3日周日 上午1:16写道： > > On Sat, Feb 02, 2019 at 09:05:11AM -0800, Florian Fainelli wrote: > > Le 2/2/19 à 6:29 AM, Rundong Ge a écrit : > > > The unbalance of master's promiscuity or allmulti will happen after ifdown > > > and ifup a slave interface which is in a bridge. > > > > > > When we ifdown a slave interface , both the 'dsa_slave_close' and > > > 'dsa_slave_change_rx_flags' will clear the master's flags. The flags > > > of master will be decrease twice. > > > In the other hand, if we ifup the slave interface again, since the > > > slave's flags were cleared the 'dsa_slave_open' won't set the master's > > > flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if' > > > will set the master's flags. The flags of master is increase once. > > > > > > Only propagating flag changes when a slave interface is up makes > > > sure this does not happen. The 'vlan_dev_change_rx_flags' had the > > > same problem and was fixed, and changes here follows that fix. > > > > VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears > > to do the same thing that you are proposing, so this looks fine to me. > > Since that is a bugfix, we should probably add: > > Hi Rundong, Florian > > I've seen issues with tcpdump causing the promiscuous count to go > negative. I wounder if this will fix that at well? > > Andrew
From: Florian Fainelli <f.fainelli@gmail.com> Date: Sat, 2 Feb 2019 09:05:11 -0800 > Le 2/2/19 à 6:29 AM, Rundong Ge a écrit : >> The unbalance of master's promiscuity or allmulti will happen after ifdown >> and ifup a slave interface which is in a bridge. >> >> When we ifdown a slave interface , both the 'dsa_slave_close' and >> 'dsa_slave_change_rx_flags' will clear the master's flags. The flags >> of master will be decrease twice. >> In the other hand, if we ifup the slave interface again, since the >> slave's flags were cleared the 'dsa_slave_open' won't set the master's >> flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if' >> will set the master's flags. The flags of master is increase once. >> >> Only propagating flag changes when a slave interface is up makes >> sure this does not happen. The 'vlan_dev_change_rx_flags' had the >> same problem and was fixed, and changes here follows that fix. > > VLAN code under net/8021q/vlan_dev.c::vlan_dev_change_rx_flags() appears > to do the same thing that you are proposing, so this looks fine to me. > Since that is a bugfix, we should probably add: > > Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol > support") Applied with Fixes tag added, and queued up for -stable. Thanks.
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index a3fcc1d..b5e4482 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -140,11 +140,14 @@ static int dsa_slave_close(struct net_device *dev) static void dsa_slave_change_rx_flags(struct net_device *dev, int change) { struct net_device *master = dsa_slave_to_master(dev); - - if (change & IFF_ALLMULTI) - dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1); - if (change & IFF_PROMISC) - dev_set_promiscuity(master, dev->flags & IFF_PROMISC ? 1 : -1); + if (dev->flags & IFF_UP) { + if (change & IFF_ALLMULTI) + dev_set_allmulti(master, + dev->flags & IFF_ALLMULTI ? 1 : -1); + if (change & IFF_PROMISC) + dev_set_promiscuity(master, + dev->flags & IFF_PROMISC ? 1 : -1); + } } static void dsa_slave_set_rx_mode(struct net_device *dev)
The unbalance of master's promiscuity or allmulti will happen after ifdown and ifup a slave interface which is in a bridge. When we ifdown a slave interface , both the 'dsa_slave_close' and 'dsa_slave_change_rx_flags' will clear the master's flags. The flags of master will be decrease twice. In the other hand, if we ifup the slave interface again, since the slave's flags were cleared the 'dsa_slave_open' won't set the master's flag, only 'dsa_slave_change_rx_flags' that triggered by 'br_add_if' will set the master's flags. The flags of master is increase once. Only propagating flag changes when a slave interface is up makes sure this does not happen. The 'vlan_dev_change_rx_flags' had the same problem and was fixed, and changes here follows that fix. Signed-off-by: Rundong Ge <rdong.ge@gmail.com> --- net/dsa/slave.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)