[PATCH RFC/RFT net-next v2 0/5] net: dsa: deny unsupported 8021q upper on bridge port configurations

[PATCH RFC/RFT net-next v2 0/5] net: dsa: deny unsupported 8021q upper on bridge port configurations

[Jonas Gorski]

Documentation/networking/switchdev.rst is quite strict on how VLAN
uppers on bridged ports should work:

- with VLAN filtering turned off, the bridge will process all ingress traffic
  for the port, except for the traffic tagged with a VLAN ID destined for a
  VLAN upper. (...)

- with VLAN filtering turned on, these VLAN devices can be created as long as
  the bridge does not have an existing VLAN entry with the same VID on any
  bridge port. (...)

This means that VLAN tagged traffic matching a VLAN upper is never
forwarded from that port (unless the VLAN upper itself is bridged).

It does *not* mean that VLAN tagged traffic matching a VLAN upper is not
forwarded to that port anymore, as VLAN uppers only consume ingressing
traffic.

Currently, there is no way to tell dsa drivers that a VLAN on a
bridged port is for a VLAN upper and should not be processed by the
bridge.

Both adding a VLAN to a bridge port of bridge and adding a VLAN upper to
a bridged port of a VLAN-aware bridge will call
dsa_switch_ops::port_vlan_add(), with no way for the driver to know
which is which. In case of VLAN-unaware bridges, there is likely no
dsa_switch_ops::port_vlan_add() call at all for the VLAN upper.

But even if DSA told drivers which type of VLAN this is, most devices
likely would not support configuring forwarding per VLAN per port.

So in order to prevent the configuration of setups with unintended
forwarding between ports:

* deny configuring more than one VLAN upper on bridged ports per VLAN on
  VLAN filtering bridges
* deny configuring any VLAN uppers on bridged ports on VLAN non
  filtering bridges
* And consequently, disallow disabling filtering as long as there are
  any VLAN uppers configured on bridged ports

An alternative solution suggested by switchdev.rst would be to treat
these ports as standalone, and do the filtering/forwarding in software.

But likely DSA supported switches are used on low power devices, where
the performance impact from this would be large.

To verify that this is needed, add appropriate selftests to
no_forwarding to verify either VLAN uppers are denied, or VLAN traffic
is not unexpectedly (still) forwarded.

These test succeed with a veth-backed software bridge, but fail on a b53
device without the DSA changes applied.

While going through the code, I also found one corner case where it was
possible to add bridge VLANs shared with VLAN uppers, while adding
VLAN uppers shared with bridge VLANs was properly denied. This is the
first patch as this seems to be like the least controversial.

Still sent as a RFC/RFT for now due to the potential impact, though a
preliminary test didn't should any failures with
bridge_vlan_{un,}aware.sh and local_termination.sh selftests on
BCM63268.

Also since net-next is closed (though I'm not sure yet if this is net or
net-next material, since this just properly prevents broken setups).

Changes v1 -> v2:

* added selftests for both VLAN-aware and VLAN-unaware bridges
* actually disallow VLAN uppers on VLAN-unware bridges, not disallow
  more than one
* fixed the description of VLAN upper notification behaviour of DSA with
  filtering disabled

Jonas Gorski (5):
  net: dsa: deny bridge VLAN with existing 8021q upper on any port
  net: dsa: deny multiple 8021q uppers on bridged ports for the same
    VLAN
  selftests: no_forwarding: test VLAN uppers on VLAN aware bridged ports
  net: dsa: deny 8021q uppers on vlan unaware bridged ports
  selftests: no_forwarding: test VLAN uppers on VLAN-unaware bridged
    ports

 net/dsa/port.c                                |  23 +---
 net/dsa/user.c                                |  51 ++++++---
 .../selftests/net/forwarding/no_forwarding.sh | 107 ++++++++++++++----
 3 files changed, 127 insertions(+), 54 deletions(-)


base-commit: 0177f0f07886e54e12c6f18fa58f63e63ddd3c58
-- 
2.43.0

[PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Jonas Gorski]

Currently adding a bridge vlan to a port only checks for an 8021q upper
of that vlan on the port, but does not check for matching 8021q uppers
on other ports.

This leads to the possibility of configuring shared vlans on ports after
adding uppers.

E.g. adding the upper after configuring the vlan would be rejected

$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ bridge vlan add dev swp2 vid 100
$ ip link add swp1.100 link swp1 type vlan id 100
RTNETLINK answers: Resource busy

But the other way around would currently be accepted:

