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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 12 Sep 2020 06:56:12 +0000 From: Nikolay Aleksandrov <nikolay@...dia.com> To: "stephen@...workplumber.org" <stephen@...workplumber.org>, "olteanv@...il.com" <olteanv@...il.com>, "kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>, Roopa Prabhu <roopa@...dia.com> CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "f.fainelli@...il.com" <f.fainelli@...il.com>, "andrew@...n.ch" <andrew@...n.ch> Subject: Re: [PATCH net-next] net: bridge: pop vlan from skb if filtering is disabled but it's a pvid 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 <olteanv@...il.com> > Tested-by: Florian Fainelli <f.fainelli@...il.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; > } >
Powered by blists - more mailing lists