| Message ID | 20220307110548.812455-1-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c43a920a5cd26511059751f594c3ac05f9a6125 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: tag_dsa: Fix tx from VLAN uppers on non-filtering bridges | expand |
On Mon, Mar 07, 2022 at 12:05:48PM +0100, Tobias Waldekranz wrote: > In this situation (VLAN filtering disabled on br0): > > br0.10 > / > br0 > / \ > swp0 swp1 > > When a frame is transmitted from the VLAN upper, the bridge will send > it down to one of the switch ports with forward offloading > enabled. This will cause tag_dsa to generate a FORWARD tag. Before > this change, that tag would have it's VID set to 10, even though VID > 10 is not loaded in the VTU. > > Before the blamed commit, the frame would trigger a VTU miss and be > forwarded according to the PVT configuration. Now that all fabric > ports are in 802.1Q secure mode, the frame is dropped instead. > > Therefore, restrict the condition under which we rewrite an 802.1Q tag > to a DSA tag. On standalone port's, reuse is always safe since we will > always generate FROM_CPU tags in that case. For bridged ports though, > we must ensure that VLAN filtering is enabled, which in turn > guarantees that the VID in question is loaded into the VTU. > > Fixes: d352b20f4174 ("net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports") > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> Thanks Tobias Tested-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Mon, Mar 07, 2022 at 12:05:48PM +0100, Tobias Waldekranz wrote: > In this situation (VLAN filtering disabled on br0): > > br0.10 > / > br0 > / \ > swp0 swp1 > > When a frame is transmitted from the VLAN upper, the bridge will send > it down to one of the switch ports with forward offloading > enabled. This will cause tag_dsa to generate a FORWARD tag. Before > this change, that tag would have it's VID set to 10, even though VID > 10 is not loaded in the VTU. > > Before the blamed commit, the frame would trigger a VTU miss and be > forwarded according to the PVT configuration. Now that all fabric > ports are in 802.1Q secure mode, the frame is dropped instead. > > Therefore, restrict the condition under which we rewrite an 802.1Q tag > to a DSA tag. On standalone port's, reuse is always safe since we will > always generate FROM_CPU tags in that case. For bridged ports though, > we must ensure that VLAN filtering is enabled, which in turn > guarantees that the VID in question is loaded into the VTU. > > Fixes: d352b20f4174 ("net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports") > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > net/dsa/tag_dsa.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c > index c8b4bbd46191..e4b6e3f2a3db 100644 > --- a/net/dsa/tag_dsa.c > +++ b/net/dsa/tag_dsa.c > @@ -127,6 +127,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, > u8 extra) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > + struct net_device *br_dev; > u8 tag_dev, tag_port; > enum dsa_cmd cmd; > u8 *dsa_header; > @@ -149,7 +150,16 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, > tag_port = dp->index; > } > > - if (skb->protocol == htons(ETH_P_8021Q)) { > + br_dev = dsa_port_bridge_dev_get(dp); > + > + /* If frame is already 802.1Q tagged, we can convert it to a DSA > + * tag (avoiding a memmove), but only if the port is standalone > + * (in which case we always send FROM_CPU) or if the port's > + * bridge has VLAN filtering enabled (in which case the CPU port > + * will be a member of the VLAN). > + */ > + if (skb->protocol == htons(ETH_P_8021Q) && > + (!br_dev || br_vlan_enabled(br_dev))) { Conservative patch. If !br_dev, we could/should inject using MV88E6XXX_VID_STANDALONE. But since we use FROM_CPU, the classified VLAN probably does not make a difference that I can see, so there is no reason to change this now (and certainly not in the same patch). Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > if (extra) { > skb_push(skb, extra); > dsa_alloc_etype_header(skb, extra); > @@ -166,10 +176,9 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, > dsa_header[2] &= ~0x10; > } > } else { > - struct net_device *br = dsa_port_bridge_dev_get(dp); > u16 vid; > > - vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE; > + vid = br_dev ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE; > > skb_push(skb, DSA_HLEN + extra); > dsa_alloc_etype_header(skb, DSA_HLEN + extra); > -- > 2.25.1 >
Hello: This patch was applied to netdev/net-next.git (master) by Paolo Abeni <pabeni@redhat.com>: On Mon, 7 Mar 2022 12:05:48 +0100 you wrote: > In this situation (VLAN filtering disabled on br0): > > br0.10 > / > br0 > / \ > swp0 swp1 > > [...] Here is the summary with links: - [net-next] net: dsa: tag_dsa: Fix tx from VLAN uppers on non-filtering bridges https://git.kernel.org/netdev/net-next/c/6c43a920a5cd You are awesome, thank you!
On Tue, Mar 08, 2022 at 11:36, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Mar 07, 2022 at 12:05:48PM +0100, Tobias Waldekranz wrote: >> In this situation (VLAN filtering disabled on br0): >> >> br0.10 >> / >> br0 >> / \ >> swp0 swp1 >> >> When a frame is transmitted from the VLAN upper, the bridge will send >> it down to one of the switch ports with forward offloading >> enabled. This will cause tag_dsa to generate a FORWARD tag. Before >> this change, that tag would have it's VID set to 10, even though VID >> 10 is not loaded in the VTU. >> >> Before the blamed commit, the frame would trigger a VTU miss and be >> forwarded according to the PVT configuration. Now that all fabric >> ports are in 802.1Q secure mode, the frame is dropped instead. >> >> Therefore, restrict the condition under which we rewrite an 802.1Q tag >> to a DSA tag. On standalone port's, reuse is always safe since we will >> always generate FROM_CPU tags in that case. For bridged ports though, >> we must ensure that VLAN filtering is enabled, which in turn >> guarantees that the VID in question is loaded into the VTU. >> >> Fixes: d352b20f4174 ("net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports") >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> net/dsa/tag_dsa.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c >> index c8b4bbd46191..e4b6e3f2a3db 100644 >> --- a/net/dsa/tag_dsa.c >> +++ b/net/dsa/tag_dsa.c >> @@ -127,6 +127,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, >> u8 extra) >> { >> struct dsa_port *dp = dsa_slave_to_port(dev); >> + struct net_device *br_dev; >> u8 tag_dev, tag_port; >> enum dsa_cmd cmd; >> u8 *dsa_header; >> @@ -149,7 +150,16 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, >> tag_port = dp->index; >> } >> >> - if (skb->protocol == htons(ETH_P_8021Q)) { >> + br_dev = dsa_port_bridge_dev_get(dp); >> + >> + /* If frame is already 802.1Q tagged, we can convert it to a DSA >> + * tag (avoiding a memmove), but only if the port is standalone >> + * (in which case we always send FROM_CPU) or if the port's >> + * bridge has VLAN filtering enabled (in which case the CPU port >> + * will be a member of the VLAN). >> + */ >> + if (skb->protocol == htons(ETH_P_8021Q) && >> + (!br_dev || br_vlan_enabled(br_dev))) { > > Conservative patch. If !br_dev, we could/should inject using > MV88E6XXX_VID_STANDALONE. But since we use FROM_CPU, the classified VLAN > probably does not make a difference that I can see, so there is no > reason to change this now (and certainly not in the same patch). We could also do that. My reasoning was: 1. There is no functional difference with a FROM_CPU frame - the CPU's word is law. 2. Performance should be better since you can avoid a per-packet memmove to make room for the tag, if the non-ethertyped version of DSA is used.
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c index c8b4bbd46191..e4b6e3f2a3db 100644 --- a/net/dsa/tag_dsa.c +++ b/net/dsa/tag_dsa.c @@ -127,6 +127,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, u8 extra) { struct dsa_port *dp = dsa_slave_to_port(dev); + struct net_device *br_dev; u8 tag_dev, tag_port; enum dsa_cmd cmd; u8 *dsa_header; @@ -149,7 +150,16 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, tag_port = dp->index; } - if (skb->protocol == htons(ETH_P_8021Q)) { + br_dev = dsa_port_bridge_dev_get(dp); + + /* If frame is already 802.1Q tagged, we can convert it to a DSA + * tag (avoiding a memmove), but only if the port is standalone + * (in which case we always send FROM_CPU) or if the port's + * bridge has VLAN filtering enabled (in which case the CPU port + * will be a member of the VLAN). + */ + if (skb->protocol == htons(ETH_P_8021Q) && + (!br_dev || br_vlan_enabled(br_dev))) { if (extra) { skb_push(skb, extra); dsa_alloc_etype_header(skb, extra); @@ -166,10 +176,9 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, dsa_header[2] &= ~0x10; } } else { - struct net_device *br = dsa_port_bridge_dev_get(dp); u16 vid; - vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE; + vid = br_dev ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE; skb_push(skb, DSA_HLEN + extra); dsa_alloc_etype_header(skb, DSA_HLEN + extra);
In this situation (VLAN filtering disabled on br0): br0.10 / br0 / \ swp0 swp1 When a frame is transmitted from the VLAN upper, the bridge will send it down to one of the switch ports with forward offloading enabled. This will cause tag_dsa to generate a FORWARD tag. Before this change, that tag would have it's VID set to 10, even though VID 10 is not loaded in the VTU. Before the blamed commit, the frame would trigger a VTU miss and be forwarded according to the PVT configuration. Now that all fabric ports are in 802.1Q secure mode, the frame is dropped instead. Therefore, restrict the condition under which we rewrite an 802.1Q tag to a DSA tag. On standalone port's, reuse is always safe since we will always generate FROM_CPU tags in that case. For bridged ports though, we must ensure that VLAN filtering is enabled, which in turn guarantees that the VID in question is loaded into the VTU. Fixes: d352b20f4174 ("net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports") Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/tag_dsa.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)