[PATCH net-next 0/4] netconsole: Allow userdata buffer to grow dynamically

[PATCH net-next 0/4] netconsole: Allow userdata buffer to grow dynamically

[Gustavo Luiz Duarte]

The current netconsole implementation allocates a static buffer for
extradata (userdata + sysdata) with a fixed size of
MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS bytes for every target,
regardless of whether userspace actually uses this feature. This forces
us to keep MAX_EXTRADATA_ITEMS small (16), which is restrictive for
users who need to attach more metadata to their log messages.

This patch series enables dynamic allocation of the userdata buffer,
allowing it to grow on-demand based on actual usage. The series:

1. Refactors send_fragmented_body() to simplify handling of separated
   userdata and sysdata (patch 1/4)
2. Splits userdata and sysdata into separate buffers (patch 2/4)
3. Implements dynamic allocation for the userdata buffer (patch 3/4)
4. Increases MAX_USERDATA_ITEMS from 16 to 256 now that we can do so
   without memory waste (patch 4/4)

Benefits:
- No memory waste when userdata is not used
- Targets that use userdata only consume what they need
- Users can attach significantly more metadata without impacting systems
  that don't use this feature

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
Gustavo Luiz Duarte (4):
      netconsole: Simplify send_fragmented_body()
      netconsole: Split userdata and sysdata
      netconsole: Dynamic allocation of userdata buffer
      netconsole: Increase MAX_USERDATA_ITEMS

 drivers/net/netconsole.c                           | 338 +++++++++------------
 .../selftests/drivers/net/netcons_overflow.sh      |   2 +-
 2 files changed, 152 insertions(+), 188 deletions(-)
---
base-commit: 89aec171d9d1ab168e43fcf9754b82e4c0aef9b9
change-id: 20251007-netconsole_dynamic_extradata-21bd9d726568

Best regards,
-- 
Gustavo Duarte <gustavold@meta.com>

[PATCH net-next 1/4] netconsole: Simplify send_fragmented_body()

[Gustavo Luiz Duarte]

Refactor send_fragmented_body() to use separate offset tracking for
msgbody, and extradata instead of complex conditional logic.
The previous implementation used boolean flags and calculated offsets
which made the code harder to follow.

The new implementation maintains independent offset counters
(msgbody_offset, extradata_offset) and processes each section
sequentially, making the data flow more straightforward and the code
easier to maintain.

This is a preparatory refactoring with no functional changes, which will
allow easily splitting extradata_complete into separate userdata and
sysdata buffers in the next patch.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
 1 file changed, 24 insertions(+), 49 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5d8d0214786c..0a8ba7c4bc9d 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
 				 const char *msgbody, int header_len,
 				 int msgbody_len, int extradata_len)
 {
-	int sent_extradata, preceding_bytes;
 	const char *extradata = NULL;
 	int body_len, offset = 0;
+	int extradata_offset = 0;
+	int msgbody_offset = 0;
 
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
 	extradata = nt->extradata_complete;
 #endif
+	if (WARN_ON_ONCE(!extradata && extradata_len != 0))
+		return;
 
 	/* body_len represents the number of bytes that will be sent. This is
 	 * bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
@@ -1575,7 +1578,6 @@ static void send_fragmented_body(struct netconsole_target *nt,
 	 */
 	while (offset < body_len) {
 		int this_header = header_len;
-		bool msgbody_written = false;
 		int this_offset = 0;
 		int this_chunk = 0;
 
@@ -1584,55 +1586,28 @@ static void send_fragmented_body(struct netconsole_target *nt,
 					 ",ncfrag=%d/%d;", offset,
 					 body_len);
 
-		/* Not all msgbody data has been written yet */
-		if (offset < msgbody_len) {
-			this_chunk = min(msgbody_len - offset,
-					 MAX_PRINT_CHUNK - this_header);
-			if (WARN_ON_ONCE(this_chunk <= 0))
-				return;
-			memcpy(nt->buf + this_header, msgbody + offset,
-			       this_chunk);
-			this_offset += this_chunk;
-		}
-
-		/* msgbody was finally written, either in the previous
-		 * messages and/or in the current buf. Time to write
-		 * the extradata.
-		 */
-		msgbody_written |= offset + this_offset >= msgbody_len;
-
-		/* Msg body is fully written and there is pending extradata to
-		 * write, append extradata in this chunk
-		 */
-		if (msgbody_written && offset + this_offset < body_len) {
-			/* Track how much user data was already sent. First
-			 * time here, sent_userdata is zero
-			 */
-			sent_extradata = (offset + this_offset) - msgbody_len;
-			/* offset of bytes used in current buf */
-			preceding_bytes = this_chunk + this_header;
-
-			if (WARN_ON_ONCE(sent_extradata < 0))
-				return;
-
-			this_chunk = min(extradata_len - sent_extradata,
-					 MAX_PRINT_CHUNK - preceding_bytes);
-			if (WARN_ON_ONCE(this_chunk < 0))
-				/* this_chunk could be zero if all the previous
-				 * message used all the buffer. This is not a
-				 * problem, extradata will be sent in the next
-				 * iteration
-				 */
-				return;
-
-			memcpy(nt->buf + this_header + this_offset,
-			       extradata + sent_extradata,
-			       this_chunk);
-			this_offset += this_chunk;
-		}
+		/* write msgbody first */
+		this_chunk = min(msgbody_len - msgbody_offset,
+				 MAX_PRINT_CHUNK - this_header);
+		memcpy(nt->buf + this_header, msgbody + msgbody_offset,
+		       this_chunk);
+		msgbody_offset += this_chunk;
+		this_offset += this_chunk;
+
+		/* after msgbody, append extradata */
+		this_chunk = min(extradata_len - extradata_offset,
+				 MAX_PRINT_CHUNK - this_header - this_offset);
+		memcpy(nt->buf + this_header + this_offset,
+		       extradata + extradata_offset, this_chunk);
+		extradata_offset += this_chunk;
+		this_offset += this_chunk;
+
+		/* if all is good, send the packet out */
+		offset += this_offset;
+		if (WARN_ON_ONCE(offset > body_len))
+			return;
 
 		send_udp(nt, nt->buf, this_header + this_offset);
-		offset += this_offset;
 	}
 }
 

-- 
2.47.3

[PATCH net-next 2/4] netconsole: Split userdata and sysdata

[Gustavo Luiz Duarte]

Separate userdata and sysdata into distinct buffers to enable independent
management. Previously, both were stored in a single extradata_complete
buffer with a fixed size that accommodated both types of data.

This separation allows:
- userdata to grow dynamically (in subsequent patch)
- sysdata to remain in a small static buffer
- removal of complex entry counting logic that tracked both types together

The split also simplifies the code by eliminating the need to check total
entry count across both userdata and sysdata when enabling features,
which allows to drop holding su_mutex on sysdata_*_enabled_store().

No functional change in this patch, just structural preparation for
dynamic userdata allocation.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 204 +++++++++++++++++++----------------------------
 1 file changed, 84 insertions(+), 120 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 0a8ba7c4bc9d..e780c884db83 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -50,7 +50,8 @@ MODULE_LICENSE("GPL");
 /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
 #define MAX_EXTRADATA_NAME_LEN		(MAX_EXTRADATA_ENTRY_LEN - \
 					MAX_EXTRADATA_VALUE_LEN - 3)
