[PATCH net] dsa: fix promiscuity leak on slave dev open error

[PATCH net] dsa: fix promiscuity leak on slave dev open error

[giladb]

From: Gilad Ben-Yossef <gilad@benyossef.com>

DSA master netdev promiscuity counter was not being properly
decremented on slave device open error path.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: Gilad Ben-Yossef <giladb@ezchip.com>
CC: David S. Miller <davem@davemloft.net>
CC: Florian Fainelli <f.fainelli@gmail.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Andrew Lunn <andrew@lunn.ch>
CC: Scott Feldman <sfeldma@gmail.com>
---
 net/dsa/slave.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 04ffad3..0917123 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -112,7 +112,7 @@ static int dsa_slave_open(struct net_device *dev)
 
 clear_promisc:
 	if (dev->flags & IFF_PROMISC)
-		dev_set_promiscuity(master, 0);
+		dev_set_promiscuity(master, -1);
 clear_allmulti:
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(master, -1);
-- 
1.7.1

Re: [PATCH net] dsa: fix promiscuity leak on slave dev open error

[Andrew Lunn]

On Thu, Jun 25, 2015 at 04:50:13PM +0300, giladb@ezchip.com wrote:
> From: Gilad Ben-Yossef <gilad@benyossef.com>
> 
> DSA master netdev promiscuity counter was not being properly
> decremented on slave device open error path.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> CC: Gilad Ben-Yossef <giladb@ezchip.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: Florian Fainelli <f.fainelli@gmail.com>
> CC: Guenter Roeck <linux@roeck-us.net>
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Scott Feldman <sfeldma@gmail.com>

Hi Gilad

Acked-by: Andrew Lunn <andrew@lunn.ch>

FYI: I grep'ed the whole kernel. This seems to be the only place 0 is
passed to dev_set_promiscuity(), i.e. the same bug is not present
anywhere else.

Thanks
	Andrew

> ---
>  net/dsa/slave.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 04ffad3..0917123 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -112,7 +112,7 @@ static int dsa_slave_open(struct net_device *dev)
>  
>  clear_promisc:
>  	if (dev->flags & IFF_PROMISC)
> -		dev_set_promiscuity(master, 0);
> +		dev_set_promiscuity(master, -1);
>  clear_allmulti:
>  	if (dev->flags & IFF_ALLMULTI)
>  		dev_set_allmulti(master, -1);
> -- 
> 1.7.1
> 

Re: [PATCH net] dsa: fix promiscuity leak on slave dev open error

[Florian Fainelli]

On 25/06/15 06:50, giladb@ezchip.com wrote:
> From: Gilad Ben-Yossef <gilad@benyossef.com>
> 
> DSA master netdev promiscuity counter was not being properly
> decremented on slave device open error path.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> CC: Gilad Ben-Yossef <giladb@ezchip.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: Florian Fainelli <f.fainelli@gmail.com>
> CC: Guenter Roeck <linux@roeck-us.net>
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Scott Feldman <sfeldma@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  net/dsa/slave.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 04ffad3..0917123 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -112,7 +112,7 @@ static int dsa_slave_open(struct net_device *dev)
>  
>  clear_promisc:
>  	if (dev->flags & IFF_PROMISC)
> -		dev_set_promiscuity(master, 0);
> +		dev_set_promiscuity(master, -1);
>  clear_allmulti:
>  	if (dev->flags & IFF_ALLMULTI)
>  		dev_set_allmulti(master, -1);
> 


-- 
Florian

Re: [PATCH net] dsa: fix promiscuity leak on slave dev open error

[Gilad Ben-Yossef]

On Fri, Jun 26, 2015 at 6:04 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Jun 25, 2015 at 04:50:13PM +0300, giladb@ezchip.com wrote:
>> From: Gilad Ben-Yossef <gilad@benyossef.com>
>>
>> DSA master netdev promiscuity counter was not being properly
>> decremented on slave device open error path.
>>
...

> Acked-by: Andrew Lunn <andrew@lunn.ch>

Thank you Andrew.
>
> FYI: I grep'ed the whole kernel. This seems to be the only place 0 is
> passed to dev_set_promiscuity(), i.e. the same bug is not present
> anywhere else.
>


It has occurred to me that dev_set_promiscuity() and its brethren
dev_set_allmulti() may not be the best of interfaces:

- On cursory inspection of code using these function their name
implies the value of the relevant counter is set to the value passed
as parameter, not incremented by it.
- No caller I've managed to spot passes anything but -1 or 1

It seems an interface of

int dev_set_promiscuity(struct net_device *dev, bool on);

int dev_set_allmulti(struct net_device *dev, bool on);

would be as functional (for current users), more readable and less
error prone but I am not sure such a tiny problem (if you can call
this a problem) is worth the churn...


Gilad






-- 
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@benyossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH net] dsa: fix promiscuity leak on slave dev open error

[Andrew Lunn]

> It has occurred to me that dev_set_promiscuity() and its brethren
> dev_set_allmulti() may not be the best of interfaces:
> 
> - On cursory inspection of code using these function their name
> implies the value of the relevant counter is set to the value passed
> as parameter, not incremented by it.
> - No caller I've managed to spot passes anything but -1 or 1
> 
> It seems an interface of
> 
> int dev_set_promiscuity(struct net_device *dev, bool on);
> 
> int dev_set_allmulti(struct net_device *dev, bool on);

on suggests it is an absolute, when in fact you are passing an
increment, so i don't think it is much of an improvement.

Adding #define, PROMISC_INC and PROMISC_DEC might be clearer, and
since this is not a fast path, you could consider parameter
validation. Or dev_get_promiscuity(), det_put_promiscuity().
 
> would be as functional (for current users), more readable and less
> error prone but I am not sure such a tiny problem (if you can call
> this a problem) is worth the churn...

There was only one instance of it wrong, so it is not a very big
problem. I would say it is not worth the churn.

	 Andrew

Re: [PATCH net] dsa: fix promiscuity leak on slave dev open error

[David Miller]

From: <giladb@ezchip.com>
Date: Thu, 25 Jun 2015 16:50:13 +0300

> From: Gilad Ben-Yossef <gilad@benyossef.com>
> 
> DSA master netdev promiscuity counter was not being properly
> decremented on slave device open error path.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Applied.

[PATCH net] dsa: fix promiscuity leak on slave dev open error

[Gilad Ben-Yossef]

DSA master netdev promiscuity counter was not being properly
decremented on slave device open error path.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: Gilad Ben-Yossef <giladb@ezchip.com>
CC: David S. Miller <davem@davemloft.net>
CC: Florian Fainelli <f.fainelli@gmail.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Andrew Lunn <andrew@lunn.ch>
CC: Scott Feldman <sfeldma@gmail.com>
---
 net/dsa/slave.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 04ffad3..0917123 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -112,7 +112,7 @@ static int dsa_slave_open(struct net_device *dev)
 
 clear_promisc:
 	if (dev->flags & IFF_PROMISC)
-		dev_set_promiscuity(master, 0);
+		dev_set_promiscuity(master, -1);
 clear_allmulti:
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(master, -1);
-- 
1.7.1