[PATCH net v2 0/2] net: preserve rps_default_mask on failed writes

[PATCH net v2 0/2] net: preserve rps_default_mask on failed writes

[Yingjie Wang]

rps_default_mask_sysctl() parses and validates writes in the live
per-netns mask. Both the bitmap parser and housekeeping validation can
modify that mask before returning an error, so a failed write can change
the default applied to RX queues created later.

Parse and validate a temporary mask before publishing it, and add a
regression test for a partially parsed invalid value.

On the base kernel, the new test fails because the mask changes from 1
to 3. With the fix, all ten rps_default_mask selftests pass. Tests with
isolated CPUs also confirm that overflow and housekeeping failures leave
the mask unchanged, while valid writes continue to work.

Changes since v1:
- Parse and validate the temporary mask before taking
  rps_default_mask_mutex, as suggested by Wang Zhan.
- Add Wang Zhan's Reviewed-by and Tested-by tags to patch 1.
- Add Kuniyuki Iwashima to Cc.

v1: https://lore.kernel.org/r/cover.1793199118.git.x-professor@outlook.com

Yingjie Wang (2):
  net: preserve rps_default_mask on failed writes
  selftests: net: check failed rps_default_mask writes

 net/core/sysctl_net_core.c                    | 31 ++++++++++---------
 .../testing/selftests/net/rps_default_mask.sh | 23 ++++++++++++++
 2 files changed, 40 insertions(+), 14 deletions(-)


base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
-- 
2.43.0

[PATCH net v2 1/2] net: preserve rps_default_mask on failed writes

[Yingjie Wang]

rps_default_mask_sysctl() passes the live per-netns mask to
cpumask_parse(). bitmap_parse() stores chunks as it scans from right to
left, so a later error can leave lower chunks changed.
rps_cpumask_housekeeping() also intersects its input in place before
returning -EINVAL when no housekeeping CPUs remain.

Consequently, a failed write changes both the value read from the sysctl
and the default copied to RX queues created later. On a four-CPU VM,
after setting the mask to 1, writing zz,00000003 failed with EOVERFLOW
but changed the mask to 3. Writing a mask containing only isolated CPUs
failed with EINVAL but changed it to 0. No kernel modification or error
injection was needed to reproduce either case.

Parse and validate a temporary mask before taking the mutex. After both
steps succeed, take the mutex and replace the live pointer. Readers
access and consume the pointer while holding the same mutex, so the
replaced mask can be freed after dropping the mutex.

The rps_default_mask selftest failed on the base kernel and passed with
the fix. The fixed kernel also kept the mask at 1 for the overflow and
housekeeping failure cases, while valid writes of 2 and 0 still
succeeded.

The issue was found while fuzzing sysctl write error paths.

Fixes: 605cfa1b1090 ("net: introduce default_rps_mask netns attribute")
Reviewed-by: Wang Zhan <wang.zhan@smartx.com>
Tested-by: Wang Zhan <wang.zhan@smartx.com>
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 net/core/sysctl_net_core.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 473c162736d69..f25281c709346 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -105,34 +105,37 @@ static int rps_default_mask_sysctl(const struct ctl_table *table, int write,
 				   void *buffer, size_t *lenp, loff_t *ppos)
 {
 	struct net *net = (struct net *)table->data;
+	struct cpumask *new_mask = NULL;
 	struct cpumask *mask;
 	int err = 0;
 
-	mutex_lock(&rps_default_mask_mutex);
-	mask = net->core.rps_default_mask;
 	if (write) {
-		if (!mask) {
-			mask = kzalloc(cpumask_size(), GFP_KERNEL);
-			net->core.rps_default_mask = mask;
-		}
-		err = -ENOMEM;
-		if (!mask)
-			goto done;
+		new_mask = kzalloc(cpumask_size(), GFP_KERNEL);
+		if (!new_mask)
+			return -ENOMEM;
 
-		err = cpumask_parse(buffer, mask);
+		err = cpumask_parse(buffer, new_mask);
 		if (err)
-			goto done;
+			goto out;
 
-		err = rps_cpumask_housekeeping(mask);
+		err = rps_cpumask_housekeeping(new_mask);
 		if (err)
-			goto done;
+			goto out;
+	}
+
+	mutex_lock(&rps_default_mask_mutex);
+	mask = net->core.rps_default_mask;
+	if (write) {
+		net->core.rps_default_mask = new_mask;
+		new_mask = mask;
 	} else {
 		err = dump_cpumask(buffer, lenp, ppos,
 				   mask ?: cpu_none_mask);
 	}
 
-done:
 	mutex_unlock(&rps_default_mask_mutex);
+out:
+	kfree(new_mask);
 	return err;
 }
 
-- 
2.43.0

[PATCH net v2 2/2] selftests: net: check failed rps_default_mask writes

[Yingjie Wang]

A failed write to rps_default_mask must not change the configured value.
Add a case with a valid low chunk followed by an invalid high chunk. The
old handler leaves the low chunk installed before returning an error,
whereas the fixed handler retains the previous mask.

Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 .../testing/selftests/net/rps_default_mask.sh | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/net/rps_default_mask.sh b/tools/testing/selftests/net/rps_default_mask.sh
index b200019b3c80a..91e163fd25450 100755
--- a/tools/testing/selftests/net/rps_default_mask.sh
+++ b/tools/testing/selftests/net/rps_default_mask.sh
@@ -45,8 +45,31 @@ chk_rps() {
 	fi
 }
 
+chk_rps_default_mask() {
+	local rps_mask expected_rps_mask=$2
+	local msg=$1
+
+	rps_mask=$(cat /proc/sys/net/core/rps_default_mask)
+	printf "%-60s" "$msg"
+
+	rps_mask=${rps_mask//,}
+	if [ "$rps_mask" -eq "$expected_rps_mask" ]; then
+		echo "[ ok ]"
+	else
+		echo "[fail] expected $expected_rps_mask found $rps_mask"
+		ret=1
+	fi
+}
+
 trap cleanup EXIT
 
+echo 1 > /proc/sys/net/core/rps_default_mask
+if echo "zz,00000003" > /proc/sys/net/core/rps_default_mask 2>/dev/null; then
+	echo "invalid rps_default_mask write unexpectedly succeeded"
+	ret=1
+fi
+chk_rps_default_mask "failed write doesn't change rps_default_mask" 1
+
 echo 0 > /proc/sys/net/core/rps_default_mask
 setup
 chk_rps "empty rps_default_mask" $NETNS lo 0
-- 
2.43.0