-#define MAX_EXTRADATA_ITEMS		16
+#define MAX_USERDATA_ITEMS		16
+#define MAX_SYSDATA_ITEMS		4
 #define MAX_PRINT_CHUNK			1000
 
 static char config[MAX_PARAM_LENGTH];
@@ -122,8 +123,9 @@ enum sysdata_feature {
  * @list:	Links this target into the target_list.
  * @group:	Links us into the configfs subsystem hierarchy.
  * @userdata_group:	Links to the userdata configfs hierarchy
- * @extradata_complete:	Cached, formatted string of append
- * @userdata_length:	String length of usedata in extradata_complete.
+ * @userdata:		Cached, formatted string of append
+ * @userdata_length:	String length of userdata.
+ * @sysdata:		Cached, formatted string of append
  * @sysdata_fields:	Sysdata features enabled.
  * @msgcounter:	Message sent counter.
  * @stats:	Packet send stats for the target. Used for debugging.
@@ -152,8 +154,10 @@ struct netconsole_target {
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	struct config_group	group;
 	struct config_group	userdata_group;
-	char extradata_complete[MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS];
+	char			userdata[MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS];
 	size_t			userdata_length;
+	char			sysdata[MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS];
+
 	/* bit-wise with sysdata_feature bits */
 	u32			sysdata_fields;
 	/* protected by target_list_lock */
@@ -802,28 +806,14 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 	return ret;
 }
 
-/* Count number of entries we have in extradata.
- * This is important because the extradata_complete only supports
- * MAX_EXTRADATA_ITEMS entries. Before enabling any new {user,sys}data
- * feature, number of entries needs to checked for available space.
+/* Count number of entries we have in userdata.
+ * This is important because userdata only supports MAX_USERDATA_ITEMS
+ * entries. Before enabling any new userdata feature, number of entries needs
+ * to checked for available space.
  */
-static size_t count_extradata_entries(struct netconsole_target *nt)
+static size_t count_userdata_entries(struct netconsole_target *nt)
 {
-	size_t entries;
-
-	/* Userdata entries */
-	entries = list_count_nodes(&nt->userdata_group.cg_children);
-	/* Plus sysdata entries */
-	if (nt->sysdata_fields & SYSDATA_CPU_NR)
-		entries += 1;
-	if (nt->sysdata_fields & SYSDATA_TASKNAME)
-		entries += 1;
-	if (nt->sysdata_fields & SYSDATA_RELEASE)
-		entries += 1;
-	if (nt->sysdata_fields & SYSDATA_MSGID)
-		entries += 1;
-
-	return entries;
+	return list_count_nodes(&nt->userdata_group.cg_children);
 }
 
 static ssize_t remote_mac_store(struct config_item *item, const char *buf,
@@ -894,13 +884,13 @@ static void update_userdata(struct netconsole_target *nt)
 
 	/* Clear the current string in case the last userdatum was deleted */
 	nt->userdata_length = 0;
-	nt->extradata_complete[0] = 0;
+	nt->userdata[0] = 0;
 
 	list_for_each(entry, &nt->userdata_group.cg_children) {
 		struct userdatum *udm_item;
 		struct config_item *item;
 
-		if (child_count >= MAX_EXTRADATA_ITEMS) {
+		if (child_count >= MAX_USERDATA_ITEMS) {
 			spin_unlock_irqrestore(&target_list_lock, flags);
 			WARN_ON_ONCE(1);
 			return;
@@ -914,11 +904,11 @@ static void update_userdata(struct netconsole_target *nt)
 		if (strnlen(udm_item->value, MAX_EXTRADATA_VALUE_LEN) == 0)
 			continue;
 
-		/* This doesn't overflow extradata_complete since it will write
-		 * one entry length (1/MAX_EXTRADATA_ITEMS long), entry count is
+		/* This doesn't overflow userdata since it will write
+		 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
 		 * checked to not exceed MAX items with child_count above
 		 */
-		nt->userdata_length += scnprintf(&nt->extradata_complete[nt->userdata_length],
+		nt->userdata_length += scnprintf(&nt->userdata[nt->userdata_length],
 						 MAX_EXTRADATA_ENTRY_LEN, " %s=%s\n",
 						 item->ci_name, udm_item->value);
 	}
@@ -960,7 +950,7 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
 				    enum sysdata_feature feature)
 {
 	nt->sysdata_fields &= ~feature;
-	nt->extradata_complete[nt->userdata_length] = 0;
+	nt->sysdata[0] = 0;
 }
 
 static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
@@ -979,12 +969,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
 	if (msgid_enabled == curr)
 		goto unlock_ok;
 
-	if (msgid_enabled &&
-	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
-		ret = -ENOSPC;
-		goto unlock;
-	}
-
 	if (msgid_enabled)
 		nt->sysdata_fields |= SYSDATA_MSGID;
 	else
@@ -992,7 +976,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
 
 unlock_ok:
 	ret = strnlen(buf, count);
-unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return ret;
 }
@@ -1013,12 +996,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
 	if (release_enabled == curr)
 		goto unlock_ok;
 
-	if (release_enabled &&
-	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
-		ret = -ENOSPC;
-		goto unlock;
-	}
-
 	if (release_enabled)
 		nt->sysdata_fields |= SYSDATA_RELEASE;
 	else
@@ -1026,7 +1003,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
 
 unlock_ok:
 	ret = strnlen(buf, count);
-unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return ret;
 }
@@ -1047,12 +1023,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
 	if (taskname_enabled == curr)
 		goto unlock_ok;
 
-	if (taskname_enabled &&
-	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
-		ret = -ENOSPC;
-		goto unlock;
-	}
-
 	if (taskname_enabled)
 		nt->sysdata_fields |= SYSDATA_TASKNAME;
 	else
@@ -1060,7 +1030,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
 
 unlock_ok:
 	ret = strnlen(buf, count);
-unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return ret;
 }
@@ -1083,27 +1052,16 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 		/* no change requested */
 		goto unlock_ok;
 
-	if (cpu_nr_enabled &&
-	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
-		/* user wants the new feature, but there is no space in the
-		 * buffer.
-		 */
-		ret = -ENOSPC;
-		goto unlock;
-	}
-
 	if (cpu_nr_enabled)
 		nt->sysdata_fields |= SYSDATA_CPU_NR;
 	else
-		/* This is special because extradata_complete might have
-		 * remaining data from previous sysdata, and it needs to be
-		 * cleaned.
+		/* This is special because sysdata might have remaining data
+		 * from previous sysdata, and it needs to be cleaned.
 		 */
 		disable_sysdata_feature(nt, SYSDATA_CPU_NR);
 
 unlock_ok:
 	ret = strnlen(buf, count);
-unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return ret;
 }
