| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Oct 2013 23:01:11 +0900
From: Toshiaki Makita <toshiaki.makita1@...il.com>
To: vyasevic@...hat.com
Cc: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
Stephen Hemminger <stephen@...workplumber.org>,
"David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
Fernando Luis Vazquez Cao <fernando_b1@....ntt.co.jp>
Subject: Re: [PATCH v2 net 2/4] bridge: Apply the PVID to priority-tagged
frames
On Thu, 2013-10-17 at 13:39 -0400, Vlad Yasevich wrote:
> On 10/17/2013 08:14 AM, Toshiaki Makita wrote:
> > On Wed, 2013-10-16 at 12:16 -0400, Vlad Yasevich wrote:
> >> On 10/16/2013 11:55 AM, Stephen Hemminger wrote:
> >>> On Wed, 16 Oct 2013 17:07:14 +0900
> >>> Toshiaki Makita <makita.toshiaki@....ntt.co.jp> wrote:
> >>>
> >>>> IEEE 802.1Q says that when we receive priority-tagged (VID 0) frames
> >>>> use the PVID for the port as its VID.
> >>>> (See IEEE 802.1Q-2011 6.9.1 and Table 9-2)
> >>>>
> >>>> Apply the PVID to not only untagged frames but also priority-tagged frames.
> >>>>
> >>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> >>>> ---
> >>>> net/bridge/br_vlan.c | 27 ++++++++++++++++++++-------
> >>>> 1 file changed, 20 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >>>> index 21b6d21..5a9c44a 100644
> >>>> --- a/net/bridge/br_vlan.c
> >>>> +++ b/net/bridge/br_vlan.c
> >>>> @@ -189,6 +189,8 @@ out:
> >>>> bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>>> struct sk_buff *skb, u16 *vid)
> >>>> {
> >>>> + int err;
> >>>> +
> >>>> /* If VLAN filtering is disabled on the bridge, all packets are
> >>>> * permitted.
> >>>> */
> >>>> @@ -201,20 +203,31 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> >>>> if (!v)
> >>>> return false;
> >>>>
> >>>> - if (br_vlan_get_tag(skb, vid)) {
> >>>> + err = br_vlan_get_tag(skb, vid);
> >>>> + if (!*vid) {
> >>>> u16 pvid = br_get_pvid(v);
> >>>
> >>> Ok, but it looks like br_vlan_get_tag() could be cleaner if it just returned
> >>> the tag, and there was another br_vlan_tag_present() function.
> >
> > Thank you for reviewing.
> > I agree with you.
> > I had been afraid that if it affects other codes because
> > br_vlan_get_tag() is used in many places else, but now I have decided
> > not to hesitate to change its signature and behavior.
> >
> >>
> >> I was just thinking about that as well. If we make br_vlan_get_tag()
> >> return either the actual tag (if the packet is tagged), or the pvid
> >> if (untagged/prio_tagged), then we can skp most of this.
> >
> > Hmm... maybe I don't fully understand you.
> >
> > Is what you intend something like
> > br_allowed_ingress(...) {
> > ...
> > vid = br_vlan_get_tag(skb, v);
> > if (!tagged(skb)) put_tag(skb, vid); /* untagged */
> > else if (!get_vid(skb)) update_vid(skb, vid); /* prio_tagged */
> > ...
> > }
> >
> > br_vlan_get_tag(skb, v) {
> > if (tagged(skb)) {
> > vid = get_vid(skb);
> > if (!vid) return get_pvid(v); /* prio_tagged */
> > return vid;
> > }
> > return get_pvid(v); /* untagged */
> > }
> >
> > This needs double check for prio_tagged at br_allowed_ingress() and
> > br_vlan_get_tag().
> >
> > Or if we modify skb->vlan_tci at br_vlan_get_tag(), isn't it a little
> > dangerous to other codes that use this function in order to just get
> > vid?
> >
> > I am thinking it makes things simple that br_vlan_get_tag() returns 0 if
> > (untagged/prio_tagged).
> >
> > br_allowed_ingress(...) {
> > ...
> > vid = br_vlan_get_tag(skb);
> > if (!vid) {
> > vid = get_pvid(v);
> > if (!tagged(skb)) put_tag(skb, vid);/* untagged */
> > else update_vid(skb, vid); /* prio_tagged */
> > }
> > ...
> > }
> >
> > br_vlan_get_tag(skb) {
> > if (tagged(skb)) return get_vid(skb);
> > return 0;
> > }
>
> With this you end up checking if the patcket is tagged quite a lot of times.
>
> What I am thinking is that once we perform a get_tag, we should get
> the vlan tag that the current packet belongs to. We can then safely
> use that tag everywhere and not have to worry too much about it.
>
> We can pass that tag to br_allowed_ingress to verify that it is
> permitted to enter.
>
> You made a valid point about multicast code using br_vlan_get_tag
> incorrectly and I plan on addressing that.
>
> As it is, the current series addresses bugs in the implementation
> that should be fixed.
>
> We can make the code better/nicer as a next step.
OK, you seem to have a better idea to avoid checking if the packet is
tagged many times.
If this patch series is acceptable just as a bug fix, I'll wait for your
proposal of improvement and fixing wrong multicast codes next time.
Thanks,
Toshiaki Makita
>
> -vlad
>
> >
> > Thanks,
> >
> > Toshiaki Makita
> >
> >>
> >>>
> >>> Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled?
> >>
> >> Yes. br_allowed_ingress becomes an inline if the config option is disabled.
> >>
> >> -vlad
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists