netdev - [RFC PATCH net 1/4] net: dsa: on 'bridge vlan add', check for 8021q uppers of all bridge ports

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue,  9 Mar 2021 04:16:54 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: [RFC PATCH net 1/4] net: dsa: on 'bridge vlan add', check for 8021q uppers of all bridge ports

From: Vladimir Oltean <vladimir.oltean@....com>

The first (original) blamed patch intended to prevent the existence of
overlapping 8021q uppers of any bridge port when attempting to offload a
bridge VLAN. Unfortunately, it doesn't check the presence of the offending
8021q upper on all bridge ports, just on the one on which the 'bridge
vlan add' command was emitted (and on the other ports in the crosschip
bitmap, i.e. CPU port and DSA links, all irrelevant).

For example, the following setup:

$ ip link add dev br0 type bridge vlan_filtering 1
$ ip link add dev lan0.100 link lan0 type vlan id 100
$ ip link set dev lan0 master br0
$ ip link set dev lan1 master br0
$ bridge vlan add dev lan1 vid 100

should return an error at the last command, because if it doesn't, the
hardware will be configured in an invalid state where forwarding will
take place between traffic belonging to lan0.100 and lan1.

The trouble is that in hardware, all VLANs kinda smell the same, so if
we offload an 8021q upper on top of a bridged port, this will be in no
way different than adding that VLAN with 'bridge vlan add', however from
the perspective of Linux network stack semantics it is - traffic sent to
8021q uppers is 'stolen' from the bridge rx_handler in the software data
path, and does not take part in bridging unless the 8021q upper is
explicitly bridged, therefore we should observe those semantics.

This changes dsa_slave_vlan_check_for_8021q_uppers into a more reusable
dsa_check_bridge_for_overlapping_8021q_uppers, and also drops the bogus
requirement for holding RCU read-side protection: we are already
serialized with potential writers to the netdev adjacency lists because
we are executing under the rtnl_mutex.

The second blamed patch is where this commit actually applies to.

Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
Fixes: 1ce39f0ee8da ("net: dsa: convert denying bridge VLAN with existing 8021q upper to PRECHANGEUPPER")
Reported-by: Tobias Waldekranz <tobias@...dekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 net/dsa/slave.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 63ee2cae4d8e..d36e11399626 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,23 +323,27 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }

-/* Must be called under rcu_read_lock() */
 static int
-dsa_slave_vlan_check_for_8021q_uppers(struct net_device *slave,
-				      const struct switchdev_obj_port_vlan *vlan)
+dsa_check_bridge_for_overlapping_8021q_uppers(struct net_device *bridge_dev,
+					      u16 vid)
 {
-	struct net_device *upper_dev;
-	struct list_head *iter;
-
-	netdev_for_each_upper_dev_rcu(slave, upper_dev, iter) {
-		u16 vid;
+	struct list_head *iter_upper, *iter_lower;
+	struct net_device *upper, *lower;

-		if (!is_vlan_dev(upper_dev))
+	netdev_for_each_lower_dev(bridge_dev, lower, iter_lower) {
+		if (!dsa_slave_dev_check(lower))
 			continue;

-		vid = vlan_dev_vlan_id(upper_dev);
-		if (vid == vlan->vid)
-			return -EBUSY;
+		netdev_for_each_upper_dev_rcu(lower, upper, iter_upper) {
+			u16 upper_vid;
+
+			if (!is_vlan_dev(upper))
+				continue;
+
+			upper_vid = vlan_dev_vlan_id(upper);
+			if (upper_vid == vid)
+				return -EBUSY;
+		}
 	}

 	return 0;
@@ -368,12 +372,11 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	 * the same VID.
 	 */
 	if (br_vlan_enabled(dp->bridge_dev)) {
-		rcu_read_lock();
-		err = dsa_slave_vlan_check_for_8021q_uppers(dev, &vlan);
-		rcu_read_unlock();
+		err = dsa_check_bridge_for_overlapping_8021q_uppers(dp->bridge_dev,
+								    vlan.vid);
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
-					   "Port already has a VLAN upper with this VID");
+					   "Bridge already has a port with a VLAN upper with this VID");
 			return err;
 		}
 	}
-- 
2.25.1