Re: [PATCH] bridge: pass correct vlan id to multicast code

[Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>]

On Tue, 2013-10-29 at 10:36 +0800, Amos Kong wrote:
> On Mon, Oct 28, 2013 at 03:45:07PM -0400, Vlad Yasevich wrote:
> > Currently multicast code attempts to extrace the vlan id from
> > the skb even when vlan filtering is disabled.  This can lead
> > to mdb entries being created with the wrong vlan id.
> > Pass the already extracted vlan id to the multicast
> > filtering code to make the correct id is used in
> > creation as well as lookup.

Thanks!

Acked-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

>  
> Hi Vlad,
> 
> Can we just update br_vlan_get_tag() to set vid to 0 if dev->vlan is
> disabled? I guess it would effect br_handle_local_finish().

br_handle_local_finish() looks also buggy.
But adding vlan enabled checking would not fix it completely because
vlan_bitmap and PVID are not taken into account in that function.

Since we cannot pass vid as an argument from br_dev_xmit() to
br_handle_[local/frame]_finish() because of NF_HOOK,
br_handle_local_finish() seems to have to check vlan_enabled,
vlan_bitmap, and pvid by itself.

IMHO it can be addressed by another patch.

> 
> > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > ---
> >  net/bridge/br_device.c    |  2 +-
> >  net/bridge/br_input.c     |  2 +-
> >  net/bridge/br_multicast.c | 44 +++++++++++++++++++-------------------------
> >  net/bridge/br_private.h   |  6 ++++--
> >  4 files changed, 25 insertions(+), 29 deletions(-)
> 
> ...
> 
> > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> > index 8b0b610..686284f 100644
> > --- a/net/bridge/br_multicast.c
> > +++ b/net/bridge/br_multicast.c
> > @@ -947,7 +947,8 @@ void br_multicast_disable_port(struct net_bridge_port *port)
> >  
> >  static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
> >  					 struct net_bridge_port *port,
> > -					 struct sk_buff *skb)
> > +					 struct sk_buff *skb,
> > +					 u16 vid)
> >  {
> >  	struct igmpv3_report *ih;
> >  	struct igmpv3_grec *grec;
> > @@ -957,12 +958,10 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
> >  	int type;
> >  	int err = 0;
> >  	__be32 group;
> > -	u16 vid = 0;
> >  
> >  	if (!pskb_may_pull(skb, sizeof(*ih)))
> >  		return -EINVAL;
> >  
> > -	br_vlan_get_tag(skb, &vid);
> 
> After applied the patch, we always use vid in br_dev_xmit()->br_allowed_ingress(),
> is it possible that the vlan of bridge is re-enabled when other
> changed functions are called?
> 
> We can just add a enabled checking before this kind of br_vlan_get_tag()?
> 
> if (!br->vlan_enabled)
>     br_vlan_get_tag(skb2, &vid);

Maybe this leads to a wrong way to update mdb in some cases like
  Vlan_filtering is disabled (by default).
  Add some vids we want to allow.
  Receive a frame whose vid wouldn't be allowed with vlan_filtering enabled.
  The frame passes br_allowed_ingress().
  Enable vlan_filtering.
  The frame reaches br_ip4_multicast_igmp3_report().
  Mdb is updated with disabled vid.


Thanks,

Toshiaki Makita

> 
> 
> >  	ih = igmpv3_report_hdr(skb);
> >  	num = ntohs(ih->ngrec);
> >  	len = sizeof(*ih);
> 
> ...
>