[PATCH net 1/2] veth: keep carrier off when resizing channels with a down peer

[PATCH net 1/2] veth: keep carrier off when resizing channels with a down peer

[Xia Yan]

veth_set_channels() temporarily turns carrier off while reconfiguring a
running interface. It then turns carrier on for both ends without
checking whether the peer is administratively up. If only one end is up,
changing its channel counts therefore reports LOWER_UP even though the
peer remains down.

For example, after creating a pair with two RX and TX queues per end:

  ip link set vca up
  ethtool -L vca rx 1 tx 1

vca incorrectly reports LOWER_UP while vcb is still down.

Only restore carrier when the peer has IFF_UP, matching veth_open().
The local interface is already known to be running at this point.

Fixes: 4752eeb3d891 ("veth: implement support for set_channel ethtool op")
Cc: stable@vger.kernel.org
Assisted-by: LLM

Signed-off-by: Xia Yan <yana06070@163.com>
---

Notes (validation):
    Testing: Built and booted UML kernels with W=1 before and after
    the fix. The regression test reproduces the bug before the fix;
    all eight cases pass after the fix.

 drivers/net/veth.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 643b97d..4e75830 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1372,9 +1372,10 @@ static int veth_set_channels(struct net_device *dev,
 		 * to identify the range we have to disable
 		 */
 		veth_disable_range_safe(dev, new_rx_count, old_rx_count);
-		netif_carrier_on(dev);
-		if (peer)
+		if (peer && (peer->flags & IFF_UP)) {
+			netif_carrier_on(dev);
 			netif_carrier_on(peer);
+		}
 	}
 
 	/* update XDP supported features */

base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
-- 
2.33.0

[PATCH net 2/2] selftests: net: check veth carrier after changing channel counts

[Xia Yan]

Add a regression test for veth_set_channels() incorrectly enabling
carrier when the peer is administratively down.

Check all combinations of the two ends' administrative states with GRO
enabled and disabled. Resize the RX and TX channel counts in both
directions on each end and verify that carrier is on only when both ends
are up. Also verify that rejecting an out-of-range channel count leaves
carrier unchanged.

On an unpatched kernel the two mixed up/down cases fail with each GRO
setting. All eight cases pass with the carrier restoration fix.

Assisted-by: LLM

Signed-off-by: Xia Yan <yana06070@163.com>
---

Notes (validation):
    Tested with iproute2 and ethtool in an isolated network namespace under
    the unpatched and patched UML kernels. The mixed up/down states fail
    before the driver fix with both GRO settings; all eight cases pass after
    the fix. The script also passes bash -n.
    
    The new file is covered by the existing MAINTAINERS entry for
    tools/testing/selftests/; no MAINTAINERS update is needed.

 tools/testing/selftests/net/Makefile          |  1 +
 .../selftests/net/veth_channels_carrier.sh    | 89 +++++++++++++++++++
 2 files changed, 90 insertions(+)
 create mode 100755 tools/testing/selftests/net/veth_channels_carrier.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 590b33e..4004014 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -128,6 +128,7 @@ TEST_PROGS := \
 	udpgso_bench.sh \
 	unicast_extensions.sh \
 	veth.sh \
+	veth_channels_carrier.sh \
 	vlan_bridge_binding.sh \
 	vlan_hw_filter.sh \
 	vrf-xfrm-tests.sh \
