| Message ID | 1381910836-718-3-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/16/2013 04:07 AM, Toshiaki Makita 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@lab.ntt.co.jp> Reviewed-by: Vlad Yasevich <vyasevic@redhat.com> > --- > 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); > > - /* Frame did not have a tag. See if pvid is set > - * on this port. That tells us which vlan untagged > - * traffic belongs to. > + /* Frame had a tag with VID 0 or did not have a tag. > + * See if pvid is set on this port. That tells us which > + * vlan untagged or priority-tagged traffic belongs to. > */ > if (pvid == VLAN_N_VID) > return false; > > - /* PVID is set on this port. Any untagged ingress > - * frame is considered to belong to this vlan. > + /* PVID is set on this port. Any untagged or priority-tagged > + * ingress frame is considered to belong to this vlan. > */ > - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid); > + if (likely(err)) > + /* Untagged Frame. */ > + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid); > + else > + /* Priority-tagged Frame. > + * At this point, We know that skb->vlan_tci had > + * VLAN_TAG_PRESENT bit and its VID field was 0x000. > + * We update only VID field and preserve PCP field. > + */ > + skb->vlan_tci |= pvid; > + > return true; > } > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 16 Oct 2013 17:07:14 +0900 Toshiaki Makita <makita.toshiaki@lab.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@lab.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. Also, does this still work if CONFIG_BRIDGE_VLAN_FILTERING is disabled? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/16/2013 11:55 AM, Stephen Hemminger wrote: > On Wed, 16 Oct 2013 17:07:14 +0900 > Toshiaki Makita <makita.toshiaki@lab.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@lab.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. 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. > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lab.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@lab.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; } 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lab.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@lab.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. -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lab.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@lab.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); - /* Frame did not have a tag. See if pvid is set - * on this port. That tells us which vlan untagged - * traffic belongs to. + /* Frame had a tag with VID 0 or did not have a tag. + * See if pvid is set on this port. That tells us which + * vlan untagged or priority-tagged traffic belongs to. */ if (pvid == VLAN_N_VID) return false; - /* PVID is set on this port. Any untagged ingress - * frame is considered to belong to this vlan. + /* PVID is set on this port. Any untagged or priority-tagged + * ingress frame is considered to belong to this vlan. */ - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid); + if (likely(err)) + /* Untagged Frame. */ + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), pvid); + else + /* Priority-tagged Frame. + * At this point, We know that skb->vlan_tci had + * VLAN_TAG_PRESENT bit and its VID field was 0x000. + * We update only VID field and preserve PCP field. + */ + skb->vlan_tci |= pvid; + return true; }
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@lab.ntt.co.jp> --- net/bridge/br_vlan.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)