[PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[Rundong Ge]

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(-)

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)
-- 
1.8.3.1

Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[Florian Fainelli]

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)
> 


-- 
Florian

Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[Andrew Lunn]

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

Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[戈润栋]

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

Re: [PATCH] net: dsa: slave: Don't propagate flag changes on down slave interfaces

[David Miller]

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.