diff --git a/tools/testing/selftests/net/veth_channels_carrier.sh b/tools/testing/selftests/net/veth_channels_carrier.sh
new file mode 100755
index 0000000..c72a03b
--- /dev/null
+++ b/tools/testing/selftests/net/veth_channels_carrier.sh
@@ -0,0 +1,89 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+source "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
+
+require_command ip
+require_command ethtool
+
+if [ "$EUID" -ne 0 ]; then
+	echo "SKIP: root privileges are required"
+	exit "$ksft_skip"
+fi
+
+trap cleanup_all_ns EXIT
+setup_ns ns || exit $?
+
+check_carrier()
+{
+	local dev=$1
+	local expected=$2
+	local carrier=0
+	local link
+
+	link=$(ip -n "$ns" -o link show dev "$dev")
+	check_err $? "Failed to read $dev link state"
+	[[ $link == *LOWER_UP* ]] && carrier=1
+	[ "$carrier" -eq "$expected" ]
+	check_err $? "$dev carrier is $carrier, expected $expected"
+}
+
+check_state()
+{
+	local left=$1
+	local right=$2
+	local gro=$3
+	local expected=0
+	local count dev
+
+	RET=0
+	if ! ip -n "$ns" link add vca numtxqueues 2 numrxqueues 2 \
+		type veth peer name vcb numtxqueues 2 numrxqueues 2; then
+		RET=$ksft_skip
+		log_test "veth carrier: $left/$right, GRO $gro"
+		return
+	fi
+
+	ip -n "$ns" link set vca "$left"
+	check_err $? "Failed to configure vca"
+	ip -n "$ns" link set vcb "$right"
+	check_err $? "Failed to configure vcb"
+	ip netns exec "$ns" ethtool -K vca gro "$gro" >/dev/null
+	check_err $? "Failed to configure GRO"
+	ip netns exec "$ns" ethtool -K vcb gro "$gro" >/dev/null
+	check_err $? "Failed to configure peer GRO"
+
+	[ "$left" = up ] && [ "$right" = up ] && expected=1
+	check_carrier vca "$expected"
+	check_carrier vcb "$expected"
+
+	for dev in vca vcb; do
+		for count in 1 2; do
+			ip netns exec "$ns" ethtool -L "$dev" \
+				rx "$count" tx "$count"
+			check_err $? "Failed to resize $dev channels"
+			check_carrier vca "$expected"
+			check_carrier vcb "$expected"
+		done
+
+		ip netns exec "$ns" ethtool -L "$dev" rx 3 tx 3 \
+			>/dev/null 2>&1
+		check_fail $? "Out-of-range channel counts were accepted"
+		check_carrier vca "$expected"
+		check_carrier vcb "$expected"
+	done
+
+	ip -n "$ns" link del vca
+	check_err $? "Failed to delete the veth pair"
+	log_test "veth carrier: $left/$right, GRO $gro"
+}
+
+for gro in off on; do
+	for left in down up; do
+		for right in down up; do
+			check_state "$left" "$right" "$gro"
+		done
+	done
+done
+
+exit "$EXIT_STATUS"
-- 
2.33.0

Re: [PATCH net 2/2] selftests: net: check veth carrier after changing channel counts

[Xia Yan]

Hi,

I checked the two selftest findings on the Sashiko dashboard:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/8081

The LOWER_UP coverage issue is valid. The test detects the original bug
through the running endpoint in the mixed up/down cases, but cannot read
the down endpoint's raw carrier state through IFF_LOWER_UP.

For v2, I have limited the assertions and commit description to LOWER_UP
on running endpoints and dropped the down/down cases. The test targets
the incorrect LOWER_UP reported by an up endpoint whose peer is down;
it does not claim to verify the raw carrier bit on a down endpoint.

I also changed the veth creation failure path to use log_test_skip().
An injected creation failure after a failed case confirmed that the SKIP
line no longer repeats the earlier error message and that the aggregate
failure is preserved.

I reran the revised test under the existing unpatched and patched UML
kernels: the four mixed up/down cases fail before the fix, and all six
cases pass after it. The driver diff is unchanged.

Thanks,
Xia Yan

Re: [PATCH net 1/2] veth: keep carrier off when resizing channels with a down peer

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/2] veth: keep carrier off when resizing channels with a down peer

[Xia Yan]

Hi,

Resending as plain text because the mailing lists rejected my previous
reply for containing HTML.

This issue was found during LLM-assisted source review of
veth_set_channels(). I reproduced it with a veth pair in a network
namespace by bringing one end up, leaving the peer down, and changing
the active end's RX/TX channel counts with ethtool -L. The UP endpoint
then incorrectly reported LOWER_UP.

I built and booted unpatched and patched UML kernels and ran the
accompanying selftest. The four mixed up/down cases fail before the
fix; all eight cases pass after the fix.

I will include the discovery method in the commit message if another
revision is needed.

Thanks,
Xia Yan