Re: [PATCH net-next] net: dsa: tag_dsa: Fix tx from VLAN uppers on non-filtering bridges

Tobias Waldekranz
2022-03-08T13:32:46Z

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.