$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link swp1 type vlan id 100
$ bridge vlan add dev swp2 vid 100
$ bridge vlan
port              vlan-id
swp2              1 PVID Egress Untagged
                  100
swp1              1 PVID Egress Untagged
br0               1 PVID Egress Untagged

Fix this by checking all members of the bridge for a matching vlan
upper, and not the port itself.

After:

$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link swp1 type vlan id 100
$ bridge vlan add dev swp2 vid 100
RTNETLINK answers: Resource busy

Fixes: 1ce39f0ee8da ("net: dsa: convert denying bridge VLAN with existing 8021q upper to PRECHANGEUPPER")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* no changes

 net/dsa/user.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index f59d66f0975d..fa1fe0f1493a 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
 
 /* Must be called under rcu_read_lock() */
 static int
-dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
+dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
 				     const struct switchdev_obj_port_vlan *vlan)
 {
-	struct net_device *upper_dev;
-	struct list_head *iter;
+	struct dsa_switch *ds = dp->ds;
+	struct dsa_port *other_dp;
 
-	netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
-		u16 vid;
+	dsa_switch_for_each_user_port(other_dp, ds) {
+		struct net_device *user = other_dp->user;
+		struct net_device *upper_dev;
+		struct list_head *iter;
 
-		if (!is_vlan_dev(upper_dev))
+		if (!dsa_port_bridge_same(dp, other_dp))
 			continue;
 
-		vid = vlan_dev_vlan_id(upper_dev);
-		if (vid == vlan->vid)
-			return -EBUSY;
+		netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
+			u16 vid;
+
+			if (!is_vlan_dev(upper_dev))
+				continue;
+
+			vid = vlan_dev_vlan_id(upper_dev);
+			if (vid == vlan->vid)
+				return -EBUSY;
+		}
 	}
 
 	return 0;
@@ -693,11 +702,11 @@ static int dsa_user_vlan_add(struct net_device *dev,
 	 */
 	if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
 		rcu_read_lock();
-		err = dsa_user_vlan_check_for_8021q_uppers(dev, vlan);
+		err = dsa_user_vlan_check_for_8021q_uppers(dp, vlan);
 		rcu_read_unlock();
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
-					   "Port already has a VLAN upper with this VID");
+					   "This VLAN already has an upper configured on a bridge port");
 			return err;
 		}
 	}
-- 
2.43.0

[PATCH RFC/RFT net-next v2 2/5] net: dsa: deny multiple 8021q uppers on bridged ports for the same VLAN

[Jonas Gorski]

When creating 8021q uppers on bridged ports on a vlan filtering bridge,
we will configure the VLAN on the ports. For the dsa driver, there is no
difference between a 8021q upper on bridged port and a port vlan
configured within the bridge.

For that reason, if we configure a second 8021q upper for the same VLAN
on a different port of the bridge, we implicitly enable forwarding
between these ports on that VLAN.

This breaks the requirement for 8021q uppers for the VLAN to be
consumed, so we need to reject these configurations. Reuse
dsa_user_vlan_check_for_8021q_uppers() and change its argument to just
the vlan id.

Before:

$ ip link add br0 type bridge vlan_filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link GbE1 type vlan id 100
$ ip link add swp2.100 link GbE2 type vlan id 100
$

After:

$ ip link add br0 type bridge vlan_filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link GbE1 type vlan id 100
$ ip link add swp2.100 link GbE2 type vlan id 100
RTNETLINK answers: Resource busy

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* no changes

 net/dsa/user.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index fa1fe0f1493a..e8c6452780b0 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -653,8 +653,7 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
 
 /* Must be called under rcu_read_lock() */
 static int
-dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
-				     const struct switchdev_obj_port_vlan *vlan)
+dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp, u16 other_vid)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_port *other_dp;
@@ -674,7 +673,7 @@ dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
 				continue;
 
 			vid = vlan_dev_vlan_id(upper_dev);
-			if (vid == vlan->vid)
+			if (vid == other_vid)
 				return -EBUSY;
 		}
 	}
@@ -702,7 +701,7 @@ static int dsa_user_vlan_add(struct net_device *dev,
 	 */
 	if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
 		rcu_read_lock();
-		err = dsa_user_vlan_check_for_8021q_uppers(dp, vlan);
+		err = dsa_user_vlan_check_for_8021q_uppers(dp, vlan->vid);
 		rcu_read_unlock();
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
@@ -3185,6 +3184,16 @@ dsa_user_check_8021q_upper(struct net_device *dev,
 		return notifier_from_errno(-EBUSY);
 	}
 
+	rcu_read_lock();
+	err = dsa_user_vlan_check_for_8021q_uppers(dp, vid);
+	rcu_read_unlock();
+
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "This VLAN already has an upper configured on a bridge port");
+		return notifier_from_errno(err);
+	}
+
 	return NOTIFY_DONE;
 }
 
