Merge branch 'netconsole-replace-target_list_lock-by-rcu-on-userdata-hot-path'

Breno Leitao says:

====================
netconsole: replace target_list_lock by RCU on userdata hot path

I would like to move netconsole to use RCU on the hot path for
a while instead of target_list_lock. My goal is to have no lock on the
tx side at all and eventually drop CON_NBCON_ATOMIC_UNSAFE, if that is
possible [1].

Start removing target_list_lock on certain parts of the code. This patch
transforms the userdata array into a RCU-protected pointer, and uses the
dynamic mutex as the write lock.

Added a selftest, given we didn't have any netconsole selftest for
userdata operations. Feel free to drop it if this is not useful, dear
maintainers.

Link: https://lore.kernel.org/all/20251121-nbcon-v1-0-503d17b2b4af@debian.org/[1]

Signed-off-by: Breno Leitao <leitao@debian.org>
====================

Link: https://patch.msgid.link/20260810-netcons-userdata-rcu-v3-0-f65557f769ce@debian.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

drivers/net/netconsole.c

@@ -151,13 +151,27 @@ enum target_state {
 	STATE_DEACTIVATED,
 };
 
+/**
+ * struct netcons_userdata - Formatted userdata payload of a target.
+ * @rcu:	Used to free the payload after a grace period.
+ * @length:	Length of @data, excluding the NUL terminator.
+ * @data:	Formatted " key=value\n" entries, NUL terminated.
+ *
+ * Immutable once published, so the transmit path never observes @data and
+ * @length disagreeing.
+ */
+struct netcons_userdata {
+	struct rcu_head		rcu;
+	size_t			length;
+	char			data[];
+};
+
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
  * @group:	Links us into the configfs subsystem hierarchy.
  * @userdata_group:	Links to the userdata configfs hierarchy
- * @userdata:		Cached, formatted string of append
- * @userdata_length:	String length of userdata.
+ * @userdata:		Cached, formatted userdata payload. RCU protected.
  * @sysdata:		Cached, formatted string of append
  * @sysdata_fields:	Sysdata features enabled.
  * @msgcounter:	Message sent counter.