@@ -1146,7 +1104,7 @@ static struct config_item *userdatum_make_item(struct config_group *group,
 
 	ud = to_userdata(&group->cg_item);
 	nt = userdata_to_target(ud);
-	if (count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS)
+	if (count_userdata_entries(nt) >= MAX_USERDATA_ITEMS)
 		return ERR_PTR(-ENOSPC);
 
 	udm = kzalloc(sizeof(*udm), GFP_KERNEL);
@@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
 
 static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset)
 {
-	/* Append cpu=%d at extradata_complete after userdata str */
-	return scnprintf(&nt->extradata_complete[offset],
+	return scnprintf(&nt->sysdata[offset],
 			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
 			 raw_smp_processor_id());
 }
 
 static int sysdata_append_taskname(struct netconsole_target *nt, int offset)
 {
-	return scnprintf(&nt->extradata_complete[offset],
+	return scnprintf(&nt->sysdata[offset],
 			 MAX_EXTRADATA_ENTRY_LEN, " taskname=%s\n",
 			 current->comm);
 }
 
 static int sysdata_append_release(struct netconsole_target *nt, int offset)
 {
-	return scnprintf(&nt->extradata_complete[offset],
+	return scnprintf(&nt->sysdata[offset],
 			 MAX_EXTRADATA_ENTRY_LEN, " release=%s\n",
 			 init_utsname()->release);
 }
@@ -1376,46 +1333,36 @@ static int sysdata_append_release(struct netconsole_target *nt, int offset)
 static int sysdata_append_msgid(struct netconsole_target *nt, int offset)
 {
 	wrapping_assign_add(nt->msgcounter, 1);
-	return scnprintf(&nt->extradata_complete[offset],
+	return scnprintf(&nt->sysdata[offset],
 			 MAX_EXTRADATA_ENTRY_LEN, " msgid=%u\n",
 			 nt->msgcounter);
 }
 
 /*
- * prepare_extradata - append sysdata at extradata_complete in runtime
+ * prepare_sysdata - append sysdata in runtime
  * @nt: target to send message to
  */
-static int prepare_extradata(struct netconsole_target *nt)
+static int prepare_sysdata(struct netconsole_target *nt)
 {
-	int extradata_len;
-
-	/* userdata was appended when configfs write helper was called
-	 * by update_userdata().
-	 */
-	extradata_len = nt->userdata_length;
+	int sysdata_len = 0;
 
 	if (!nt->sysdata_fields)
 		goto out;
 
 	if (nt->sysdata_fields & SYSDATA_CPU_NR)
-		extradata_len += sysdata_append_cpu_nr(nt, extradata_len);
+		sysdata_len += sysdata_append_cpu_nr(nt, sysdata_len);
 	if (nt->sysdata_fields & SYSDATA_TASKNAME)
-		extradata_len += sysdata_append_taskname(nt, extradata_len);
+		sysdata_len += sysdata_append_taskname(nt, sysdata_len);
 	if (nt->sysdata_fields & SYSDATA_RELEASE)
-		extradata_len += sysdata_append_release(nt, extradata_len);
+		sysdata_len += sysdata_append_release(nt, sysdata_len);
 	if (nt->sysdata_fields & SYSDATA_MSGID)
-		extradata_len += sysdata_append_msgid(nt, extradata_len);
+		sysdata_len += sysdata_append_msgid(nt, sysdata_len);
 
-	WARN_ON_ONCE(extradata_len >
-		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);
+	WARN_ON_ONCE(sysdata_len >
+		     MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS);
 
 out:
-	return extradata_len;
-}
-#else /* CONFIG_NETCONSOLE_DYNAMIC not set */
-static int prepare_extradata(struct netconsole_target *nt)
-{
-	return 0;
+	return sysdata_len;
 }
 #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
 
@@ -1517,13 +1464,8 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
 				      int msg_len,
 				      int release_len)
 {
-	const char *extradata = NULL;
 	const char *release;
 
-#ifdef CONFIG_NETCONSOLE_DYNAMIC
-	extradata = nt->extradata_complete;
-#endif
-
 	if (release_len) {
 		release = init_utsname()->release;
 
@@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
 		memcpy(nt->buf, msg, msg_len);
 	}
 
-	if (extradata)
-		msg_len += scnprintf(&nt->buf[msg_len],
-				     MAX_PRINT_CHUNK - msg_len,
-				     "%s", extradata);
-
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+	msg_len += scnprintf(&nt->buf[msg_len],
+			     MAX_PRINT_CHUNK - msg_len,
+			     "%s%s", nt->userdata, nt->sysdata);
+#endif
 	send_udp(nt, nt->buf, msg_len);
 }
 
@@ -1551,30 +1493,41 @@ static void append_release(char *buf)
 
 static void send_fragmented_body(struct netconsole_target *nt,
 				 const char *msgbody, int header_len,
-				 int msgbody_len, int extradata_len)
+				 int msgbody_len, int sysdata_len)
 {
-	const char *extradata = NULL;
+	const char *userdata = NULL;
+	const char *sysdata = NULL;
 	int body_len, offset = 0;
-	int extradata_offset = 0;
+	int userdata_offset = 0;
 	int msgbody_offset = 0;
+	int sysdata_offset = 0;
+	int userdata_len = 0;
 
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
-	extradata = nt->extradata_complete;
+	userdata = nt->userdata;
+	sysdata = nt->sysdata;
+	userdata_len = nt->userdata_length;
 #endif
-	if (WARN_ON_ONCE(!extradata && extradata_len != 0))
+	if (WARN_ON_ONCE(!userdata && userdata_len != 0))
+		return;
+
+	if (WARN_ON_ONCE(!sysdata && sysdata_len != 0))
 		return;
 
 	/* body_len represents the number of bytes that will be sent. This is
 	 * bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
 	 * packets
 	 */
-	body_len = msgbody_len + extradata_len;
+	body_len = msgbody_len + userdata_len + sysdata_len;
 
 	/* In each iteration of the while loop below, we send a packet
 	 * containing the header and a portion of the body. The body is
-	 * composed of two parts: msgbody and extradata. We keep track of how
-	 * many bytes have been sent so far using the offset variable, which
-	 * ranges from 0 to the total length of the body.
+	 * composed of three parts: msgbody, userdata and sysdata.
+	 * We keep track of how many bytes have been sent from each part using
+	 * the *_offset variables.
+	 * We keep track of how many bytes have been sent overall using the
+	 * offset variable, which ranges from 0 to the total length of the
+	 * body.
 	 */
 	while (offset < body_len) {
 		int this_header = header_len;
@@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt,
 		msgbody_offset += this_chunk;
 		this_offset += this_chunk;
 
-		/* after msgbody, append extradata */
-		this_chunk = min(extradata_len - extradata_offset,
+		/* after msgbody, append userdata */
+		this_chunk = min(userdata_len - userdata_offset,
 				 MAX_PRINT_CHUNK - this_header - this_offset);
 		memcpy(nt->buf + this_header + this_offset,
-		       extradata + extradata_offset, this_chunk);
-		extradata_offset += this_chunk;
+		       userdata + userdata_offset, this_chunk);
+		userdata_offset += this_chunk;
+		this_offset += this_chunk;
+
+		/* after userdata, append sysdata */
+		this_chunk = min(sysdata_len - sysdata_offset,
+				 MAX_PRINT_CHUNK - this_header - this_offset);
+		memcpy(nt->buf + this_header + this_offset,
+		       sysdata + sysdata_offset, this_chunk);
+		sysdata_offset += this_chunk;
 		this_offset += this_chunk;
 
 		/* if all is good, send the packet out */
@@ -1615,7 +1576,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 				const char *msg,
 				int msg_len,
 				int release_len,
-				int extradata_len)
+				int sysdata_len)
 {
 	int header_len, msgbody_len;
 	const char *msgbody;
@@ -1644,7 +1605,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	 * will be replaced
 	 */
 	send_fragmented_body(nt, msgbody, header_len, msgbody_len,
-			     extradata_len);
+			     sysdata_len);
 }
 
 /**
@@ -1660,19 +1621,22 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 			     int msg_len)
 {
+	int userdata_len = 0;
 	int release_len = 0;
-	int extradata_len;
-
-	extradata_len = prepare_extradata(nt);
+	int sysdata_len = 0;
 
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+	sysdata_len = prepare_sysdata(nt);
+	userdata_len = nt->userdata_length;
+#endif
 	if (nt->release)
 		release_len = strlen(init_utsname()->release) + 1;
 
-	if (msg_len + release_len + extradata_len <= MAX_PRINT_CHUNK)
+	if (msg_len + release_len + sysdata_len + userdata_len <= MAX_PRINT_CHUNK)
 		return send_msg_no_fragmentation(nt, msg, msg_len, release_len);
 
 	return send_msg_fragmented(nt, msg, msg_len, release_len,
-				   extradata_len);
+				   sysdata_len);
 }
 
 static void write_ext_msg(struct console *con, const char *msg,

-- 
2.47.3

[PATCH net-next 3/4] netconsole: Dynamic allocation of userdata buffer

[Gustavo Luiz Duarte]

The userdata buffer in struct netconsole_target is currently statically
allocated with a size of MAX_USERDATA_ITEMS * MAX_EXTRADATA_ENTRY_LEN
(16 * 256 = 4096 bytes). This wastes memory when userdata entries are
not used or when only a few entries are configured, which is common in
typical usage scenarios. It also forces us to keep MAX_USERDATA_ITEMS
small to limit the memory wasted.

Change the userdata buffer from a static array to a dynamically
allocated pointer. The buffer is now allocated on-demand in
update_userdata() whenever userdata entries are added, modified, or
removed via configfs. The implementation calculates the exact size
needed for all current userdata entries, allocates a new buffer of that
size, formats the entries into it, and atomically swaps it with the old
buffer.

This approach provides several benefits:
- Memory efficiency: Targets with no userdata use zero bytes instead of
  4KB, and targets with userdata only allocate what they need;
- Scalability: Makes it practical to increase MAX_USERDATA_ITEMS to a
  much larger value without imposing a fixed memory cost on every
  target;
- No hot-path overhead: Allocation occurs during configuration (write to
  configfs), not during message transmission

If memory allocation fails during userdata update, -ENOMEM is returned
to userspace through the configfs attribute write operation.

The sysdata buffer remains statically allocated since it has a smaller
fixed size (MAX_SYSDATA_ITEMS * MAX_EXTRADATA_ENTRY_LEN = 4 * 256 = 1024
bytes) and its content length is less predictable.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 87 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 31 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index e780c884db83..8a11b3ca2763 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -154,7 +154,7 @@ struct netconsole_target {
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	struct config_group	group;
 	struct config_group	userdata_group;
-	char			userdata[MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS];
+	char			*userdata;
 	size_t			userdata_length;
 	char			sysdata[MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS];
 
@@ -874,45 +874,61 @@ static ssize_t userdatum_value_show(struct config_item *item, char *buf)
 	return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
 }
 
-static void update_userdata(struct netconsole_target *nt)
+static int update_userdata(struct netconsole_target *nt)
 {
+	struct userdatum *udm_item;
+	struct config_item *item;
 	struct list_head *entry;
-	int child_count = 0;
+	char *old_buf = NULL;
+	char *new_buf = NULL;
 	unsigned long flags;
+	int offset = 0;
+	int len = 0;
 
-	spin_lock_irqsave(&target_list_lock, flags);
-
-	/* Clear the current string in case the last userdatum was deleted */
-	nt->userdata_length = 0;
-	nt->userdata[0] = 0;
-
+	/* Calculate buffer size */
 	list_for_each(entry, &nt->userdata_group.cg_children) {
-		struct userdatum *udm_item;
-		struct config_item *item;
-
-		if (child_count >= MAX_USERDATA_ITEMS) {
-			spin_unlock_irqrestore(&target_list_lock, flags);
-			WARN_ON_ONCE(1);
-			return;
+		item = container_of(entry, struct config_item, ci_entry);
+		udm_item = to_userdatum(item);
+		/* Skip userdata with no value set */
+		if (udm_item->value[0]) {
+			len += snprintf(NULL, 0, " %s=%s\n", item->ci_name,
+					udm_item->value);
 		}
-		child_count++;
+	}
+
+	WARN_ON_ONCE(len > MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS);
+
+	/* Allocate new buffer */
+	if (len) {
+		new_buf = kmalloc(len + 1, GFP_KERNEL);
+		if (!new_buf)
+			return -ENOMEM;
+	}
 
+	/* Write userdata to new buffer */
+	list_for_each(entry, &nt->userdata_group.cg_children) {
 		item = container_of(entry, struct config_item, ci_entry);
 		udm_item = to_userdatum(item);
-
 		/* Skip userdata with no value set */
-		if (strnlen(udm_item->value, MAX_EXTRADATA_VALUE_LEN) == 0)
-			continue;
-
-		/* This doesn't overflow userdata since it will write
-		 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
-		 * checked to not exceed MAX items with child_count above
-		 */
-		nt->userdata_length += scnprintf(&nt->userdata[nt->userdata_length],
-						 MAX_EXTRADATA_ENTRY_LEN, " %s=%s\n",
-						 item->ci_name, udm_item->value);
+		if (udm_item->value[0]) {
+			offset += scnprintf(&new_buf[offset], len + 1 - offset,
+					    " %s=%s\n", item->ci_name,
+					    udm_item->value);
+		}
 	}
+
+	WARN_ON_ONCE(offset != len);
+
+	/* 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 = len;
 	spin_unlock_irqrestore(&target_list_lock, flags);
+
+	kfree(old_buf);
+
+	return 0;
 }
 
 static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
@@ -935,7 +951,9 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 
 	ud = to_userdata(item->ci_parent);
 	nt = userdata_to_target(ud);
-	update_userdata(nt);
+	ret = update_userdata(nt);
+	if (ret < 0)
+		goto out_unlock;
 	ret = count;
 out_unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
@@ -1182,7 +1200,10 @@ static struct configfs_attribute *netconsole_target_attrs[] = {
 
 static void netconsole_target_release(struct config_item *item)
 {
-	kfree(to_target(item));
+	struct netconsole_target *nt = to_target(item);
+
+	kfree(nt->userdata);
+	kfree(nt);
 }
 
 static struct configfs_item_operations netconsole_target_item_ops = {
@@ -1478,7 +1499,8 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
 #ifdef CONFIG_NETCONSOLE_DYNAMIC
 	msg_len += scnprintf(&nt->buf[msg_len],
 			     MAX_PRINT_CHUNK - msg_len,
-			     "%s%s", nt->userdata, nt->sysdata);
+			     "%s%s", nt->userdata ? nt->userdata : "",
+			     nt->sysdata);
 #endif
 	send_udp(nt, nt->buf, msg_len);
 }
@@ -1841,6 +1863,9 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 static void free_param_target(struct netconsole_target *nt)
 {
 	netpoll_cleanup(&nt->np);
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+	kfree(nt->userdata);
+#endif
 	kfree(nt);
 }
 

-- 
2.47.3

[PATCH net-next 4/4] netconsole: Increase MAX_USERDATA_ITEMS

[Gustavo Luiz Duarte]

Increase MAX_USERDATA_ITEMS from 16 to 256 entries now that the userdata
buffer is allocated dynamically.

The previous limit of 16 was necessary because the buffer was statically
allocated for all targets. With dynamic allocation, we can support more
entries without wasting memory on targets that don't use userdata.

This allows users to attach more metadata to their netconsole messages,
which is useful for complex debugging and logging scenarios.

Also update the testcase accordingly.

Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c                                | 2 +-
 tools/testing/selftests/drivers/net/netcons_overflow.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 8a11b3ca2763..040bae29d485 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -50,7 +50,7 @@ MODULE_LICENSE("GPL");
 /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
 #define MAX_EXTRADATA_NAME_LEN		(MAX_EXTRADATA_ENTRY_LEN - \
 					MAX_EXTRADATA_VALUE_LEN - 3)
-#define MAX_USERDATA_ITEMS		16
+#define MAX_USERDATA_ITEMS		256
 #define MAX_SYSDATA_ITEMS		4
 #define MAX_PRINT_CHUNK			1000
 
diff --git a/tools/testing/selftests/drivers/net/netcons_overflow.sh b/tools/testing/selftests/drivers/net/netcons_overflow.sh
index 29bad56448a2..06089643b771 100755
--- a/tools/testing/selftests/drivers/net/netcons_overflow.sh
+++ b/tools/testing/selftests/drivers/net/netcons_overflow.sh
@@ -15,7 +15,7 @@ SCRIPTDIR=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")
 
 source "${SCRIPTDIR}"/lib/sh/lib_netcons.sh
 # This is coming from netconsole code. Check for it in drivers/net/netconsole.c
-MAX_USERDATA_ITEMS=16
+MAX_USERDATA_ITEMS=256
 
 # Function to create userdata entries
 function create_userdata_max_entries() {

-- 
2.47.3

Re: [PATCH net-next 1/4] netconsole: Simplify send_fragmented_body()

[Breno Leitao]

On Wed, Nov 05, 2025 at 09:06:43AM -0800, Gustavo Luiz Duarte wrote:
> Refactor send_fragmented_body() to use separate offset tracking for
> msgbody, and extradata instead of complex conditional logic.
> The previous implementation used boolean flags and calculated offsets
> which made the code harder to follow.
> 
> The new implementation maintains independent offset counters
> (msgbody_offset, extradata_offset) and processes each section
> sequentially, making the data flow more straightforward and the code
> easier to maintain.
> 
> This is a preparatory refactoring with no functional changes, which will
> allow easily splitting extradata_complete into separate userdata and
> sysdata buffers in the next patch.
> 
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> ---
>  drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
>  1 file changed, 24 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 5d8d0214786c..0a8ba7c4bc9d 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
>  				 const char *msgbody, int header_len,
>  				 int msgbody_len, int extradata_len)
>  {
> -	int sent_extradata, preceding_bytes;
>  	const char *extradata = NULL;
>  	int body_len, offset = 0;
> +	int extradata_offset = 0;
> +	int msgbody_offset = 0;
>  
>  #ifdef CONFIG_NETCONSOLE_DYNAMIC
>  	extradata = nt->extradata_complete;
>  #endif


extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
unset. Basically extradata=NULL will not be replaced.

> +	if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> +		return;

And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.

> +		/* write msgbody first */
> +		this_chunk = min(msgbody_len - msgbody_offset,
> +				 MAX_PRINT_CHUNK - this_header);
> +		memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> +		       this_chunk);
> +		msgbody_offset += this_chunk;
> +		this_offset += this_chunk;
> +
> +		/* after msgbody, append extradata */
> +		this_chunk = min(extradata_len - extradata_offset,
> +				 MAX_PRINT_CHUNK - this_header - this_offset);
> +		memcpy(nt->buf + this_header + this_offset,
> +		       extradata + extradata_offset, this_chunk);

then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).

