From: Amos Kong <akong@redhat•com>
To: Toshiaki Makita <makita.toshiaki@lab•ntt.co.jp>
Cc: Vlad Yasevich <vyasevic@redhat•com>,
netdev@vger•kernel.org, shemminger@vyatta•com
Subject: Re: [PATCH] bridge: pass correct vlan id to multicast code
Date: Tue, 29 Oct 2013 21:39:15 +0800 [thread overview]
Message-ID: <20131029133915.GA799@amosk.info> (raw)
In-Reply-To: <1383044915.3518.41.camel@ubuntu-vm-makita>
On Tue, Oct 29, 2013 at 08:08:35PM +0900, Toshiaki Makita wrote:
> 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
Thanks all your explanation, I'm ok with the patch.
--
Amos.
next prev parent reply other threads:[~2013-10-29 13:39 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-28 19:45 [PATCH] bridge: pass correct vlan id to multicast code Vlad Yasevich
2013-10-29 2:36 ` Amos Kong
2013-10-29 11:08 ` Toshiaki Makita
2013-10-29 13:39 ` Amos Kong [this message]
2013-10-29 12:45 ` Vlad Yasevich
2013-10-29 15:00 ` Vlad Yasevich
2013-10-29 21:40 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131029133915.GA799@amosk.info \
--to=akong@redhat$(echo .)com \
--cc=makita.toshiaki@lab$(echo .)ntt.co.jp \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shemminger@vyatta$(echo .)com \
--cc=vyasevic@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox