| Message ID | 1435240213-15179-1-git-send-email-giladb@ezchip.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); >
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
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);