Re: [PATCH net-next 3/4] bridge: Consider the Nearest Customer Bridge group addresses

[Toshiaki Makita <toshiaki.makita1@gmail.com>]

On Mon, 2014-06-09 at 08:52 -0700, Stephen Hemminger wrote:
> On Mon,  9 Jun 2014 20:34:46 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
> > An 802.1ad bridge must forward the Nearest Customer Bridge group addresses.
> >  01-80-C2-00-00-00
> >  01-80-C2-00-00-0B
> >  01-80-C2-00-00-0C
> >  01-80-C2-00-00-0D
> >  01-80-C2-00-00-0F
> > (For details, see IEEE 802.1Q-2011 8.6.3.)
> > 
> > An exception is the br->group_addr, which needs to be passed to the higher
> > layer entity so that STP works.
> > 
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > ---
> >  net/bridge/br_input.c   | 15 ++++++++++++++-
> >  net/bridge/br_private.h | 10 ++++++++++
> >  2 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index 04d6348..b05d419 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -194,13 +194,26 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >  		case 0x00:	/* Bridge Group Address */
> >  			/* If STP is turned off,
> >  			   then must forward to keep loop detection */
> > -			if (p->br->stp_enabled == BR_NO_STP)
> > +			if (p->br->stp_enabled == BR_NO_STP ||
> > +			    (br_vlan_enabled(p->br) &&
> > +			     br_vlan_get_proto(p->br) == htons(ETH_P_8021AD) &&
> > +			     p->br->group_addr[5] != dest[5]))
> >  				goto forward;
> >  			break;
> >  
> >  		case 0x01:	/* IEEE MAC (Pause) */
> >  			goto drop;
> >  
> > +		case 0x0B:
> > +		case 0x0C:
> > +		case 0x0D:
> > +		case 0x0F:
> > +			/* The Nearest Customer Bridge group address */
> > +			if (br_vlan_enabled(p->br) &&
> > +			    br_vlan_get_proto(p->br) == htons(ETH_P_8021AD) &&
> > +			    p->br->group_addr[5] != dest[5])
> > +				goto forward;
> > +			/* fall through */
> >  		default:
> >  			/* Allow selective forwarding for most other protocols */
> >  			if (p->br->group_fwd_mask & (1u << dest[5]))
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index b65fee9..65204c2 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -647,6 +647,11 @@ static inline int br_vlan_enabled(struct net_bridge *br)
> >  {
> >  	return br->vlan_enabled;
> >  }
> > +
> > +static inline __be16 br_vlan_get_proto(struct net_bridge *br)
> > +{
> > +	return br->vlan_proto;
> > +}
> >  #else
> >  static inline bool br_allowed_ingress(struct net_bridge *br,
> >  				      struct net_port_vlans *v,
> > @@ -742,6 +747,11 @@ static inline int br_vlan_enabled(struct net_bridge *br)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline __be16 br_vlan_get_proto(struct net_bridge *br)
> > +{
> > +	return 0;
> > +}
> >  #endif
> >  
> >  /* br_netfilter.c */
> 
> Rather than special casing this around vlan filtering, I would prefer
> the code always forward these packets, or manipulate group_fwd_mask
> to allow it that way.

These addresses must be forwarded only if the bridge is an S-VLAN
bridge. When it is a C-VLAN bridge or a .1D bridge, they may not be
forwarded. So, I don't think we can forward them always.

Using group_fwd_mask is a bit complicated. If we use it to forward them,
user can optionally turn off forwarding ability of those addresses...
but we maybe need another information (named like group_fwd_mask_set)
that indicates which bit is set by user. (We have to set group_fwd_mask
automatically when we set vlan_proto to 88a8.)
Is this way acceptable?

Thanks,
Toshiaki Makita