Linux Ethernet Bridging - Jun 2014 - [Bridge] [PATCH v2 net-next] bridge: Fix incorrect judgment of promisc

br_manage_promisc() incorrectly expects br_auto_port() to return only 0
or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK.

Signed-off-by: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
---
 net/bridge/br_if.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index a08d2b8..3eca3fd 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -153,7 +153,8 @@ void br_manage_promisc(struct net_bridge *br)
 			 * This lets us disable promiscuous mode and write
 			 * this config to hw.
 			 */
-			if (br->auto_cnt <= br_auto_port(p))
+			if (br->auto_cnt == 0 ||
+			    (br->auto_cnt == 1 && br_auto_port(p)))
 				br_port_clear_promisc(p);
 			else
 				br_port_set_promisc(p);
-- 
1.8.1.2

On Thu, Jun 05, 2014 at 08:53:32PM +0900, Toshiaki Makita
wrote:
> br_manage_promisc() incorrectly expects br_auto_port() to return only 0
> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
> ---
>  net/bridge/br_if.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index a08d2b8..3eca3fd 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -153,7 +153,8 @@ void br_manage_promisc(struct net_bridge *br)
>  			 * This lets us disable promiscuous mode and write
>  			 * this config to hw.
>  			 */
> -			if (br->auto_cnt <= br_auto_port(p))
> +			if (br->auto_cnt == 0 ||
> +			    (br->auto_cnt == 1 && br_auto_port(p)))
>  				br_port_clear_promisc(p);
>  			else
>  				br_port_set_promisc(p);
It's all a nasty side-effect of using macros IMHO.

How about we just make these inline functions returning bool?

The bugfix will fall out naturally.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>

Warning: untested.


diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 53d6e32..5818dd2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -200,8 +200,15 @@ struct net_bridge_port
 #endif
 };
 
-#define br_auto_port(p) ((p)->flags & BR_AUTO_MASK)
-#define br_promisc_port(p) ((p)->flags & BR_PROMISC)
+static inline bool br_auto_port(struct net_bridge_port *p)
+{
+	return p->flags & BR_AUTO_MASK;
+}
+
+static inline bool br_promisc_port(struct net_bridge_port *p)
+{
+	return p->flags & BR_PROMISC;
+}
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Thu,  5 Jun 2014 20:53:32 +0900
> br_manage_promisc() incorrectly expects br_auto_port() to return only 0
> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
I'm applying this as-is for now, even though I saw the other
suggestions in this thread (which BTW didn't get picked up by
patchwork, maybe some of you dropped the Message-Id in your replies by
accident).

From: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
Date: Thu,  5 Jun 2014 20:53:32 +0900
> br_manage_promisc() incorrectly expects br_auto_port() to return only 0
> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
I'm applying this as-is for now, even though I saw the other
suggestions in this thread (which BTW didn't get picked up by
patchwork, maybe some of you dropped the Message-Id in your replies by
accident).

From: David Miller [mailto:davem@davemloft.net]
> > br_manage_promisc() incorrectly expects br_auto_port() to return only
0
> > or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK.
> >
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> 
> I'm applying this as-is for now, even though I saw the other
> suggestions in this thread (which BTW didn't get picked up by
> patchwork, maybe some of you dropped the Message-Id in your replies by
> accident).
I don't have a problem with that.
The condition looks odd, but it is enabling promiscuous mode
if any other ports are in 'auto' mode.
Possibly the comment above made that clear, but it was truncated
in the diffs.

	David