From mboxrd@z Thu Jan 1 00:00:00 1970 From: Toshiaki Makita Subject: Re: [PATCH] bridge: pass correct vlan id to multicast code Date: Tue, 29 Oct 2013 20:08:35 +0900 Message-ID: <1383044915.3518.41.camel@ubuntu-vm-makita> References: <1382989507-23061-1-git-send-email-vyasevic@redhat.com> <20131029023646.GA2795@amosk.info> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Vlad Yasevich , netdev@vger.kernel.org, shemminger@vyatta.com To: Amos Kong Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:57622 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751804Ab3J2LIz (ORCPT ); Tue, 29 Oct 2013 07:08:55 -0400 In-Reply-To: <20131029023646.GA2795@amosk.info> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > 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 > > --- > > 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); > > ... >