On Sat, 2020-09-12 at 02:16 +0300, Vladimir Oltean wrote:
> Currently the bridge untags VLANs from its VLAN group in
> __allowed_ingress() only when VLAN filtering is enabled.
>
> When installing a pvid in egress-tagged mode, DSA switches have a
> problem:
>
> ip link add dev br0 type bridge vlan_filtering 0
> ip link set swp0 master br0
> bridge vlan del dev swp0 vid 1
> bridge vlan add dev swp0 vid 1 pvid
>
> When adding a VLAN on a DSA switch interface, DSA configures the VLAN
> membership of the CPU port using the same flags as swp0 (in this case
> "pvid and not untagged"), in an attempt to copy the frame as-is from
> ingress to the CPU.
>
> However, in this case, the packet may arrive untagged on ingress, it
> will be pvid-tagged by the ingress port, and will be sent as
> egress-tagged towards the CPU. Otherwise stated, the CPU will see a VLAN
> tag where there was none to speak of on ingress.
>
> When vlan_filtering is 1, this is not a problem, as stated in the first
> paragraph, because __allowed_ingress() will pop it. But currently, when
> vlan_filtering is 0 and we have such a VLAN configuration, we need an
> 8021q upper (br0.1) to be able to ping over that VLAN.
>
> Make the 2 cases (vlan_filtering 0 and 1) behave the same way by popping
> the pvid, if the skb happens to be tagged with it, when vlan_filtering
> is 0.
>
> There was an attempt to resolve this issue locally within the DSA
> receive data path, but even though we can determine that we are under a
> bridge with vlan_filtering=0, there are still some challenges:
> - we cannot be certain that the skb will end up in the software bridge's
> data path, and for that reason, we may be popping the VLAN for
> nothing. Example: there might exist an 8021q upper with the same VLAN,
> or this interface might be a DSA master for another switch. In that
> case, the VLAN should definitely not be popped even if it is equal to
> the default_pvid of the bridge, because it will be consumed about the
> DSA layer below.
Could you point me to a thread where these problems were discussed and why
they couldn't be resolved within DSA in detail ?
> - the bridge API only offers a race-free API for determining the pvid of
> a port, br_vlan_get_pvid(), under RTNL.
>
The API can be easily extended.
> And in fact this might not even be a situation unique to DSA. Any driver
> that receives untagged frames as pvid-tagged is now able to communicate
> without needing an 8021q upper for the pvid.
>
I would prefer we don't add hardware/driver-specific fixes in the bridge, when
vlan filtering is disabled there should be no vlan manipulation/filtering done
by the bridge. This could potentially break users who have added 8021q devices
as bridge ports. At the very least this needs to be hidden behind a new option,
but I would like to find a way to actually push it back to DSA. But again adding
hardware/driver-specific options should be avoided.
Can you use tc to pop the vlan on ingress ? I mean the cases above are visible
to the user, so they might decide to add the ingress vlan rule.
Thanks,
Nik
> Signed-off-by: Vladimir Oltean <olte...@gmail.com>
> Tested-by: Florian Fainelli <f.faine...@gmail.com>
> ---
> net/bridge/br_vlan.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index d2b8737f9fc0..ecfdb9cd3183 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -580,7 +580,23 @@ bool br_allowed_ingress(const struct net_bridge *br,
> * permitted.
> */
> if (!br_opt_get(br, BROPT_VLAN_ENABLED)) {
> + u16 v;
> +
> BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
> +
> + /* See comment in __allowed_ingress about how skb can end up
> + * here not having a hwaccel tag
> + */
> + if (unlikely(!skb_vlan_tag_present(skb) &&
> + skb->protocol == br->vlan_proto)) {
> + skb = skb_vlan_untag(skb);
> + if (unlikely(!skb))
> + return false;
> + }
> +
> + if (!br_vlan_get_tag(skb, &v) && v == br_get_pvid(vg))
> + __vlan_hwaccel_clear_tag(skb);
> +
> return true;
> }
>