I got this my vim LSP that printed:

	Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]

Re: [PATCH net-next 2/4] netconsole: Split userdata and sysdata

[Breno Leitao]

On Wed, Nov 05, 2025 at 09:06:44AM -0800, Gustavo Luiz Duarte wrote:
> Separate userdata and sysdata into distinct buffers to enable independent
> management. Previously, both were stored in a single extradata_complete
> buffer with a fixed size that accommodated both types of data.
> 
> This separation allows:
> - userdata to grow dynamically (in subsequent patch)
> - sysdata to remain in a small static buffer
> - removal of complex entry counting logic that tracked both types together
> 
> The split also simplifies the code by eliminating the need to check total
> entry count across both userdata and sysdata when enabling features,
> which allows to drop holding su_mutex on sysdata_*_enabled_store().
> 
> No functional change in this patch, just structural preparation for
> dynamic userdata allocation.
> 
> Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> ---
>  drivers/net/netconsole.c | 204 +++++++++++++++++++----------------------------
>  1 file changed, 84 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 0a8ba7c4bc9d..e780c884db83 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -50,7 +50,8 @@ MODULE_LICENSE("GPL");
>  /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
>  #define MAX_EXTRADATA_NAME_LEN		(MAX_EXTRADATA_ENTRY_LEN - \
>  					MAX_EXTRADATA_VALUE_LEN - 3)
> -#define MAX_EXTRADATA_ITEMS		16
> +#define MAX_USERDATA_ITEMS		16