-- 
2.43.0

[PATCH RFC/RFT net-next v2 3/5] selftests: no_forwarding: test VLAN uppers on VLAN aware bridged ports

[Jonas Gorski]

Add a test (mainly for switchdev implementors) to test that multiple
VLAN uppers on a VLAN aware bridge for the same VLAN do not enable
forwarding of that VLAN between those ports.

Since we are testing VLAN uppers, skip checking untagged traffic in
those cases.

Disallowing VLAN uppers on bridge ports is a valid choice for switchdev
drivers, so test if we can create them first and skip the tests if not.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* new patch

 .../selftests/net/forwarding/no_forwarding.sh | 89 ++++++++++++++-----
 1 file changed, 67 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh
index 694ece9ba3a7..c8adf04e1328 100755
--- a/tools/testing/selftests/net/forwarding/no_forwarding.sh
+++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh
@@ -1,7 +1,7 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="standalone two_bridges one_bridge_two_pvids"
+ALL_TESTS="standalone two_bridges one_bridge_two_pvids bridge_aware_vlan_uppers"
 NUM_NETIFS=4
 
 source lib.sh
@@ -90,6 +90,7 @@ check_rcv()
 run_test()
 {
 	local test_name="$1"
+	local swp_uppers=${2:0}
 	local smac=$(mac_get $h1)
 	local dmac=$(mac_get $h2)
 	local h1_ipv6_lladdr=$(ipv6_lladdr_get $h1)
@@ -99,16 +100,24 @@ run_test()
 
 	tcpdump_start $h2
 
-	send_non_ip $h1 $smac $dmac
-	send_non_ip $h1 $smac $NON_IP_MC
-	send_non_ip $h1 $smac $BC
-	send_uc_ipv4 $h1 $dmac
-	send_mc_ipv4 $h1
-	send_uc_ipv6 $h1 $dmac
-	send_mc_ipv6 $h1
+	if [ "$swp_uppers" -eq 0 ]; then
+		send_non_ip $h1 $smac $dmac
+		send_non_ip $h1 $smac $NON_IP_MC
+		send_non_ip $h1 $smac $BC
+		send_uc_ipv4 $h1 $dmac
+		send_mc_ipv4 $h1
+		send_uc_ipv6 $h1 $dmac
+		send_mc_ipv6 $h1
+	fi
 
 	for vid in "${vids[@]}"; do
 		vlan_create $h1 $vid
+		if [  "$swp_uppers" -ge 1 ]; then
+			vlan_create $swp1 $vid
+		fi
+		if [  "$swp_uppers" -ge 2 ]; then
+			vlan_create $swp2 $vid
+		fi
 		simple_if_init $h1.$vid $H1_IPV4/24 $H1_IPV6/64
 
 		send_non_ip $h1.$vid $smac $dmac
@@ -120,6 +129,12 @@ run_test()
 		send_mc_ipv6 $h1.$vid
 
 		simple_if_fini $h1.$vid $H1_IPV4/24 $H1_IPV6/64
+		if [  "$swp_uppers" -ge 2 ]; then
+			vlan_destroy $swp2 $vid
+		fi
+		if [  "$swp_uppers" -ge 1 ]; then
+			vlan_destroy $swp1 $vid
+		fi
 		vlan_destroy $h1 $vid
 	done
 
@@ -129,26 +144,28 @@ run_test()
 
 	tcpdump_stop $h2
 
-	check_rcv $h2 "$test_name: Unicast non-IP untagged" \
-		"$smac > $dmac, 802.3, length 4:"
+	if [ "$swp_uppers" -eq 0 ]; then
+		check_rcv $h2 "$test_name: Unicast non-IP untagged" \
+			"$smac > $dmac, 802.3, length 4:"
 
-	check_rcv $h2 "$test_name: Multicast non-IP untagged" \
-		"$smac > $NON_IP_MC, 802.3, length 4:"
+		check_rcv $h2 "$test_name: Multicast non-IP untagged" \
+			"$smac > $NON_IP_MC, 802.3, length 4:"
 
-	check_rcv $h2 "$test_name: Broadcast non-IP untagged" \
-		"$smac > $BC, 802.3, length 4:"
+		check_rcv $h2 "$test_name: Broadcast non-IP untagged" \
+			"$smac > $BC, 802.3, length 4:"
 
-	check_rcv $h2 "$test_name: Unicast IPv4 untagged" \
-		"$smac > $dmac, ethertype IPv4 (0x0800)"
+		check_rcv $h2 "$test_name: Unicast IPv4 untagged" \
+			"$smac > $dmac, ethertype IPv4 (0x0800)"
 
-	check_rcv $h2 "$test_name: Multicast IPv4 untagged" \
-		"$smac > $MACV4_ALLNODES, ethertype IPv4 (0x0800).*: $H1_IPV4 > $IPV4_ALLNODES"
+		check_rcv $h2 "$test_name: Multicast IPv4 untagged" \
+			"$smac > $MACV4_ALLNODES, ethertype IPv4 (0x0800).*: $H1_IPV4 > $IPV4_ALLNODES"
 
-	check_rcv $h2 "$test_name: Unicast IPv6 untagged" \
-		"$smac > $dmac, ethertype IPv6 (0x86dd).*8: $H1_IPV6 > $H2_IPV6"
+		check_rcv $h2 "$test_name: Unicast IPv6 untagged" \
+			"$smac > $dmac, ethertype IPv6 (0x86dd).*8: $H1_IPV6 > $H2_IPV6"
 
-	check_rcv $h2 "$test_name: Multicast IPv6 untagged" \
-		"$smac > $MACV6_ALLNODES, ethertype IPv6 (0x86dd).*: $h1_ipv6_lladdr > $IPV6_ALLNODES"
+		check_rcv $h2 "$test_name: Multicast IPv6 untagged" \
+			"$smac > $MACV6_ALLNODES, ethertype IPv6 (0x86dd).*: $h1_ipv6_lladdr > $IPV6_ALLNODES"
+	fi
 
 	for vid in "${vids[@]}"; do
 		check_rcv $h2 "$test_name: Unicast non-IP VID $vid" \
@@ -209,6 +226,34 @@ one_bridge_two_pvids()
 	ip link del br0
 }
 