@@ -198,8 +212,7 @@ struct netconsole_target {
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	struct config_group	group;
 	struct config_group	userdata_group;
-	char			*userdata;
-	size_t			userdata_length;
+	struct netcons_userdata __rcu *userdata;
 	char			sysdata[MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS];
 
 	/* bit-wise with sysdata_feature bits */
@@ -1351,12 +1364,11 @@ static int calc_userdata_len(struct netconsole_target *nt)
 
 static int update_userdata(struct netconsole_target *nt)
 {
+	struct netcons_userdata *new = NULL;
+	struct netcons_userdata *old;
 	struct userdatum *udm_item;
 	struct config_item *item;
 	struct list_head *entry;
-	char *old_buf = NULL;
-	char *new_buf = NULL;
-	unsigned long flags;
 	int offset = 0;
 	int len;
 
@@ -1368,8 +1380,8 @@ static int update_userdata(struct netconsole_target *nt)
 
 	/* Allocate new buffer */
 	if (len) {
-		new_buf = kmalloc(len + 1, GFP_KERNEL);
-		if (!new_buf)
+		new = kmalloc_flex(*new, data, len + 1);
+		if (!new)
 			return -ENOMEM;
 	}
 
@@ -1379,22 +1391,21 @@ static int update_userdata(struct netconsole_target *nt)
 		udm_item = to_userdatum(item);
 		/* Skip userdata with no value set */
 		if (udm_item->value[0]) {
-			offset += scnprintf(&new_buf[offset], len + 1 - offset,
+			offset += scnprintf(&new->data[offset],
+					    len + 1 - offset,
 					    " %s=%s\n", item->ci_name,
 					    udm_item->value);
 		}
 	}
 
 	WARN_ON_ONCE(offset != len);
+	if (new)
+		new->length = offset;
 
-	/* Switch to new buffer and free old buffer */
-	spin_lock_irqsave(&target_list_lock, flags);
-	old_buf = nt->userdata;
-	nt->userdata = new_buf;
-	nt->userdata_length = offset;
-	spin_unlock_irqrestore(&target_list_lock, flags);
-
-	kfree(old_buf);
+	/* Writers are serialized by dynamic_netconsole_mutex. */
+	old = rcu_replace_pointer(nt->userdata, new,
+				  lockdep_is_held(&dynamic_netconsole_mutex));
+	kfree_rcu(old, rcu);
 
 	return 0;
 }
@@ -1684,7 +1695,7 @@ static void netconsole_target_release(struct config_item *item)
 {
 	struct netconsole_target *nt = to_target(item);
 
-	kfree(nt->userdata);
+	kfree(rcu_access_pointer(nt->userdata));
 	kfree(nt);
 }
 
@@ -2227,14 +2238,13 @@ static void send_udp(struct netconsole_target *nt, const char *msg, int len)
 static void send_msg_no_fragmentation(struct netconsole_target *nt,
 				      const char *msg,
 				      int msg_len,
-				      int release_len)
+				      int release_len,
+				      const struct netcons_userdata *userdata)
 {
-	const char *userdata = NULL;
 	const char *sysdata = NULL;
 	const char *release;
 
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
-	userdata = nt->userdata;
 	sysdata = nt->sysdata;
 #endif
 
@@ -2251,7 +2261,7 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
 	if (userdata)
 		msg_len += scnprintf(&nt->buf[msg_len],
 				     sizeof(nt->buf) - msg_len, "%s",
-				     userdata);
+				     userdata->data);
 
 	if (sysdata)
 		msg_len += scnprintf(&nt->buf[msg_len],
@@ -2271,7 +2281,8 @@ static void append_release(char *buf)
 
 static void send_fragmented_body(struct netconsole_target *nt,
 				 const char *msgbody_ptr, int header_len,
-				 int msgbody_len, int sysdata_len)
+				 int msgbody_len, int sysdata_len,
+				 const struct netcons_userdata *userdata)
 {
 	const char *userdata_ptr = NULL;
 	const char *sysdata_ptr = NULL;
@@ -2282,12 +2293,12 @@ static void send_fragmented_body(struct netconsole_target *nt,
 	int userdata_len = 0;
 
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
-	userdata_ptr = nt->userdata;
 	sysdata_ptr = nt->sysdata;
-	userdata_len = nt->userdata_length;
 #endif
-	if (WARN_ON_ONCE(!userdata_ptr && userdata_len != 0))
-		return;
+	if (userdata) {
+		userdata_ptr = userdata->data;
+		userdata_len = userdata->length;
+	}
 
 	if (WARN_ON_ONCE(!sysdata_ptr && sysdata_len != 0))
 		return;
@@ -2364,7 +2375,8 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 				const char *msg,
 				int msg_len,
 				int release_len,
-				int sysdata_len)
+				int sysdata_len,
+				const struct netcons_userdata *userdata)
 {
 	int header_len, msgbody_len;
 	const char *msgbody;
@@ -2393,7 +2405,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	 * will be replaced
 	 */
 	send_fragmented_body(nt, msgbody, header_len, msgbody_len,
-			     sysdata_len);
+			     sysdata_len, userdata);
 }
 
 /**
@@ -2408,25 +2420,33 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 static void send_ext_msg_udp(struct netconsole_target *nt,
 			     struct nbcon_write_context *wctxt)
 {
+	const struct netcons_userdata *userdata = NULL;
 	int userdata_len = 0;
 	int release_len = 0;
 	int sysdata_len = 0;
 	int len;
 
+	/* Keeps the payload picked below alive until the last send_udp(). */
+	rcu_read_lock();
+
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
 	sysdata_len = prepare_sysdata(nt, wctxt);
-	userdata_len = nt->userdata_length;
+	userdata = rcu_dereference(nt->userdata);
+	if (userdata)
+		userdata_len = userdata->length;
 #endif
 	if (nt->release)
 		release_len = strlen(init_utsname()->release) + 1;
 
 	len = wctxt->len + release_len + sysdata_len + userdata_len;
 	if (len <= MAX_PRINT_CHUNK)
-		return send_msg_no_fragmentation(nt, wctxt->outbuf,
-						 wctxt->len, release_len);
+		send_msg_no_fragmentation(nt, wctxt->outbuf, wctxt->len,
+					  release_len, userdata);
+	else
+		send_msg_fragmented(nt, wctxt->outbuf, wctxt->len, release_len,
+				    sysdata_len, userdata);
 