Do we still need to have MAX_USERDATA_ITEMS cap with your new approach?

> +#define MAX_SYSDATA_ITEMS		4

Can we have this one inside enum sysdata_feature?

Something as:

  enum sysdata_feature {
      SYSDATA_CPU_NR = BIT(0),
      SYSDATA_TASKNAME = BIT(1),
      SYSDATA_RELEASE = BIT(2),
      SYSDATA_MSGID = BIT(3),
      MAX_SYSDATA_ITEMS = 4  /* Sentinel: highest bit position */
  };

>  #define MAX_PRINT_CHUNK			1000
>  
>  static char config[MAX_PARAM_LENGTH];
> @@ -122,8 +123,9 @@ enum sysdata_feature {
>   * @list:	Links this target into the target_list.
>   * @group:	Links us into the configfs subsystem hierarchy.
>   * @userdata_group:	Links to the userdata configfs hierarchy
> - * @extradata_complete:	Cached, formatted string of append
> - * @userdata_length:	String length of usedata in extradata_complete.
> + * @userdata:		Cached, formatted string of append
> + * @userdata_length:	String length of userdata.
> + * @sysdata:		Cached, formatted string of append
>   * @sysdata_fields:	Sysdata features enabled.
>   * @msgcounter:	Message sent counter.
>   * @stats:	Packet send stats for the target. Used for debugging.
> @@ -152,8 +154,10 @@ struct netconsole_target {
>  #ifdef	CONFIG_NETCONSOLE_DYNAMIC
>  	struct config_group	group;
>  	struct config_group	userdata_group;
> -	char extradata_complete[MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS];
> +	char			userdata[MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS];
>  	size_t			userdata_length;
> +	char			sysdata[MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS];
> +
>  	/* bit-wise with sysdata_feature bits */
>  	u32			sysdata_fields;
>  	/* protected by target_list_lock */
> @@ -802,28 +806,14 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
>  	return ret;
>  }
>  
> -/* Count number of entries we have in extradata.
> - * This is important because the extradata_complete only supports
> - * MAX_EXTRADATA_ITEMS entries. Before enabling any new {user,sys}data
> - * feature, number of entries needs to checked for available space.
> +/* Count number of entries we have in userdata.
> + * This is important because userdata only supports MAX_USERDATA_ITEMS
> + * entries. Before enabling any new userdata feature, number of entries needs
> + * to checked for available space.
>   */
> -static size_t count_extradata_entries(struct netconsole_target *nt)
> +static size_t count_userdata_entries(struct netconsole_target *nt)
>  {
> -	size_t entries;
> -
> -	/* Userdata entries */
> -	entries = list_count_nodes(&nt->userdata_group.cg_children);
> -	/* Plus sysdata entries */
> -	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_TASKNAME)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_RELEASE)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_MSGID)
> -		entries += 1;
> -
> -	return entries;
> +	return list_count_nodes(&nt->userdata_group.cg_children);
>  }
>  
>  static ssize_t remote_mac_store(struct config_item *item, const char *buf,
> @@ -894,13 +884,13 @@ static void update_userdata(struct netconsole_target *nt)
>  
>  	/* Clear the current string in case the last userdatum was deleted */
>  	nt->userdata_length = 0;
> -	nt->extradata_complete[0] = 0;
> +	nt->userdata[0] = 0;
>  
>  	list_for_each(entry, &nt->userdata_group.cg_children) {
>  		struct userdatum *udm_item;
>  		struct config_item *item;
>  
> -		if (child_count >= MAX_EXTRADATA_ITEMS) {
> +		if (child_count >= MAX_USERDATA_ITEMS) {
>  			spin_unlock_irqrestore(&target_list_lock, flags);
>  			WARN_ON_ONCE(1);
>  			return;
> @@ -914,11 +904,11 @@ static void update_userdata(struct netconsole_target *nt)
>  		if (strnlen(udm_item->value, MAX_EXTRADATA_VALUE_LEN) == 0)
>  			continue;
>  
> -		/* This doesn't overflow extradata_complete since it will write
> -		 * one entry length (1/MAX_EXTRADATA_ITEMS long), entry count is
> +		/* This doesn't overflow userdata since it will write
> +		 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
>  		 * checked to not exceed MAX items with child_count above
>  		 */
> -		nt->userdata_length += scnprintf(&nt->extradata_complete[nt->userdata_length],
> +		nt->userdata_length += scnprintf(&nt->userdata[nt->userdata_length],
>  						 MAX_EXTRADATA_ENTRY_LEN, " %s=%s\n",
>  						 item->ci_name, udm_item->value);
>  	}
> @@ -960,7 +950,7 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
>  				    enum sysdata_feature feature)
>  {
>  	nt->sysdata_fields &= ~feature;
> -	nt->extradata_complete[nt->userdata_length] = 0;
> +	nt->sysdata[0] = 0;
>  }
>  
>  static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
> @@ -979,12 +969,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
>  	if (msgid_enabled == curr)
>  		goto unlock_ok;
>  
> -	if (msgid_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (msgid_enabled)
>  		nt->sysdata_fields |= SYSDATA_MSGID;
>  	else
> @@ -992,7 +976,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1013,12 +996,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
>  	if (release_enabled == curr)
>  		goto unlock_ok;
>  
> -	if (release_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (release_enabled)
>  		nt->sysdata_fields |= SYSDATA_RELEASE;
>  	else
> @@ -1026,7 +1003,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1047,12 +1023,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
>  	if (taskname_enabled == curr)
>  		goto unlock_ok;
>  
> -	if (taskname_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (taskname_enabled)
>  		nt->sysdata_fields |= SYSDATA_TASKNAME;
>  	else
> @@ -1060,7 +1030,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1083,27 +1052,16 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
>  		/* no change requested */
>  		goto unlock_ok;
>  
> -	if (cpu_nr_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		/* user wants the new feature, but there is no space in the
> -		 * buffer.
> -		 */
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (cpu_nr_enabled)
>  		nt->sysdata_fields |= SYSDATA_CPU_NR;
>  	else
> -		/* This is special because extradata_complete might have
> -		 * remaining data from previous sysdata, and it needs to be
> -		 * cleaned.
> +		/* This is special because sysdata might have remaining data
> +		 * from previous sysdata, and it needs to be cleaned.
>  		 */
>  		disable_sysdata_feature(nt, SYSDATA_CPU_NR);
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1146,7 +1104,7 @@ static struct config_item *userdatum_make_item(struct config_group *group,
>  
>  	ud = to_userdata(&group->cg_item);
>  	nt = userdata_to_target(ud);
> -	if (count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS)
> +	if (count_userdata_entries(nt) >= MAX_USERDATA_ITEMS)
>  		return ERR_PTR(-ENOSPC);
>  
>  	udm = kzalloc(sizeof(*udm), GFP_KERNEL);
> @@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
>  
>  static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset)
>  {
> -	/* Append cpu=%d at extradata_complete after userdata str */
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",

This is confusing. It is writing to sysdata but checking extradata entry len.

>  static int sysdata_append_taskname(struct netconsole_target *nt, int offset)
>  {
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " taskname=%s\n",

Same as above.

>  }
>  
>  static int sysdata_append_release(struct netconsole_target *nt, int offset)
>  {
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " release=%s\n",
>  			 init_utsname()->release);
>  }
> @@ -1376,46 +1333,36 @@ static int sysdata_append_release(struct netconsole_target *nt, int offset)
>  static int sysdata_append_msgid(struct netconsole_target *nt, int offset)
>  {
>  	wrapping_assign_add(nt->msgcounter, 1);
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " msgid=%u\n",
>  			 nt->msgcounter);
>  }
>  
>  /*
> - * prepare_extradata - append sysdata at extradata_complete in runtime
> + * prepare_sysdata - append sysdata in runtime
>   * @nt: target to send message to
>   */
> -static int prepare_extradata(struct netconsole_target *nt)
> +static int prepare_sysdata(struct netconsole_target *nt)
>  {
> -	int extradata_len;
> -
> -	/* userdata was appended when configfs write helper was called
> -	 * by update_userdata().
> -	 */
> -	extradata_len = nt->userdata_length;
> +	int sysdata_len = 0;
>  
>  	if (!nt->sysdata_fields)
>  		goto out;
>  
>  	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> -		extradata_len += sysdata_append_cpu_nr(nt, extradata_len);
> +		sysdata_len += sysdata_append_cpu_nr(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_TASKNAME)
> -		extradata_len += sysdata_append_taskname(nt, extradata_len);
> +		sysdata_len += sysdata_append_taskname(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_RELEASE)
> -		extradata_len += sysdata_append_release(nt, extradata_len);
> +		sysdata_len += sysdata_append_release(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_MSGID)
> -		extradata_len += sysdata_append_msgid(nt, extradata_len);
> +		sysdata_len += sysdata_append_msgid(nt, sysdata_len);
>  
> -	WARN_ON_ONCE(extradata_len >
> -		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);
> +	WARN_ON_ONCE(sysdata_len >
> +		     MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS);
>  
>  out:
> -	return extradata_len;
> -}
> -#else /* CONFIG_NETCONSOLE_DYNAMIC not set */
> -static int prepare_extradata(struct netconsole_target *nt)
> -{
> -	return 0;
> +	return sysdata_len;
>  }
>  #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
>  
> @@ -1517,13 +1464,8 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
>  				      int msg_len,
>  				      int release_len)
>  {
> -	const char *extradata = NULL;
>  	const char *release;
>  
> -#ifdef CONFIG_NETCONSOLE_DYNAMIC
> -	extradata = nt->extradata_complete;
> -#endif
> -
>  	if (release_len) {
>  		release = init_utsname()->release;
>  
> @@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
>  		memcpy(nt->buf, msg, msg_len);
>  	}
>  
> -	if (extradata)
> -		msg_len += scnprintf(&nt->buf[msg_len],
> -				     MAX_PRINT_CHUNK - msg_len,
> -				     "%s", extradata);
> -
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +	msg_len += scnprintf(&nt->buf[msg_len],
> +			     MAX_PRINT_CHUNK - msg_len,
> +			     "%s%s", nt->userdata, nt->sysdata);
> +#endif

I am not sure I like this ifdef in here. Can you if userdata or sysdata are
valid, and then scnprintf() instead of using ifdef?

>  				 const char *msgbody, int header_len,
> -				 int msgbody_len, int extradata_len)
> +				 int msgbody_len, int sysdata_len)
>  {
> -	const char *extradata = NULL;
> +	const char *userdata = NULL;
> +	const char *sysdata = NULL;
>  	int body_len, offset = 0;
> -	int extradata_offset = 0;
> +	int userdata_offset = 0;
>  	int msgbody_offset = 0;
> +	int sysdata_offset = 0;
> +	int userdata_len = 0;
>  
>  #ifdef CONFIG_NETCONSOLE_DYNAMIC
> -	extradata = nt->extradata_complete;
> +	userdata = nt->userdata;
> +	sysdata = nt->sysdata;
> +	userdata_len = nt->userdata_length;
>  #endif
> -	if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> +	if (WARN_ON_ONCE(!userdata && userdata_len != 0))
> +		return;
> +
> +	if (WARN_ON_ONCE(!sysdata && sysdata_len != 0))
>  		return;
>  
>  	/* body_len represents the number of bytes that will be sent. This is
>  	 * bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
>  	 * packets
>  	 */
> -	body_len = msgbody_len + extradata_len;
> +	body_len = msgbody_len + userdata_len + sysdata_len;
>  
>  	/* In each iteration of the while loop below, we send a packet
>  	 * containing the header and a portion of the body. The body is
> -	 * composed of two parts: msgbody and extradata. We keep track of how
> -	 * many bytes have been sent so far using the offset variable, which
> -	 * ranges from 0 to the total length of the body.
> +	 * composed of three parts: msgbody, userdata and sysdata.
> +	 * We keep track of how many bytes have been sent from each part using
> +	 * the *_offset variables.
> +	 * We keep track of how many bytes have been sent overall using the
> +	 * offset variable, which ranges from 0 to the total length of the
> +	 * body.
>  	 */
>  	while (offset < body_len) {
>  		int this_header = header_len;
> @@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt,
>  		msgbody_offset += this_chunk;
>  		this_offset += this_chunk;
>  
> -		/* after msgbody, append extradata */
> -		this_chunk = min(extradata_len - extradata_offset,
> +		/* after msgbody, append userdata */
> +		this_chunk = min(userdata_len - userdata_offset,

Please assign this "userdata_len - userdata_offset" to a variable and give it a
name, so, it help us to reason about the code. 

>  				 MAX_PRINT_CHUNK - this_header - this_offset);
>  		memcpy(nt->buf + this_header + this_offset,
> -		       extradata + extradata_offset, this_chunk);
> -		extradata_offset += this_chunk;
> +		       userdata + userdata_offset, this_chunk);
> +		userdata_offset += this_chunk;
> +		this_offset += this_chunk;
> +
> +		/* after userdata, append sysdata */
> +		this_chunk = min(sysdata_len - sysdata_offset,
> +				 MAX_PRINT_CHUNK - this_header - this_offset);
> +		memcpy(nt->buf + this_header + this_offset,
> +		       sysdata + sysdata_offset, this_chunk);

s/this_header/this_header_offset?

Now that you are touching this code, please review these variable to keep them named correct.

Maybe adding _ptr for pointer, and _offset for offsets and _len for lenghts?

Thank you for your work reasong about all of this!
--breno

Re: [PATCH net-next 1/4] netconsole: Simplify send_fragmented_body()

[Gustavo Luiz Duarte]

On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@debian.org> wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> >                                const char *msgbody, int header_len,
> >                                int msgbody_len, int extradata_len)
> >  {
> > -     int sent_extradata, preceding_bytes;
> >       const char *extradata = NULL;
> >       int body_len, offset = 0;
> > +     int extradata_offset = 0;
> > +     int msgbody_offset = 0;
> >
> >  #ifdef CONFIG_NETCONSOLE_DYNAMIC
> >       extradata = nt->extradata_complete;
> >  #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > +     if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > +             return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > +             /* write msgbody first */
> > +             this_chunk = min(msgbody_len - msgbody_offset,
> > +                              MAX_PRINT_CHUNK - this_header);
> > +             memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > +                    this_chunk);
> > +             msgbody_offset += this_chunk;
> > +             this_offset += this_chunk;
> > +
> > +             /* after msgbody, append extradata */
> > +             this_chunk = min(extradata_len - extradata_offset,
> > +                              MAX_PRINT_CHUNK - this_header - this_offset);
> > +             memcpy(nt->buf + this_header + this_offset,
> > +                    extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).

I believe passing NULL to memcpy() should be safe as long as count is
zero (which is the case here).
However, what I didn't realize at first is that we would be doing
pointer arithmetic with NULL, which is undefined behavior :(
I will add a check if extradata is NULL here.

Thanks for the careful review!

>
> I got this my vim LSP that printed:
>
>         Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>

On Fri, Nov 7, 2025 at 12:15 PM Breno Leitao <leitao@debian.org> wrote:
>
> On Wed, Nov 05, 2025 at 09:06:43AM -0800, Gustavo Luiz Duarte wrote:
> > Refactor send_fragmented_body() to use separate offset tracking for
> > msgbody, and extradata instead of complex conditional logic.
> > The previous implementation used boolean flags and calculated offsets
> > which made the code harder to follow.
> >
> > The new implementation maintains independent offset counters
> > (msgbody_offset, extradata_offset) and processes each section
> > sequentially, making the data flow more straightforward and the code
> > easier to maintain.
> >
> > This is a preparatory refactoring with no functional changes, which will
> > allow easily splitting extradata_complete into separate userdata and
> > sysdata buffers in the next patch.
> >
> > Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> > ---
> >  drivers/net/netconsole.c | 73 ++++++++++++++++--------------------------------
> >  1 file changed, 24 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5d8d0214786c..0a8ba7c4bc9d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1553,13 +1553,16 @@ static void send_fragmented_body(struct netconsole_target *nt,
> >                                const char *msgbody, int header_len,
> >                                int msgbody_len, int extradata_len)
> >  {
> > -     int sent_extradata, preceding_bytes;
> >       const char *extradata = NULL;
> >       int body_len, offset = 0;
> > +     int extradata_offset = 0;
> > +     int msgbody_offset = 0;
> >
> >  #ifdef CONFIG_NETCONSOLE_DYNAMIC
> >       extradata = nt->extradata_complete;
> >  #endif
>
>
> extradata could be NULL at this time if CONFIG_NETCONSOLE_DYNAMIC is
> unset. Basically extradata=NULL will not be replaced.
>
> > +     if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> > +             return;
>
> And entradata_len = 0 for CONFIG_NETCONSOLE_DYNAMIC disabled.
>
> > +             /* write msgbody first */
> > +             this_chunk = min(msgbody_len - msgbody_offset,
> > +                              MAX_PRINT_CHUNK - this_header);
> > +             memcpy(nt->buf + this_header, msgbody + msgbody_offset,
> > +                    this_chunk);
> > +             msgbody_offset += this_chunk;
> > +             this_offset += this_chunk;
> > +
> > +             /* after msgbody, append extradata */
> > +             this_chunk = min(extradata_len - extradata_offset,
> > +                              MAX_PRINT_CHUNK - this_header - this_offset);
> > +             memcpy(nt->buf + this_header + this_offset,
> > +                    extradata + extradata_offset, this_chunk);
>
> then you are going to memcpy from NULL pointer (`extradata + extradata_offset` == 0).
>
> I got this my vim LSP that printed:
>
>         Null pointer passed as 2nd argument to memory copy function [unix.cstring.NullArg]
>

Re: [PATCH net-next 2/4] netconsole: Split userdata and sysdata

[Gustavo Luiz Duarte]

On Fri, Nov 7, 2025 at 1:23 PM Breno Leitao <leitao@debian.org> wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 0a8ba7c4bc9d..e780c884db83 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -50,7 +50,8 @@ MODULE_LICENSE("GPL");
> >  /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
> >  #define MAX_EXTRADATA_NAME_LEN               (MAX_EXTRADATA_ENTRY_LEN - \
> >                                       MAX_EXTRADATA_VALUE_LEN - 3)
> > -#define MAX_EXTRADATA_ITEMS          16
> > +#define MAX_USERDATA_ITEMS           16
>
> Do we still need to have MAX_USERDATA_ITEMS cap with your new approach?

That is a good point. I did think about this and ended up deciding to
keep a limit as a safety measure against userspace creating a boatload
of items until we run out of memory.

>
> > +#define MAX_SYSDATA_ITEMS            4
>
> Can we have this one inside enum sysdata_feature?
>
> Something as:
>
>   enum sysdata_feature {
>       SYSDATA_CPU_NR = BIT(0),
>       SYSDATA_TASKNAME = BIT(1),
>       SYSDATA_RELEASE = BIT(2),
>       SYSDATA_MSGID = BIT(3),
>       MAX_SYSDATA_ITEMS = 4  /* Sentinel: highest bit position */
>   };

Sure, I will do this in v2.


> > @@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
> >
> >  static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset)
> >  {
> > -     /* Append cpu=%d at extradata_complete after userdata str */
> > -     return scnprintf(&nt->extradata_complete[offset],
> > +     return scnprintf(&nt->sysdata[offset],
> >                        MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
>
> This is confusing. It is writing to sysdata but checking extradata entry len.

My understanding is that extradata is a way to refer to both userdata
and sysdata, and MAX_EXTRADATA_ENTRY_LEN is the maximum size for both
(and the arithmetic used to define MAX_EXTRADATA_VALUE_LEN and
MAX_EXTRADATA_NAME_LEN also applies to both). Do you want to define
separate maximum sizes for userdata items and sysdata items?

> > @@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
> >               memcpy(nt->buf, msg, msg_len);
> >       }
> >
> > -     if (extradata)
> > -             msg_len += scnprintf(&nt->buf[msg_len],
> > -                                  MAX_PRINT_CHUNK - msg_len,
> > -                                  "%s", extradata);
> > -
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +     msg_len += scnprintf(&nt->buf[msg_len],
> > +                          MAX_PRINT_CHUNK - msg_len,
> > +                          "%s%s", nt->userdata, nt->sysdata);
> > +#endif
>
> I am not sure I like this ifdef in here. Can you if userdata or sysdata are
> valid, and then scnprintf() instead of using ifdef?

OK, will do that in v2.


> > @@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt,
> >               msgbody_offset += this_chunk;
> >               this_offset += this_chunk;
> >
> > -             /* after msgbody, append extradata */
> > -             this_chunk = min(extradata_len - extradata_offset,
> > +             /* after msgbody, append userdata */
> > +             this_chunk = min(userdata_len - userdata_offset,
>
> Please assign this "userdata_len - userdata_offset" to a variable and give it a
> name, so, it help us to reason about the code.

What about:

int data_remaining = userdata_len - userdata_offset;
int buffer_remaining = MAX_PRINT_CHUNK - this_header - this_offset;
this_chunk = min(data_remaining, buffer_remaining);

>
> >                                MAX_PRINT_CHUNK - this_header - this_offset);
> >               memcpy(nt->buf + this_header + this_offset,
> > -                    extradata + extradata_offset, this_chunk);
> > -             extradata_offset += this_chunk;
> > +                    userdata + userdata_offset, this_chunk);
> > +             userdata_offset += this_chunk;
> > +             this_offset += this_chunk;
> > +
> > +             /* after userdata, append sysdata */
> > +             this_chunk = min(sysdata_len - sysdata_offset,
> > +                              MAX_PRINT_CHUNK - this_header - this_offset);
> > +             memcpy(nt->buf + this_header + this_offset,
> > +                    sysdata + sysdata_offset, this_chunk);

I realize we have the same NULL pointer arithmetic problem here. I
will fix it by checking if sysdata or userdata is NULL.

>
> s/this_header/this_header_offset?

I just realized that this_header is not even necessary. I can simply
add header_len to this_offset and get rid of this variable altogether.

>
> Now that you are touching this code, please review these variable to keep them named correct.
>
> Maybe adding _ptr for pointer, and _offset for offsets and _len for lenghts?

Once I get rid of this_header and add the _ptr suffix, I think it will
be much clearer.

Also, I find offset and this_offset confusing. What about replacing by
msg_offset and buffer_offset ?

>
> Thank you for your work reasong about all of this!

Thanks for the review!