+bridge_aware_vlan_uppers()
+{
+	ip link add br0 type bridge vlan_filtering 1 vlan_default_pvid 0
+	ip link set br0 up
+	ip link set $swp1 master br0
+	ip link set $swp2 master br0
+
+	if ! ip link add name $swp1.10 link $swp1 type vlan id 10 2>/dev/null; then
+		ip link del br0
+		echo "SKIP: vlan-aware bridge does not allow vlan uppers on bridge ports"
+		exit "$ksft_skip"
+	fi
+
+	if ! ip link add name $swp2.10 link $swp2 type vlan id 10 2>/dev/null; then
+		vlan_destroy $swp1 10
+		ip link del br0
+		echo "SKIP: vlan-aware bridge does not allow multiple vlan uppers per VLAN on bridge ports"
+		exit "$ksft_skip"
+	fi
+
+	vlan_destroy $swp1 10
+	vlan_destroy $swp2 10
+
+	run_test "Switch ports in VLAN-aware bridge with VLAN uppers" 2
+
+	ip link del br0
+}
+
 h1_create()
 {
 	simple_if_init $h1 $H1_IPV4/24 $H1_IPV6/64
-- 
2.43.0

[PATCH RFC/RFT net-next v2 4/5] net: dsa: deny 8021q uppers on vlan unaware bridged ports

[Jonas Gorski]

Documentation/networking/switchdev.rst says:

- with VLAN filtering turned off, the bridge will process all ingress
  traffic for the port, except for the traffic tagged with a VLAN ID
  destined for a VLAN upper.

But DSA currently does not notify drivers about uppers on bridge ports
of a VLAN unaware bridge: Bridged ports on a VLAN unaware bridge will
usually not have filtering enabled and thus do not have
NETIF_F_HW_VLAN_CTAG_FILTER so dsa_user_vlan_rx_add_vid() is never
called.

And if filtering is always on for a driver, then DSA will just call
dsa_port_vlan_add(), in the same way it would for VLANs added to the
bridge. And VLANs programmed to the bridge are supposed to be ignored
while filtering is disabled, so drivers would still not act upon it.

Therefore traffic tagged with the VID will continue to be forwarded to
other ports, and therefore we cannot support VLAN uppers on ports of a
VLAN unaware bridges.

So reject any VLAN uppers for bridged ports of VLAN unaware bridges, and
reject disabling filtering as long as any VLAN uppers on bridged ports
exist.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* actually deny VLAN uppers on VLAN-unware bridges
* fix DSA behaviour description for non-filtering bridge ports

 net/dsa/port.c | 23 ++++-------------------
 net/dsa/user.c |  9 ++++++++-
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 082573ae6864..d7746885f7e0 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -728,35 +728,20 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_port *other_dp;
-	int err;
 
-	/* VLAN awareness was off, so the question is "can we turn it on".
+	/* VLAN awareness was on, so the question is "can we turn it off".
 	 * We may have had 8021q uppers, those need to go. Make sure we don't
 	 * enter an inconsistent state: deny changing the VLAN awareness state
 	 * as long as we have 8021q uppers.
 	 */
-	if (vlan_filtering && dsa_port_is_user(dp)) {
-		struct net_device *br = dsa_port_bridge_dev_get(dp);
+	if (!vlan_filtering && dsa_port_is_user(dp)) {
 		struct net_device *upper_dev, *user = dp->user;
 		struct list_head *iter;
 
 		netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
-			struct bridge_vlan_info br_info;
-			u16 vid;
-
-			if (!is_vlan_dev(upper_dev))
-				continue;
-
-			vid = vlan_dev_vlan_id(upper_dev);
-
-			/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
-			 * device, respectively the VID is not found, returning
-			 * 0 means success, which is a failure for us here.
-			 */
-			err = br_vlan_get_info(br, vid, &br_info);
-			if (err == 0) {
+			if (is_vlan_dev(upper_dev)) {
 				NL_SET_ERR_MSG_MOD(extack,
-						   "Must first remove VLAN uppers having VIDs also present in bridge");
+						   "Must first remove VLAN uppers from bridged ports");
 				return false;
 			}
 		}
diff --git a/net/dsa/user.c b/net/dsa/user.c
index e8c6452780b0..442cf3b2dc30 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -3167,10 +3167,17 @@ dsa_user_check_8021q_upper(struct net_device *dev,
 	int err = NOTIFY_DONE;
 	u16 vid;
 
-	if (!br || !br_vlan_enabled(br))
+	if (!br)
 		return NOTIFY_DONE;
 
 	extack = netdev_notifier_info_to_extack(&info->info);
+
+	if (!br_vlan_enabled(br)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "VLAN uppers not supported with non filtering bridges");
+		return notifier_from_errno(-EBUSY);
+	}
+
 	vid = vlan_dev_vlan_id(info->upper_dev);
 
 	/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
-- 
2.43.0

[PATCH RFC/RFT net-next v2 5/5] selftests: no_forwarding: test VLAN uppers on VLAN-unaware bridged ports

[Jonas Gorski]

A VLAN upper on a bridged port consumes the VLAN on this port and
inhibits forwarding of it.

Add a test that for VLAN-unaware bridges a single VLAN upper prevents
forwarding of that VLAN from that port. This is asymmetric "blocking",
as other ports' traffic can still be forwarded to this port. This is not
tested, as this is a no-forward test, not a forward test.

Since we are testing VLAN uppers, skip checking untagged traffic in
those cases.

Disallowing VLAN uppers on bridge ports is a valid choice for switchdev
drivers, so test if we can create them first and skip the tests if not.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* new patch

 .../selftests/net/forwarding/no_forwarding.sh | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh
index c8adf04e1328..d223b5b79a4f 100755
--- a/tools/testing/selftests/net/forwarding/no_forwarding.sh
+++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh
@@ -1,7 +1,7 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="standalone two_bridges one_bridge_two_pvids bridge_aware_vlan_uppers"
+ALL_TESTS="standalone two_bridges one_bridge_two_pvids bridge_unaware_vlan_upper bridge_aware_vlan_uppers"
 NUM_NETIFS=4
 
 source lib.sh
@@ -226,6 +226,24 @@ one_bridge_two_pvids()
 	ip link del br0
 }
 
+bridge_unaware_vlan_upper()
+{
+	ip link add br0 type bridge && ip link set br0 up
+	ip link set $swp1 master br0
+	ip link set $swp2 master br0
+
+	if ! ip link add name $swp1.10 link $swp1 type vlan id 10 2>/dev/null; then
+		ip link del br0
+		echo "SKIP: bridge does not allow vlan uppers on bridge ports"
+		exit "$ksft_skip"
+	fi
+	vlan_destroy $swp1 10
+
+	run_test "Switch ports in VLAN-unaware bridge with VLAN upper" 1
+
+	ip link del br0
+}
+
 bridge_aware_vlan_uppers()
 {
 	ip link add br0 type bridge vlan_filtering 1 vlan_default_pvid 0
-- 
2.43.0

Re: [PATCH RFC/RFT net-next v2 3/5] selftests: no_forwarding: test VLAN uppers on VLAN aware bridged ports

[Simon Horman]

On Mon, Dec 01, 2025 at 11:28:15AM +0100, Jonas Gorski wrote:
> Add a test (mainly for switchdev implementors) to test that multiple
> VLAN uppers on a VLAN aware bridge for the same VLAN do not enable
> forwarding of that VLAN between those ports.
> 
> Since we are testing VLAN uppers, skip checking untagged traffic in
> those cases.
> 
> Disallowing VLAN uppers on bridge ports is a valid choice for switchdev
> drivers, so test if we can create them first and skip the tests if not.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> ---
> v1 -> v2:
> * new patch
> 
>  .../selftests/net/forwarding/no_forwarding.sh | 89 ++++++++++++++-----
>  1 file changed, 67 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> index 694ece9ba3a7..c8adf04e1328 100755
> --- a/tools/testing/selftests/net/forwarding/no_forwarding.sh
> +++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> @@ -1,7 +1,7 @@
>  #!/bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  
> -ALL_TESTS="standalone two_bridges one_bridge_two_pvids"
> +ALL_TESTS="standalone two_bridges one_bridge_two_pvids bridge_aware_vlan_uppers"
>  NUM_NETIFS=4
>  
>  source lib.sh
> @@ -90,6 +90,7 @@ check_rcv()
>  run_test()
>  {
>  	local test_name="$1"
> +	local swp_uppers=${2:0}

Hi Jonas,

Should this be as follows?

	local swp_uppers=${2:-0}


I.e. default to 0 if $2 is not set,
     rather than take a substring of $2 at index 0 (which is all of $2)

Flagged by Claude Code with review-prompts.

https://netdev-ai.bots.linux.dev/ai-review.html?id=3d47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-2


>  	local smac=$(mac_get $h1)
>  	local dmac=$(mac_get $h2)
>  	local h1_ipv6_lladdr=$(ipv6_lladdr_get $h1)

...

> +bridge_aware_vlan_uppers()
> +{

...

> +	run_test "Switch ports in VLAN-aware bridge with VLAN uppers" 2
> +
> +	ip link del br0
> +}
> +

...

Re: [PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Simon Horman]

On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:

...

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index f59d66f0975d..fa1fe0f1493a 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
>  
>  /* Must be called under rcu_read_lock() */
>  static int
> -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
>  				     const struct switchdev_obj_port_vlan *vlan)
>  {
> -	struct net_device *upper_dev;
> -	struct list_head *iter;
> +	struct dsa_switch *ds = dp->ds;
> +	struct dsa_port *other_dp;
>  
> -	netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> -		u16 vid;
> +	dsa_switch_for_each_user_port(other_dp, ds) {
> +		struct net_device *user = other_dp->user;

Hi Jonas,

The AI robot is concerned that user may be NULL here.
And I can't convince myself that cannot be the case.

Could you take a look?

https://netdev-ai.bots.linux.dev/ai-review.html?id=3d47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0

> +		struct net_device *upper_dev;
> +		struct list_head *iter;
>  
> -		if (!is_vlan_dev(upper_dev))
> +		if (!dsa_port_bridge_same(dp, other_dp))
>  			continue;
>  
> -		vid = vlan_dev_vlan_id(upper_dev);
> -		if (vid == vlan->vid)
> -			return -EBUSY;
> +		netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> +			u16 vid;
> +
> +			if (!is_vlan_dev(upper_dev))
> +				continue;
> +
> +			vid = vlan_dev_vlan_id(upper_dev);
> +			if (vid == vlan->vid)
> +				return -EBUSY;
> +		}
>  	}
>  
>  	return 0;

...

Re: [PATCH RFC/RFT net-next v2 3/5] selftests: no_forwarding: test VLAN uppers on VLAN aware bridged ports

[Jonas Gorski]

Hi,

On Mon, Dec 1, 2025 at 3:19 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Dec 01, 2025 at 11:28:15AM +0100, Jonas Gorski wrote:
> > Add a test (mainly for switchdev implementors) to test that multiple
> > VLAN uppers on a VLAN aware bridge for the same VLAN do not enable
> > forwarding of that VLAN between those ports.
> >
> > Since we are testing VLAN uppers, skip checking untagged traffic in
> > those cases.
> >
> > Disallowing VLAN uppers on bridge ports is a valid choice for switchdev
> > drivers, so test if we can create them first and skip the tests if not.
> >
> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> > ---
> > v1 -> v2:
> > * new patch
> >
> >  .../selftests/net/forwarding/no_forwarding.sh | 89 ++++++++++++++-----
> >  1 file changed, 67 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> > index 694ece9ba3a7..c8adf04e1328 100755
> > --- a/tools/testing/selftests/net/forwarding/no_forwarding.sh
> > +++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh
> > @@ -1,7 +1,7 @@
> >  #!/bin/bash
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > -ALL_TESTS="standalone two_bridges one_bridge_two_pvids"
> > +ALL_TESTS="standalone two_bridges one_bridge_two_pvids bridge_aware_vlan_uppers"
> >  NUM_NETIFS=4
> >
> >  source lib.sh
> > @@ -90,6 +90,7 @@ check_rcv()
> >  run_test()
> >  {
> >       local test_name="$1"
> > +     local swp_uppers=${2:0}
>
> Hi Jonas,
>
> Should this be as follows?
>
>         local swp_uppers=${2:-0}
>
>
> I.e. default to 0 if $2 is not set,
>      rather than take a substring of $2 at index 0 (which is all of $2)

Yes it is, I fat fingered that (or cold fingered?). But since bash has
no abort on error enabled, it just chucks along and works by accident
anyway (there may be some complaints on stderr).

Best regards,
Jonas

Re: [PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Jonas Gorski]

Hi,

On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
>
> ...
>
> > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > index f59d66f0975d..fa1fe0f1493a 100644
> > --- a/net/dsa/user.c
> > +++ b/net/dsa/user.c
> > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> >
> >  /* Must be called under rcu_read_lock() */
> >  static int
> > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> >                                    const struct switchdev_obj_port_vlan *vlan)
> >  {
> > -     struct net_device *upper_dev;
> > -     struct list_head *iter;
> > +     struct dsa_switch *ds = dp->ds;
> > +     struct dsa_port *other_dp;
> >
> > -     netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > -             u16 vid;
> > +     dsa_switch_for_each_user_port(other_dp, ds) {
> > +             struct net_device *user = other_dp->user;
>
> Hi Jonas,
>
> The AI robot is concerned that user may be NULL here.
> And I can't convince myself that cannot be the case.
>
> Could you take a look?
>
> https://netdev-ai.bots.linux.dev/ai-review.html?id=3d47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0

At this point it can be NULL. But it being NULL is not an issue, as ...
>
> > +             struct net_device *upper_dev;
> > +             struct list_head *iter;
> >
> > -             if (!is_vlan_dev(upper_dev))
> > +             if (!dsa_port_bridge_same(dp, other_dp))
> >                       continue;

... this condition will filter all cases where it is NULL. For
dsa_port_bridge_same() to return true both ports need to be attached
to a bridge (and to the same bridge), and to be attached to a bridge a
net_device is required, so other_dp->user cannot be NULL. And we only
access user after here.

Best regards,
Jonas

Re: [PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Chris Mason]

On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote:

> Hi,
> 
> On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
> >
> > ...
> >
> > > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > > index f59d66f0975d..fa1fe0f1493a 100644
> > > --- a/net/dsa/user.c
> > > +++ b/net/dsa/user.c
> > > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> > >
> > >  /* Must be called under rcu_read_lock() */
> > >  static int
> > > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> > >                                    const struct switchdev_obj_port_vlan *vlan)
> > >  {
> > > -     struct net_device *upper_dev;
> > > -     struct list_head *iter;
> > > +     struct dsa_switch *ds = dp->ds;
> > > +     struct dsa_port *other_dp;
> > >
> > > -     netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > > -             u16 vid;
> > > +     dsa_switch_for_each_user_port(other_dp, ds) {
> > > +             struct net_device *user = other_dp->user;
> >
> > Hi Jonas,
> >
> > The AI robot is concerned that user may be NULL here.
> > And I can't convince myself that cannot be the case.
> >
> > Could you take a look?
> >
> > https://netdev-ai.bots.linux.dev/ai-review.html?id=47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
> 
> At this point it can be NULL. But it being NULL is not an issue, as ...
> >
> > > +             struct net_device *upper_dev;
> > > +             struct list_head *iter;
> > >
> > > -             if (!is_vlan_dev(upper_dev))
> > > +             if (!dsa_port_bridge_same(dp, other_dp))
> > >                       continue;
> 
> ... this condition will filter all cases where it is NULL. For
> dsa_port_bridge_same() to return true both ports need to be attached
> to a bridge (and to the same bridge), and to be attached to a bridge a
> net_device is required, so other_dp->user cannot be NULL. And we only
> access user after here.

I reproduced this false positive here, thanks for the explanation.  This is an
example of a class of review mistakes I've wanted to fix, so I used it to
improve the prompts around NULL pointers that are protected via other checks.

I'll test this on some more commits and push it out.

-chris

Re: [PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Simon Horman]

On Mon, Dec 01, 2025 at 02:48:48PM -0800, Chris Mason wrote:
> On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote:
> 
> > Hi,
> > 
> > On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
> > >
> > > On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
> > >
> > > ...
> > >
> > > > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > > > index f59d66f0975d..fa1fe0f1493a 100644
> > > > --- a/net/dsa/user.c
> > > > +++ b/net/dsa/user.c
> > > > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> > > >
> > > >  /* Must be called under rcu_read_lock() */
> > > >  static int
> > > > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > > > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> > > >                                    const struct switchdev_obj_port_vlan *vlan)
> > > >  {
> > > > -     struct net_device *upper_dev;
> > > > -     struct list_head *iter;
> > > > +     struct dsa_switch *ds = dp->ds;
> > > > +     struct dsa_port *other_dp;
> > > >
> > > > -     netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > > > -             u16 vid;
> > > > +     dsa_switch_for_each_user_port(other_dp, ds) {
> > > > +             struct net_device *user = other_dp->user;
> > >
> > > Hi Jonas,
> > >
> > > The AI robot is concerned that user may be NULL here.
> > > And I can't convince myself that cannot be the case.
> > >
> > > Could you take a look?
> > >
> > > https://netdev-ai.bots.linux.dev/ai-review.html?id=47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
> > 
> > At this point it can be NULL. But it being NULL is not an issue, as ...
> > >
> > > > +             struct net_device *upper_dev;
> > > > +             struct list_head *iter;
> > > >
> > > > -             if (!is_vlan_dev(upper_dev))
> > > > +             if (!dsa_port_bridge_same(dp, other_dp))
> > > >                       continue;
> > 
> > ... this condition will filter all cases where it is NULL. For
> > dsa_port_bridge_same() to return true both ports need to be attached
> > to a bridge (and to the same bridge), and to be attached to a bridge a
> > net_device is required, so other_dp->user cannot be NULL. And we only
> > access user after here.

Thanks for the explanation Jonas.

I wasn't very confident with this report.
And I was too focused on working out if user could be NULL rather
than if it matters. Still, I may not have worked it out.

> 
> I reproduced this false positive here, thanks for the explanation.  This is an
> example of a class of review mistakes I've wanted to fix, so I used it to
> improve the prompts around NULL pointers that are protected via other checks.
> 
> I'll test this on some more commits and push it out.

Thanks for following-up on this Chris.

I guess everyone has their own opinion on AI.
And, in a similar vein, many have opinions on the review-prompts.
But, FTR, I've been impressed by the output I've seen,
having used them for a few weeks now. And I look forward
to that improving further.

Re: [PATCH RFC/RFT net-next v2 1/5] net: dsa: deny bridge VLAN with existing 8021q upper on any port

[Chris Mason]

On 12/2/25 11:16 AM, Simon Horman wrote:
> On Mon, Dec 01, 2025 at 02:48:48PM -0800, Chris Mason wrote:
>> On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote:

[ ... ]

>>
>> I reproduced this false positive here, thanks for the explanation.  This is an
>> example of a class of review mistakes I've wanted to fix, so I used it to
>> improve the prompts around NULL pointers that are protected via other checks.
>>
>> I'll test this on some more commits and push it out.
> 
> Thanks for following-up on this Chris.
> 
> I guess everyone has their own opinion on AI.
> And, in a similar vein, many have opinions on the review-prompts.
> But, FTR, I've been impressed by the output I've seen,
> having used them for a few weeks now. And I look forward
> to that improving further.
> 

Thanks, I really appreciate everyone being willing to experiment with
the reviews (and huge thanks to Jakub for wiring this up).  I'll try to
stay on top of the false positives, but please also let me know if I'm
missing classes of bugs.

I think Jonas's other_dp->user check is really at the limit of claude's
ability to reason about the code in a generic way, and I'm still trying
to get it to process this patch consistently.

If all else fails we can fix some of these with more networking specific
knowledge, but I'm trying to avoid whack-a-mole.

-chris