-	return send_msg_fragmented(nt, wctxt->outbuf, wctxt->len, release_len,
-				   sysdata_len);
+	rcu_read_unlock();
 }
 
 static void send_msg_udp(struct netconsole_target *nt, const char *msg,
@@ -2669,7 +2689,7 @@ static void free_param_target(struct netconsole_target *nt)
 		netconsole_skb_pool_flush(nt);
 	netpoll_cleanup(&nt->np);
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
-	kfree(nt->userdata);
+	kfree(rcu_access_pointer(nt->userdata));
 #endif
 	kfree(nt);
 }

tools/testing/selftests/drivers/net/netconsole/Makefile

@@ -13,6 +13,7 @@ TEST_PROGS := \
 	netcons_resume.sh \
 	netcons_sysdata.sh \
 	netcons_torture.sh \
+	netcons_userdata.sh \
 # end of TEST_PROGS
 
 include ../../../lib.mk

tools/testing/selftests/drivers/net/netconsole/netcons_userdata.sh

@@ -0,0 +1,229 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Exercise the netconsole userdata payload.
+#
+# The first part checks that the payload the target transmits follows what
+# configfs says: a value shows up in the next message, an update replaces the
+# previous one, clearing the value drops the entry, and so does removing the
+# key.
+#
+# The second part rewrites values, creates and deletes keys, and clears the
+# payload entirely while messages are being sent, so the transmit path keeps
+# picking up payloads that are being replaced underneath it. It runs twice,
+# once with a payload small enough to fit in a single packet and once large
+# enough to be fragmented.
+#
+# Author: Breno Leitao <leitao@debian.org>
+
+set -euo pipefail
+
+SCRIPTDIR=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")
+
+source "${SCRIPTDIR}"/../lib/sh/lib_netcons.sh
+
+# Number of times each torture worker loops
+ITERATIONS=${1:-200}
+
+# Keys owned by each torture worker. Workers do not share keys, so a failing
+# configfs operation means a real problem and not a lost race.
+CHURN_KEY="churnkey"
+TRANSIENT_KEY="transientkey"
+# Number of keys used to push a message past MAX_PRINT_CHUNK
+BULK_KEYS=8
+
+USERDATA_DIR="${NETCONS_PATH}/userdata"
+# Values are capped at MAX_EXTRADATA_VALUE_LEN(200) bytes, so ${BULK_KEYS}
+# entries of this size are enough to force fragmentation
+LONG_VALUE=$(printf -- 'v%.0s' {1..190})
+
+function write_key() {
+	local KEY="${1}"
+	local VALUE="${2}"
+
+	mkdir -p "${USERDATA_DIR}/${KEY}"
+	echo "${VALUE}" > "${USERDATA_DIR}/${KEY}/value"
+}
+
+# Send a single message and capture it on the destination interface
+function send_and_capture() {
+	rm -f "${OUTPUT_FILE}"
+
+	listen_port_and_save_to "${OUTPUT_FILE}" &
+	wait_for_port "${NAMESPACE}" "${PORT}" "${IP_VERSION}"
+	echo "${MSG}: ${TARGET}" > /dev/kmsg
+	busywait "${BUSYWAIT_TIMEOUT}" test -s "${OUTPUT_FILE}" || true
+	pkill_socat
+	validate_msg "${OUTPUT_FILE}"
+}
+
+function expect_in_msg() {
+	local WANTED="${1}"
+
+	if ! grep -q -- "${WANTED}" "${OUTPUT_FILE}"; then
+		echo "FAIL: '${WANTED}' not found in ${OUTPUT_FILE}" >&2
+		cat "${OUTPUT_FILE}" >&2
+		exit "${ksft_fail}"
+	fi
+}
+
+function expect_not_in_msg() {
+	local UNWANTED="${1}"
+
+	if grep -q -- "${UNWANTED}" "${OUTPUT_FILE}"; then
+		echo "FAIL: '${UNWANTED}' found in ${OUTPUT_FILE}" >&2
+		cat "${OUTPUT_FILE}" >&2
+		exit "${ksft_fail}"
+	fi
+}
+
+# Every write publishes a new payload and frees the previous one. An empty
+# value is skipped when the payload is formatted, so this also drives the
+# target through having no payload at all.
+function churn_value() {
+	local i
+
+	for i in $(seq "${ITERATIONS}")
+	do
+		echo "value${i}" > "${USERDATA_DIR}/${CHURN_KEY}/value"
+		echo > "${USERDATA_DIR}/${CHURN_KEY}/value"
+	done
+}
+
+# Create and delete a key underneath the sender
+function churn_key() {
+	local i
+
+	for i in $(seq "${ITERATIONS}")
+	do
+		mkdir "${USERDATA_DIR}/${TRANSIENT_KEY}"
+		echo "transient${i}" > "${USERDATA_DIR}/${TRANSIENT_KEY}/value"
+		rmdir "${USERDATA_DIR}/${TRANSIENT_KEY}"
+	done
+}
+
+# Keep the transmit path busy while the payload is being replaced
+function send_messages() {
+	local i
+
+	for i in $(seq "${ITERATIONS}")
+	do
+		echo "${MSG}: ${TARGET} ${i}" > /dev/kmsg
+	done
+}
+
+# Run the workers concurrently and fail if any of them hits an error
+function run_workers() {
+	local PIDS=()
+	local WORKER
+	local RET=0
+	local PID
+
+	for WORKER in "$@"
+	do
+		"${WORKER}" &
+		PIDS+=("$!")
+	done
+
+	# Reap every worker before reporting a failure, otherwise a surviving
+	# worker keeps writing to configfs while the exit trap cleans it up.
+	for PID in "${PIDS[@]}"
+	do
+		wait "${PID}" || RET=1
+	done
+
+	if [[ "${RET}" -ne 0 ]]
+	then
+		echo "FAIL: userdata torture worker failed" >&2
+		exit "${ksft_fail}"
+	fi
+}
+
+function create_bulk_keys() {
+	local i
+
+	for i in $(seq "${BULK_KEYS}")
+	do
+		write_key "bulk${i}" "${LONG_VALUE}"
+	done
+}
+
+function delete_bulk_keys() {
+	local i
+
+	for i in $(seq "${BULK_KEYS}")
+	do
+		rmdir "${USERDATA_DIR}/bulk${i}"
+	done
+}
+
+# ========== #
+# Start here #
+# ========== #
+
+modprobe netdevsim 2> /dev/null || true
+modprobe netconsole 2> /dev/null || true
+
+IP_VERSION="ipv4"
+# The content of kmsg will be saved to the following file
+OUTPUT_FILE="/tmp/${TARGET}"
+
+# Check for basic system dependency and exit if not found
+check_for_dependencies
+# Set current loglevel to KERN_INFO(6), and default to KERN_NOTICE(5)
+echo "6 5" > /proc/sys/kernel/printk
+# Remove the namespace, interfaces and netconsole target on exit
+trap cleanup EXIT
+# Create one namespace and two interfaces
+set_network "${IP_VERSION}"
+# Create a dynamic target for netconsole
+create_dynamic_target
+
+# ===================================================
+# TEST #1
+# A value written to configfs reaches the destination
+# ===================================================
+write_key "${USERDATA_KEY}" "first"
+send_and_capture
+expect_in_msg "${USERDATA_KEY}=first"
+
+# ===================================================
+# TEST #2
+# Updating the value replaces the previous payload
+# ===================================================
+write_key "${USERDATA_KEY}" "second"
+send_and_capture
+expect_in_msg "${USERDATA_KEY}=second"
+expect_not_in_msg "${USERDATA_KEY}=first"
+
+# ===================================================
+# TEST #3
+# Clearing the value drops the entry
+# ===================================================
+echo > "${USERDATA_DIR}/${USERDATA_KEY}/value"
+send_and_capture
+expect_not_in_msg "${USERDATA_KEY}="
+
+# ===================================================
+# TEST #4
+# Removing the key drops the entry
+# ===================================================
+write_key "${USERDATA_KEY}" "third"
+rmdir "${USERDATA_DIR}/${USERDATA_KEY}"
+send_and_capture
+expect_not_in_msg "${USERDATA_KEY}="
+rm "${OUTPUT_FILE}"
+
+# ===================================================
+# TEST #5
+# Torture the payload while messages are being sent,
+# first unfragmented and then fragmented
+# ===================================================
+write_key "${CHURN_KEY}" "${USERDATA_VALUE}"
+run_workers churn_value churn_key send_messages
+
+create_bulk_keys
+run_workers churn_value churn_key send_messages
+delete_bulk_keys
+
+exit "${ksft_pass}"