[PATCH net-next v2 0/8] netconsole: Add userdata append support

[PATCH net-next v2 0/8] netconsole: Add userdata append support

[Matthew Wood]

Add the ability to add custom userdata to every outbound netconsole message
as a collection of key/value pairs, allowing users to add metadata to every
netconsole message which can be used for  for tagging, filtering, and
aggregating log messages.

In a previous patch series the ability to prepend the uname release was
added towards the goals above. This patch series builds on that
idea to allow any userdata, keyed by a user provided name, to be
included in netconsole messages.

If CONFIG_NETCONSOLE_DYNAMIC is enabled an additional userdata
directory will be presented in the netconsole configfs tree, allowing
the addition of userdata entries.

    /sys/kernel/config/netconsole/
			<target>/
				enabled
				release
				dev_name
				local_port
				remote_port
				local_ip
				remote_ip
				local_mac
				remote_mac
				userdata/
					<key>/
						value
					<key>/
						value
          ...

v1->v2:
 * Updated netconsole_target docs, kdoc is now clean

--------

Testing for this series is as follows:

Build every patch without CONFIG_NETCONSOLE_DYNAMIC, and also built
with CONFIG_NETCONSOLE_DYNAMIC enabled for every patch after the config
option was added

Test Userdata configfs

    # Adding userdata
    cd /sys/kernel/config/netconsole/ && mkdir cmdline0 && cd cmdline0
    mkdir userdata/release && echo hotfix1 > userdata/release/value
    preview=$(for f in `ls userdata`; do echo $f=$(cat userdata/$f/value); done)
    [[ "$preview" == $'release=hotfix1' ]] && echo pass || echo fail
    mkdir userdata/testing && echo something > userdata/testing/value
    preview=$(for f in `ls userdata`; do echo $f=$(cat userdata/$f/value); done)
    [[ "$preview" == $'release=hotfix1\ntesting=something' ]] && echo pass || echo fail
    #
    # Removing Userdata
    rmdir userdata/testing
    preview=$(for f in `ls userdata`; do echo $f=$(cat userdata/$f/value); done)
    [[ "$preview" == $'release=hotfix1' ]] && echo pass || echo fail
    rmdir userdata/release
    [[ `cat userdata/complete` == $'release=hotfix1' ]] && echo pass || echo fail
    #
    # Adding userdata key with too large of 6.7.0-rc8-virtme,12,481,17954104,-directory name [<54 chars]
    mkdir userdata/testing12345678901234567890123456789012345678901234567890
    [[ $? == 1 ]] && echo pass || echo fail
    #
    # Adding userdata value with too large of value [<200 chars]
    mkdir userdata/testing
    echo `for i in {1..201};do printf "%s" "v";done` > userdata/testing/value
    [[ $? == 1 ]] && echo pass || echo fail
    rmdir userdata/testing

- Output:

    pass
    pass
    pass
    pass
    pass
    mkdir: cannot create directory ‘cmdline0/userdata/testing12345678901234567890123456789012345678901234567890’: File name too long
    pass
    bash: echo: write error: Message too long
    pass

Test netconsole messages (w/ msg fragmentation)

    echo `for i in {1..996};do printf "%s" "v";done` > /dev/kmsg

- Output:

    6.7.0-rc8-virtme,12,484,84321212,-,ncfrag=0/997;vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv6.7.0-rc8-virtme,12,484,84321212,-,ncfrag=952/997;vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv

Test empty userdatum

    cd /sys/kernel/config/netconsole/ && mkdir cmdline0
    mkdir cmdline0/userdata/empty
    echo test > /dev/kmsg
    rmdir cmdline0/userdata/empty

- Output:

Test netconsole messages (w/o userdata fragmentation)

    cd /sys/kernel/config/netconsole/ && mkdir cmdline0
    mkdir cmdline0/userdata/release && echo hotfix1 > cmdline0/userdata/release/value
    mkdir cmdline0/userdata/testing && echo something > cmdline0/userdata/testing/value
    echo test > /dev/kmsg
    rmdir cmdline0/userdata/release
    rmdir cmdline0/userdata/testing

- Output:
    6.7.0-rc8-virtme,12,500,1646292204,-;test
    release=hotfix1
    testing=something

Test netconsole messages (w/ long body fragmentation)
    cd /sys/kernel/config/netconsole/ && mkdir cmdline0
    mkdir cmdline0/userdata/release && echo hotfix1 > cmdline0/userdata/release/value
    mkdir cmdline0/userdata/testing && echo something > cmdline0/userdata/testing/value
    echo `for i in {1..996};do printf "%s" "v";done` > /dev/kmsg
    rmdir cmdline0/userdata/release
    rmdir cmdline0/userdata/testing

- Output:
    6.7.0-rc8-virtme,12,486,156664417,-,ncfrag=0/1031;vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv6.7.0-rc8-virtme,12,486,156664417,-,ncfrag=950/1031;vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
    release=hotfix1
    testing=something

Test netconsole messages (w/ long userdata fragmentation)

    cd /sys/kernel/config/netconsole/ && mkdir cmdline0
    LOREM="Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut"
    for i in {0..5}; do mkdir cmdline0/userdata/value${i} && echo ${LOREM} > cmdline0/userdata/value${i}/value; done
    echo test > /dev/kmsg
    for i in {0..5}; do rmdir cmdline0/userdata/value${i}; done

- Output:
    6.7.0-rc8-virtme,12,487,221763007,-,ncfrag=0/1241;test
    value0=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    value1=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    value2=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    value3=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    value4=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magn6.7.0-rc8-virtme,12,487,221763007,-,ncfrag=950/1241;a aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    value5=Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut


Matthew Wood (8):
  net: netconsole: cleanup formatting lints
  net: netconsole: move netconsole_target config_item to config_group
  net: netconsole: move newline trimming to function
  net: netconsole: add docs for appending netconsole user data
  net: netconsole: add a userdata config_group member to
    netconsole_target
  net: netconsole: cache userdata formatted string in netconsole_target
  net: netconsole: append userdata to netconsole messages
  net: netconsole: append userdata to fragmented netconsole messages

 Documentation/networking/netconsole.rst |  68 +++++
 drivers/net/netconsole.c                | 360 ++++++++++++++++++++----
 2 files changed, 373 insertions(+), 55 deletions(-)

--
2.43.0

[PATCH net-next v2 1/8] net: netconsole: cleanup formatting lints

[Matthew Wood]

Address checkpatch lint suggestions in preparation for later changes

Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 6e14ba5e06c8..93fc3b509706 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -49,7 +49,7 @@ static char config[MAX_PARAM_LENGTH];
 module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]");
 
-static bool oops_only = false;
+static bool oops_only;
 module_param(oops_only, bool, 0600);
 MODULE_PARM_DESC(oops_only, "Only log oops messages");
 
@@ -501,6 +501,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 
 	if (strnchr(buf, count, ':')) {
 		const char *end;
+
 		if (in6_pton(buf, count, nt->np.local_ip.in6.s6_addr, -1, &end) > 0) {
 			if (*end && *end != '\n') {
 				pr_err("invalid IPv6 address at: <%c>\n", *end);
@@ -510,9 +511,9 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 		} else
 			goto out_unlock;
 	} else {
-		if (!nt->np.ipv6) {
+		if (!nt->np.ipv6)
 			nt->np.local_ip.ip = in_aton(buf);
-		} else
+		else
 			goto out_unlock;
 	}
 
@@ -537,6 +538,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 
 	if (strnchr(buf, count, ':')) {
 		const char *end;
+
 		if (in6_pton(buf, count, nt->np.remote_ip.in6.s6_addr, -1, &end) > 0) {
 			if (*end && *end != '\n') {
 				pr_err("invalid IPv6 address at: <%c>\n", *end);
@@ -546,9 +548,9 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 		} else
 			goto out_unlock;
 	} else {
-		if (!nt->np.ipv6) {
+		if (!nt->np.ipv6)
 			nt->np.remote_ip.ip = in_aton(buf);
-		} else
+		else
 			goto out_unlock;
 	}
 
@@ -781,6 +783,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	spin_unlock_irqrestore(&target_list_lock, flags);
 	if (stopped) {
 		const char *msg = "had an event";
+
 		switch (event) {
 		case NETDEV_UNREGISTER:
 			msg = "unregistered";
-- 
2.43.0

[PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group

[Matthew Wood]

In order to support a nested userdata config_group in later patches,
use a config_group for netconsole_target instead of a
config_item. It's a no-op functionality-wise, since
config_group maintains all features of a config_item via the cg_item
member.

Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 61 ++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 93fc3b509706..085350beca87 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -79,7 +79,7 @@ static struct console netconsole_ext;
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
- * @item:	Links us into the configfs subsystem hierarchy.
+ * @group:	Links us into the configfs subsystem hierarchy.
  * @enabled:	On / off knob to enable / disable target.
  *		Visible from userspace (read-write).
  *		We maintain a strict 1:1 correspondence between this and
@@ -102,7 +102,7 @@ static struct console netconsole_ext;
 struct netconsole_target {
 	struct list_head	list;
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
-	struct config_item	item;
+	struct config_group	group;
 #endif
 	bool			enabled;
 	bool			extended;
@@ -134,14 +134,14 @@ static void __exit dynamic_netconsole_exit(void)
  */
 static void netconsole_target_get(struct netconsole_target *nt)
 {
-	if (config_item_name(&nt->item))
-		config_item_get(&nt->item);
+	if (config_item_name(&nt->group.cg_item))
+		config_group_get(&nt->group);
 }
 
 static void netconsole_target_put(struct netconsole_target *nt)
 {
-	if (config_item_name(&nt->item))
-		config_item_put(&nt->item);
+	if (config_item_name(&nt->group.cg_item))
+		config_group_put(&nt->group);
 }
 
 #else	/* !CONFIG_NETCONSOLE_DYNAMIC */
@@ -221,9 +221,13 @@ static struct netconsole_target *alloc_and_init(void)
 
 static struct netconsole_target *to_target(struct config_item *item)
 {
-	return item ?
-		container_of(item, struct netconsole_target, item) :
-		NULL;
+	struct config_group *cfg_group;
+
+	cfg_group = to_config_group(item);
+	if (!cfg_group)
+		return NULL;
+	return container_of(to_config_group(item),
+			    struct netconsole_target, group);
 }
 
 /*
@@ -370,7 +374,7 @@ static ssize_t release_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		err = -EINVAL;
 		goto out_unlock;
 	}
@@ -398,7 +402,7 @@ static ssize_t extended_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		err = -EINVAL;
 		goto out_unlock;
 	}
@@ -425,7 +429,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		mutex_unlock(&dynamic_netconsole_mutex);
 		return -EINVAL;
 	}
@@ -450,7 +454,7 @@ static ssize_t local_port_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		goto out_unlock;
 	}
 
@@ -473,7 +477,7 @@ static ssize_t remote_port_store(struct config_item *item,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		goto out_unlock;
 	}
 
@@ -495,7 +499,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		goto out_unlock;
 	}
 
@@ -532,7 +536,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		goto out_unlock;
 	}
 
@@ -570,7 +574,7 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",
-		       config_item_name(&nt->item));
+		       config_item_name(&nt->group.cg_item));
 		goto out_unlock;
 	}
 
@@ -638,7 +642,7 @@ static struct netconsole_target *find_cmdline_target(const char *name)
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
-		if (!strcmp(nt->item.ci_name, name)) {
+		if (!strcmp(nt->group.cg_item.ci_name, name)) {
 			ret = nt;
 			break;
 		}
@@ -652,8 +656,8 @@ static struct netconsole_target *find_cmdline_target(const char *name)
  * Group operations and type for netconsole_subsys.
  */
 
-static struct config_item *make_netconsole_target(struct config_group *group,
-						  const char *name)
+static struct config_group *make_netconsole_target(struct config_group *group,
+						   const char *name)
 {
 	struct netconsole_target *nt;
 	unsigned long flags;
@@ -665,8 +669,9 @@ static struct config_item *make_netconsole_target(struct config_group *group,
 	if (!strncmp(name, NETCONSOLE_PARAM_TARGET_PREFIX,
 		     strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
 		nt = find_cmdline_target(name);
-		if (nt)
-			return &nt->item;
+		if (nt) {
+			return &nt->group;
+		}
 	}
 
 	nt = alloc_and_init();
@@ -674,14 +679,14 @@ static struct config_item *make_netconsole_target(struct config_group *group,
 		return ERR_PTR(-ENOMEM);
 
 	/* Initialize the config_item member */
-	config_item_init_type_name(&nt->item, name, &netconsole_target_type);
+	config_group_init_type_name(&nt->group, name, &netconsole_target_type);
 
 	/* Adding, but it is disabled */
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_add(&nt->list, &target_list);
 	spin_unlock_irqrestore(&target_list_lock, flags);
 
-	return &nt->item;
+	return &nt->group;
 }
 
 static void drop_netconsole_target(struct config_group *group,
@@ -701,11 +706,11 @@ static void drop_netconsole_target(struct config_group *group,
 	if (nt->enabled)
 		netpoll_cleanup(&nt->np);
 
-	config_item_put(&nt->item);
+	config_item_put(&nt->group.cg_item);
 }
 
 static struct configfs_group_operations netconsole_subsys_group_ops = {
-	.make_item	= make_netconsole_target,
+	.make_group	= make_netconsole_target,
 	.drop_item	= drop_netconsole_target,
 };
 
@@ -731,8 +736,8 @@ static void populate_configfs_item(struct netconsole_target *nt,
 
 	snprintf(target_name, sizeof(target_name), "%s%d",
 		 NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);
-	config_item_init_type_name(&nt->item, target_name,
-				   &netconsole_target_type);
+	config_group_init_type_name(&nt->group, target_name,
+				    &netconsole_target_type);
 }
 
 #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
-- 
2.43.0

[PATCH net-next v2 3/8] net: netconsole: move newline trimming to function

[Matthew Wood]

Move newline trimming logic from `dev_name_store()` to a new function
(trim_newline()) for shared use in netconsole.c

Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 085350beca87..b280d06bf152 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
 			    struct netconsole_target, group);
 }

+/* Get rid of possible trailing newline, returning the new length */
+static void trim_newline(char *s, size_t maxlen)
+{
+	size_t len;
+
+	len = strnlen(s, maxlen);
+	if (s[len - 1] == '\n')
+		s[len - 1] = '\0';
+}
+
 /*
  * Attribute operations for netconsole_target.
  */
@@ -424,7 +434,6 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 		size_t count)
 {
 	struct netconsole_target *nt = to_target(item);
-	size_t len;

 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
@@ -435,11 +444,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 	}

 	strscpy(nt->np.dev_name, buf, IFNAMSIZ);
-
-	/* Get rid of possible trailing newline from echo(1) */
-	len = strnlen(nt->np.dev_name, IFNAMSIZ);
-	if (nt->np.dev_name[len - 1] == '\n')
-		nt->np.dev_name[len - 1] = '\0';
+	trim_newline(nt->np.dev_name, IFNAMSIZ);

 	mutex_unlock(&dynamic_netconsole_mutex);
 	return strnlen(buf, count);
--
2.43.0

[PATCH net-next v2 4/8] net: netconsole: add docs for appending netconsole user data

[Matthew Wood]

Add a new User Data section to the netconsole docs to describe the
appending of user data capability (for netconsole dynamic configuration)
with usage and netconsole output examples.

Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 Documentation/networking/netconsole.rst | 68 +++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index 390730a74332..54f072f47921 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -15,6 +15,8 @@ Extended console support by Tejun Heo <tj@kernel.org>, May 1 2015
 
 Release prepend support by Breno Leitao <leitao@debian.org>, Jul 7 2023
 
+Userdata append support by Matthew Wood <thepacketgeek@gmail.com>, Jan 22 2024
+
 Please send bug reports to Matt Mackall <mpm@selenic.com>
 Satyam Sharma <satyam.sharma@gmail.com>, and Cong Wang <xiyou.wangcong@gmail.com>
 
@@ -171,6 +173,72 @@ You can modify these targets in runtime by creating the following targets::
  cat cmdline1/remote_ip
  10.0.0.3
 
+Append User Data
+================
+
+Custom user data can be appended to the end of messages with netconsole
+dynamic configuration enabled. User data entries can be modified without
+changing the "enabled" attribute of a target.
+
+Directories (keys) under `userdata` are limited to 54 character length, and
+data in `userdata/<key>/value` are limited to 200 bytes::
+
+ cd /sys/kernel/config/netconsole && mkdir cmdline0
+ cd cmdline0
+ mkdir userdata/foo
+ echo bar > userdata/foo/value
+ mkdir userdata/qux
+ echo baz > userdata/qux/value
+
+Messages will now include this additional user data::
+
+ echo "This is a message" > /dev/kmsg
+
+Sends::
+
+ 12,607,22085407756,-;This is a message
+ foo=bar
+ qux=baz
+
+Preview the userdata that will be appended with::
+
+ cd /sys/kernel/config/netconsole/cmdline0/userdata
+ for f in `ls userdata`; do echo $f=$(cat userdata/$f/value); done
+
+If a `userdata` entry is created but no data is written to the `value` file,
+the entry will be omitted from netconsole messages::
+
+ cd /sys/kernel/config/netconsole && mkdir cmdline0
+ cd cmdline0
+ mkdir userdata/foo
+ echo bar > userdata/foo/value
+ mkdir userdata/qux
+
+The `qux` key is omitted since it has no value::
+
+ echo "This is a message" > /dev/kmsg
+ 12,607,22085407756,-;This is a message
+ foo=bar
+
+Delete `userdata` entries with `rmdir`::
+
+ rmdir /sys/kernel/config/netconsole/cmdline0/userdata/qux
+
+Beware of `userdata` entries with newlines since values are printed with
+newlines preserved. A userdatum value with a newline could cause the
+netconsole message receiver to interpret a value as a new userdata key::
+
+ cat userdata/foo/value
+ bar
+ bar2
+
+The `qux` key is omitted since it has no value::
+
+ echo "This is a message" > /dev/kmsg
+ 12,607,22085407756,-;This is a message
+ foo=bar
+ bar2
+
 Extended console:
 =================
 
-- 
2.43.0

[PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target

[Matthew Wood]

Create configfs machinery for netconsole userdata appending, which depends
on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
config_group to netconsole_target for managing userdata entries as a tree
under the netconsole configfs subsystem. Directory names created under the
userdata directory become userdatum keys; the userdatum value is the
content of the value file.

Include the minimum-viable-changes for userdata configfs config_group.
init_target_config_group() ties in the complete configfs machinery to
avoid unused func/variable errors during build. Initializing the
netconsole_target->group is moved to init_target_config_group, which
will also init and add the userdata config_group.

Each userdatum entry has a limit of 256 bytes (54 for
the key/directory, 200 for the value, and 2 for '=' and '\n'
characters), which is enforced by the configfs functions for updating
the userdata config_group.

When a new netconsole_target is created, initialize the userdata
config_group and add it as a default group for netconsole_target
config_group, allowing the userdata configfs sub-tree to be presented
in the netconsole configfs tree under the userdata directory.

Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 139 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index b280d06bf152..a5ac21136f02 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -43,6 +43,10 @@ MODULE_DESCRIPTION("Console driver for network interfaces");
 MODULE_LICENSE("GPL");
 
 #define MAX_PARAM_LENGTH	256
+#define MAX_USERDATA_NAME_LENGTH	54
+#define MAX_USERDATA_VALUE_LENGTH	200
+#define MAX_USERDATA_ENTRY_LENGTH	256
+#define MAX_USERDATA_ITEMS		16
 #define MAX_PRINT_CHUNK		1000
 
 static char config[MAX_PARAM_LENGTH];
@@ -80,6 +84,7 @@ static struct console netconsole_ext;
  * 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
  * @enabled:	On / off knob to enable / disable target.
  *		Visible from userspace (read-write).
  *		We maintain a strict 1:1 correspondence between this and
@@ -103,6 +108,7 @@ struct netconsole_target {
 	struct list_head	list;
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	struct config_group	group;
+	struct config_group	userdata_group;
 #endif
 	bool			enabled;
 	bool			extended;
@@ -215,6 +221,10 @@ static struct netconsole_target *alloc_and_init(void)
  *				|	remote_ip
  *				|	local_mac
  *				|	remote_mac
+ *				|	userdata/
+ *				|		<key>/
+ *				|			value
+ *				|		...
  *				|
  *				<target>/...
  */
@@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
 	return -EINVAL;
 }
 
+struct userdatum {
+	struct config_item item;
+	char value[MAX_USERDATA_VALUE_LENGTH];
+};
+
+static inline struct userdatum *to_userdatum(struct config_item *item)
+{
+	return container_of(item, struct userdatum, item);
+}
+
+struct userdata {
+	struct config_group group;
+};
+
+static inline struct userdata *to_userdata(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct userdata, group);
+}
+
+static struct netconsole_target *userdata_to_target(struct userdata *ud)
+{
+	struct config_group *netconsole_group;
+
+	netconsole_group = to_config_group(ud->group.cg_item.ci_parent);
+	return to_target(&netconsole_group->cg_item);
+}
+
+static ssize_t userdatum_value_show(struct config_item *item, char *buf)
+{
+	return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
+}
+
+static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
+				     size_t count)
+{
+	struct userdatum *udm = to_userdatum(item);
+	int ret;
+
+	if (count > MAX_USERDATA_VALUE_LENGTH)
+		return -EMSGSIZE;
+
+	mutex_lock(&dynamic_netconsole_mutex);
+
+	ret = strscpy(udm->value, buf, sizeof(udm->value));
+	if (ret < 0)
+		goto out_unlock;
+	trim_newline(udm->value, sizeof(udm->value));
+
+	mutex_unlock(&dynamic_netconsole_mutex);
+	return count;
+out_unlock:
+	mutex_unlock(&dynamic_netconsole_mutex);
+	return ret;
+}
+
+CONFIGFS_ATTR(userdatum_, value);
+
+static struct configfs_attribute *userdatum_attrs[] = {
+	&userdatum_attr_value,
+	NULL,
+};
+
+static void userdatum_release(struct config_item *item)
+{
+	kfree(to_userdatum(item));
+}
+
+static struct configfs_item_operations userdatum_ops = {
+	.release = userdatum_release,
+};
+
+static const struct config_item_type userdatum_type = {
+	.ct_item_ops	= &userdatum_ops,
+	.ct_attrs	= userdatum_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *userdatum_make_item(struct config_group *group,
+					       const char *name)
+{
+	struct netconsole_target *nt;
+	struct userdatum *udm;
+	struct userdata *ud;
+	size_t child_count;
+
+	if (strlen(name) > MAX_USERDATA_NAME_LENGTH)
+		return ERR_PTR(-ENAMETOOLONG);
+
+	ud = to_userdata(&group->cg_item);
+	nt = userdata_to_target(ud);
+	child_count = list_count_nodes(&nt->userdata_group.cg_children);
+	if (child_count >= MAX_USERDATA_ITEMS)
+		return ERR_PTR(-ENOSPC);
+
+	udm = kzalloc(sizeof(*udm), GFP_KERNEL);
+	if (!udm)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&udm->item, name, &userdatum_type);
+	return &udm->item;
+}
+
+static struct configfs_attribute *userdata_attrs[] = {
+	NULL,
+};
+
+static struct configfs_group_operations userdata_ops = {
+	.make_item		= userdatum_make_item,
+};
+
+static struct config_item_type userdata_type = {
+	.ct_item_ops	= &userdatum_ops,
+	.ct_group_ops	= &userdata_ops,
+	.ct_attrs	= userdata_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
 CONFIGFS_ATTR(, enabled);
 CONFIGFS_ATTR(, extended);
 CONFIGFS_ATTR(, dev_name);
@@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
 	.ct_owner		= THIS_MODULE,
 };
 
+static void init_target_config_group(struct netconsole_target *nt, const char *name)
+{
+	config_group_init_type_name(&nt->group, name, &netconsole_target_type);
+	config_group_init_type_name(&nt->userdata_group, "userdata",
+				    &userdata_type);
+	configfs_add_default_group(&nt->userdata_group, &nt->group);
+}
+
 static struct netconsole_target *find_cmdline_target(const char *name)
 {
 	struct netconsole_target *nt, *ret = NULL;
@@ -675,6 +810,7 @@ static struct config_group *make_netconsole_target(struct config_group *group,
 		     strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
 		nt = find_cmdline_target(name);
 		if (nt) {
+			init_target_config_group(nt, name);
 			return &nt->group;
 		}
 	}
@@ -683,8 +819,8 @@ static struct config_group *make_netconsole_target(struct config_group *group,
 	if (!nt)
 		return ERR_PTR(-ENOMEM);
 
-	/* Initialize the config_item member */
-	config_group_init_type_name(&nt->group, name, &netconsole_target_type);
+	/* Initialize the config_group member */
+	init_target_config_group(nt, name);
 
 	/* Adding, but it is disabled */
 	spin_lock_irqsave(&target_list_lock, flags);
@@ -741,8 +877,7 @@ static void populate_configfs_item(struct netconsole_target *nt,
 
 	snprintf(target_name, sizeof(target_name), "%s%d",
 		 NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);
-	config_group_init_type_name(&nt->group, target_name,
-				    &netconsole_target_type);
+	init_target_config_group(nt, target_name);
 }
 
 #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
-- 
2.43.0

[PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target

[Matthew Wood]

Store a formatted string for userdata that will be appended to netconsole
messages. The string has a capacity of 4KB, as calculated by the userdatum
entry length of 256 bytes and a max of 16 userdata entries.

Update the stored netconsole_target->userdata_complete string with the new
formatted userdata values when a userdatum is created, edited, or
removed. Each userdata entry contains a trailing newline, which will be
formatted as such in netconsole messages::

    6.7.0-rc8-virtme,12,500,1646292204,-;test
    release=foo
    something=bar
    6.7.0-rc8-virtme,12,500,1646292204,-;another test
    release=foo
    something=bar

Enforcement of MAX_USERDATA_ITEMS is done in userdatum_make_item;
update_userdata will not check for this case but will skip any userdata
children over the limit of MAX_USERDATA_ITEMs.

If a userdata entry/dir is created but no value is provided, that entry
will be skipped. This is in part because update_userdata() can't be
called in userdatum_make_item() since the item will not have been added
to the userdata config_group children yet. To preserve the experience of
adding an empty userdata that doesn't show up in the netconsole
messages, purposefully skip emtpy userdata items even when
update_userdata() can be called.

Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 63 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a5ac21136f02..73feba0b3c93 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -85,6 +85,8 @@ static struct console netconsole_ext;
  * @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_complete:	Cached, formatted string of append
+ * @userdata_length:	String length of userdata_complete
  * @enabled:	On / off knob to enable / disable target.
  *		Visible from userspace (read-write).
  *		We maintain a strict 1:1 correspondence between this and
@@ -109,6 +111,8 @@ struct netconsole_target {
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	struct config_group	group;
 	struct config_group	userdata_group;
+	char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
+	size_t			userdata_length;
 #endif
 	bool			enabled;
 	bool			extended;
@@ -638,10 +642,50 @@ 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)
+{
+	int complete_idx = 0, child_count = 0;
+	struct list_head *entry;
+	struct userdata *ud;
+
+	/* Clear the current string in case the last userdatum was deleted */
+	nt->userdata_length = 0;
+	nt->userdata_complete[0] = 0;
+
+	ud = to_userdata(&nt->userdata_group.cg_item);
+	list_for_each(entry, &nt->userdata_group.cg_children) {
+		struct userdatum *udm_item;
+		struct config_item *item;
+
+		if (child_count >= MAX_USERDATA_ITEMS)
+			break;
+		child_count++;
+
+		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_USERDATA_VALUE_LENGTH) == 0)
+			continue;
+
+		/* This doesn't overflow userdata_complete 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
+		 */
+		complete_idx += scnprintf(&nt->userdata_complete[complete_idx],
+					  MAX_USERDATA_ENTRY_LENGTH, "%s=%s\n",
+					  item->ci_name, udm_item->value);
+	}
+	nt->userdata_length = strnlen(nt->userdata_complete,
+				      sizeof(nt->userdata_complete));
+}
+
 static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 				     size_t count)
 {
 	struct userdatum *udm = to_userdatum(item);
+	struct netconsole_target *nt;
+	struct userdata *ud;
 	int ret;
 
 	if (count > MAX_USERDATA_VALUE_LENGTH)
@@ -654,6 +698,10 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	trim_newline(udm->value, sizeof(udm->value));
 
+	ud = to_userdata(item->ci_parent);
+	nt = userdata_to_target(ud);
+	update_userdata(nt);
+
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return count;
 out_unlock:
@@ -708,12 +756,27 @@ static struct config_item *userdatum_make_item(struct config_group *group,
 	return &udm->item;
 }
 
+static void userdatum_drop(struct config_group *group, struct config_item *item)
+{
+	struct netconsole_target *nt;
+	struct userdata *ud;
+
+	ud = to_userdata(&group->cg_item);
+	nt = userdata_to_target(ud);
+
+	mutex_lock(&dynamic_netconsole_mutex);
+	update_userdata(nt);
+	config_item_put(item);
+	mutex_unlock(&dynamic_netconsole_mutex);
+}
+
 static struct configfs_attribute *userdata_attrs[] = {
 	NULL,
 };
 
 static struct configfs_group_operations userdata_ops = {
 	.make_item		= userdatum_make_item,
+	.drop_item		= userdatum_drop,
 };
 
 static struct config_item_type userdata_type = {
-- 
2.43.0

[PATCH net-next v2 7/8] net: netconsole: append userdata to netconsole messages

[Matthew Wood]

Append userdata to outgoing unfragmented (<1000 bytes) netconsole messages.
When sending messages the userdata string is already formatted and stored
in netconsole_target->userdata_complete.

Always write the outgoing message to buf, so userdata can be appended in
a standard fashion. This is a change from only using buf when the
release needs to be prepended to the message.

Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 73feba0b3c93..de668a0794b1 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1035,19 +1035,34 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 	const char *msg_ready = msg;
 	const char *release;
 	int release_len = 0;
+	int userdata_len = 0;
+	char *userdata = NULL;
+
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+	userdata = nt->userdata_complete;
+	userdata_len = nt->userdata_length;
+#endif
 
 	if (nt->release) {
 		release = init_utsname()->release;
 		release_len = strlen(release) + 1;
 	}
 
-	if (msg_len + release_len <= MAX_PRINT_CHUNK) {
+	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK) {
 		/* No fragmentation needed */
 		if (nt->release) {
 			scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
 			msg_len += release_len;
-			msg_ready = buf;
+		} else {
+			memcpy(buf, msg, msg_len);
 		}
+
+		if (userdata)
+			msg_len += scnprintf(&buf[msg_len],
+					     MAX_PRINT_CHUNK - msg_len,
+					     "%s", userdata);
+
+		msg_ready = buf;
 		netpoll_send_udp(&nt->np, msg_ready, msg_len);
 		return;
 	}
-- 
2.43.0

[PATCH net-next v2 8/8] net: netconsole: append userdata to fragmented netconsole messages

[Matthew Wood]

Regardless of whether the original message body or formatted userdata
exceeds the MAX_PRINT_CHUNK, append userdata to the netconsole message
starting with the first chunk that has available space after writing the
body.

Co-developed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 50 +++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index de668a0794b1..b33ceb110434 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1086,24 +1086,48 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 	memcpy(buf + release_len, header, header_len);
 	header_len += release_len;
 
-	while (offset < body_len) {
+	while (offset < body_len + userdata_len) {
 		int this_header = header_len;
-		int this_chunk;
+		int this_offset = 0;
+		int this_chunk = 0;
 
 		this_header += scnprintf(buf + this_header,
 					 sizeof(buf) - this_header,
-					 ",ncfrag=%d/%d;", offset, body_len);
-
-		this_chunk = min(body_len - offset,
-				 MAX_PRINT_CHUNK - this_header);
-		if (WARN_ON_ONCE(this_chunk <= 0))
-			return;
-
-		memcpy(buf + this_header, body + offset, this_chunk);
-
-		netpoll_send_udp(&nt->np, buf, this_header + this_chunk);
+					 ",ncfrag=%d/%d;", offset,
+					 body_len + userdata_len);
+
+		/* Not all body data has been written yet */
+		if (offset < body_len) {
+			this_chunk = min(body_len - offset,
+					 MAX_PRINT_CHUNK - this_header);
+			if (WARN_ON_ONCE(this_chunk <= 0))
+				return;
+			memcpy(buf + this_header, body + offset, this_chunk);
+			this_offset += this_chunk;
+		}
+		/* Body is fully written and there is pending userdata to write,
+		 * append userdata in this chunk
+		 */
+		if (offset + this_offset >= body_len &&
+		    offset + this_offset < userdata_len + body_len) {
+			int sent_userdata = (offset + this_offset) - body_len;
+			int preceding_bytes = this_chunk + this_header;
+
+			if (WARN_ON_ONCE(sent_userdata < 0))
+				return;
+
+			this_chunk = min(userdata_len - sent_userdata,
+					 MAX_PRINT_CHUNK - preceding_bytes);
+			if (WARN_ON_ONCE(this_chunk <= 0))
+				return;
+			memcpy(buf + this_header + this_offset,
+			       userdata + sent_userdata,
+			       this_chunk);
+			this_offset += this_chunk;
+		}
 
-		offset += this_chunk;
+		netpoll_send_udp(&nt->np, buf, this_header + this_offset);
+		offset += this_offset;
 	}
 }
 
-- 
2.43.0

Re: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target

[kernel test robot]

Hi Matthew,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthew-Wood/net-netconsole-cleanup-formatting-lints/20240127-072017
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240126231348.281600-7-thepacketgeek%40gmail.com
patch subject: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240127/202401272022.r5Z4OtUg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240127/202401272022.r5Z4OtUg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401272022.r5Z4OtUg-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/netconsole.c: In function 'update_userdata':
>> drivers/net/netconsole.c:649:26: warning: variable 'ud' set but not used [-Wunused-but-set-variable]
     649 |         struct userdata *ud;
         |                          ^~


vim +/ud +649 drivers/net/netconsole.c

   644	
   645	static void update_userdata(struct netconsole_target *nt)
   646	{
   647		int complete_idx = 0, child_count = 0;
   648		struct list_head *entry;
 > 649		struct userdata *ud;
   650	
   651		/* Clear the current string in case the last userdatum was deleted */
   652		nt->userdata_length = 0;
   653		nt->userdata_complete[0] = 0;
   654	
   655		ud = to_userdata(&nt->userdata_group.cg_item);
   656		list_for_each(entry, &nt->userdata_group.cg_children) {
   657			struct userdatum *udm_item;
   658			struct config_item *item;
   659	
   660			if (child_count >= MAX_USERDATA_ITEMS)
   661				break;
   662			child_count++;
   663	
   664			item = container_of(entry, struct config_item, ci_entry);
   665			udm_item = to_userdatum(item);
   666	
   667			/* Skip userdata with no value set */
   668			if (strnlen(udm_item->value, MAX_USERDATA_VALUE_LENGTH) == 0)
   669				continue;
   670	
   671			/* This doesn't overflow userdata_complete since it will write
   672			 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
   673			 * checked to not exceed MAX items with child_count above
   674			 */
   675			complete_idx += scnprintf(&nt->userdata_complete[complete_idx],
   676						  MAX_USERDATA_ENTRY_LENGTH, "%s=%s\n",
   677						  item->ci_name, udm_item->value);
   678		}
   679		nt->userdata_length = strnlen(nt->userdata_complete,
   680					      sizeof(nt->userdata_complete));
   681	}
   682	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function

[Breno Leitao]

On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> Move newline trimming logic from `dev_name_store()` to a new function
> (trim_newline()) for shared use in netconsole.c
> 
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
>  drivers/net/netconsole.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 085350beca87..b280d06bf152 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
>  			    struct netconsole_target, group);
>  }
> 
> +/* Get rid of possible trailing newline, returning the new length */
> +static void trim_newline(char *s, size_t maxlen)
> +{
> +	size_t len;
> +
> +	len = strnlen(s, maxlen);
> +	if (s[len - 1] == '\n')
> +		s[len - 1] = '\0';
> +}

I am thinking about this one. Should we replace the first `\n` in the
file by `\0` no matter where it is? This will probably make it easier to
implement the netconsd, where we know it will be impossible to have `\n`
in the userdata.

Maybe something as:

	static inline void trim_newline(char *str)
	{
		char *pos = strchr(str, '\n');

		if (pos)
			*pos = '\0';
	}


All in all, this is a good clean up, which make the code easier to read.
Thanks!

Re: [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group

[Breno Leitao]

On Fri, Jan 26, 2024 at 03:13:37PM -0800, Matthew Wood wrote:
> In order to support a nested userdata config_group in later patches,
> use a config_group for netconsole_target instead of a
> config_item. It's a no-op functionality-wise, since
> config_group maintains all features of a config_item via the cg_item
> member.
> 
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net-next v2 1/8] net: netconsole: cleanup formatting lints

[Breno Leitao]

On Fri, Jan 26, 2024 at 03:13:36PM -0800, Matthew Wood wrote:
> Address checkpatch lint suggestions in preparation for later changes
> 
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function

[Packet Geek]

On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
>
> On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > Move newline trimming logic from `dev_name_store()` to a new function
> > (trim_newline()) for shared use in netconsole.c
> >
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > ---
> >  drivers/net/netconsole.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 085350beca87..b280d06bf152 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> >                           struct netconsole_target, group);
> >  }
> >
> > +/* Get rid of possible trailing newline, returning the new length */
> > +static void trim_newline(char *s, size_t maxlen)
> > +{
> > +     size_t len;
> > +
> > +     len = strnlen(s, maxlen);
> > +     if (s[len - 1] == '\n')
> > +             s[len - 1] = '\0';
> > +}
>
> I am thinking about this one. Should we replace the first `\n` in the
> file by `\0` no matter where it is? This will probably make it easier to
> implement the netconsd, where we know it will be impossible to have `\n`
> in the userdata.
>
> Maybe something as:
>
>         static inline void trim_newline(char *str)
>         {
>                 char *pos = strchr(str, '\n');
>
>                 if (pos)
>                         *pos = '\0';
>         }
>
>
> All in all, this is a good clean up, which make the code easier to read.
> Thanks!

I like this idea, I agree that only accepting userdata values upto the
first newline clears up the expectations for log output and parsing on
the receiving side. I would prefer that to the case where multiple
values (delimited by newlines) are somehow attempted with a single
key, seems like just using additional key/value pairs would be
cleaner.

Re: [PATCH net-next v2 3/8] net: netconsole: move newline trimming to function

[Matthew Wood]

On Wed, Jan 31, 2024 at 8:45 PM Matthew Wood <thepacketgeek@gmail.com> wrote:
>
> On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
> >
> > On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > > Move newline trimming logic from `dev_name_store()` to a new function
> > > (trim_newline()) for shared use in netconsole.c
> > >
> > > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > > ---
> > >  drivers/net/netconsole.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index 085350beca87..b280d06bf152 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> > >                           struct netconsole_target, group);
> > >  }
> > >
> > > +/* Get rid of possible trailing newline, returning the new length */
> > > +static void trim_newline(char *s, size_t maxlen)
> > > +{
> > > +     size_t len;
> > > +
> > > +     len = strnlen(s, maxlen);
> > > +     if (s[len - 1] == '\n')
> > > +             s[len - 1] = '\0';
> > > +}
> >
> > I am thinking about this one. Should we replace the first `\n` in the
> > file by `\0` no matter where it is? This will probably make it easier to
> > implement the netconsd, where we know it will be impossible to have `\n`
> > in the userdata.
> >
> > Maybe something as:
> >
> >         static inline void trim_newline(char *str)
> >         {
> >                 char *pos = strchr(str, '\n');
> >
> >                 if (pos)
> >                         *pos = '\0';
> >         }
> >
> >
> > All in all, this is a good clean up, which make the code easier to read.
> > Thanks!
>
> I like this idea, I agree that only accepting userdata values upto the
> first newline clears up the expectations for log output and parsing on
> the receiving side. I would prefer that to the case where multiple
> values (delimited by newlines) are somehow attempted with a single
> key, seems like just using additional key/value pairs would be
> cleaner.

In practice truncating at the first newline makes no difference as
printk, echo, and other methods seem to buffer and write per-line. So
in this example, the stored value will be "val2" with or without the
suggested change:

$ printf "val1\nval2" > userdata/testing/value
# This results in two calls to userdatum_value_store, the first with
"val1\n" and the second with "val2". "val2" remains as the latest
write.
$ cat userdata/testing/value
val2

I will add a warning about this possibly unexpected behavior in the
docs for v3 for the patch

Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target

[Simon Horman]

On Fri, Jan 26, 2024 at 03:13:40PM -0800, Matthew Wood wrote:
> Create configfs machinery for netconsole userdata appending, which depends
> on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
> config_group to netconsole_target for managing userdata entries as a tree
> under the netconsole configfs subsystem. Directory names created under the
> userdata directory become userdatum keys; the userdatum value is the
> content of the value file.
> 
> Include the minimum-viable-changes for userdata configfs config_group.
> init_target_config_group() ties in the complete configfs machinery to
> avoid unused func/variable errors during build. Initializing the
> netconsole_target->group is moved to init_target_config_group, which
> will also init and add the userdata config_group.
> 
> Each userdatum entry has a limit of 256 bytes (54 for
> the key/directory, 200 for the value, and 2 for '=' and '\n'
> characters), which is enforced by the configfs functions for updating
> the userdata config_group.
> 
> When a new netconsole_target is created, initialize the userdata
> config_group and add it as a default group for netconsole_target
> config_group, allowing the userdata configfs sub-tree to be presented
> in the netconsole configfs tree under the userdata directory.
> 
> Co-developed-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>

Hi Matthew,

some minor feedback from my side, as it looks like there will be another
revision of this patchset anyway.

> ---
>  drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 139 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c

...

> @@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
>  	return -EINVAL;
>  }
>  
> +struct userdatum {
> +	struct config_item item;
> +	char value[MAX_USERDATA_VALUE_LENGTH];
> +};
> +
> +static inline struct userdatum *to_userdatum(struct config_item *item)
> +{
> +	return container_of(item, struct userdatum, item);
> +}

Please don't use the inline keyword in C files,
unless there is a demonstrable reason to do so.
Rather, please let the compiler inline code as is sees fit.

...

> @@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
>  	.ct_owner		= THIS_MODULE,
>  };
>  
> +static void init_target_config_group(struct netconsole_target *nt, const char *name)

nit: Networking still prefers code to be 80 columns wide or less.

...

Re: [PATCH net-next v2 6/8] net: netconsole: cache userdata formatted string in netconsole_target

[Simon Horman]

On Fri, Jan 26, 2024 at 03:13:41PM -0800, Matthew Wood wrote:
> Store a formatted string for userdata that will be appended to netconsole
> messages. The string has a capacity of 4KB, as calculated by the userdatum
> entry length of 256 bytes and a max of 16 userdata entries.
> 
> Update the stored netconsole_target->userdata_complete string with the new
> formatted userdata values when a userdatum is created, edited, or
> removed. Each userdata entry contains a trailing newline, which will be
> formatted as such in netconsole messages::
> 
>     6.7.0-rc8-virtme,12,500,1646292204,-;test
>     release=foo
>     something=bar
>     6.7.0-rc8-virtme,12,500,1646292204,-;another test
>     release=foo
>     something=bar
> 
> Enforcement of MAX_USERDATA_ITEMS is done in userdatum_make_item;
> update_userdata will not check for this case but will skip any userdata
> children over the limit of MAX_USERDATA_ITEMs.
> 
> If a userdata entry/dir is created but no value is provided, that entry
> will be skipped. This is in part because update_userdata() can't be
> called in userdatum_make_item() since the item will not have been added
> to the userdata config_group children yet. To preserve the experience of
> adding an empty userdata that doesn't show up in the netconsole
> messages, purposefully skip emtpy userdata items even when

nit: empty

> update_userdata() can be called.
> 
> Co-developed-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>

...

Re: [PATCH net-next v2 2/8] net: netconsole: move netconsole_target config_item to config_group

[Simon Horman]

On Fri, Jan 26, 2024 at 03:13:37PM -0800, Matthew Wood wrote:
> In order to support a nested userdata config_group in later patches,
> use a config_group for netconsole_target instead of a
> config_item. It's a no-op functionality-wise, since
> config_group maintains all features of a config_item via the cg_item
> member.
> 
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
>  drivers/net/netconsole.c | 61 ++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c

...

> @@ -665,8 +669,9 @@ static struct config_item *make_netconsole_target(struct config_group *group,
>  	if (!strncmp(name, NETCONSOLE_PARAM_TARGET_PREFIX,
>  		     strlen(NETCONSOLE_PARAM_TARGET_PREFIX))) {
>  		nt = find_cmdline_target(name);
> -		if (nt)
> -			return &nt->item;
> +		if (nt) {
> +			return &nt->group;
> +		}

nit: no need for {} here.

>  	}
>  
>  	nt = alloc_and_init();

...

Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target

[Matthew Wood]

On Fri, Feb 2, 2024 at 3:52 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Jan 26, 2024 at 03:13:40PM -0800, Matthew Wood wrote:
> > Create configfs machinery for netconsole userdata appending, which depends
> > on CONFIG_NETCONSOLE_DYNAMIC (for configfs interface). Add a userdata
> > config_group to netconsole_target for managing userdata entries as a tree
> > under the netconsole configfs subsystem. Directory names created under the
> > userdata directory become userdatum keys; the userdatum value is the
> > content of the value file.
> >
> > Include the minimum-viable-changes for userdata configfs config_group.
> > init_target_config_group() ties in the complete configfs machinery to
> > avoid unused func/variable errors during build. Initializing the
> > netconsole_target->group is moved to init_target_config_group, which
> > will also init and add the userdata config_group.
> >
> > Each userdatum entry has a limit of 256 bytes (54 for
> > the key/directory, 200 for the value, and 2 for '=' and '\n'
> > characters), which is enforced by the configfs functions for updating
> > the userdata config_group.
> >
> > When a new netconsole_target is created, initialize the userdata
> > config_group and add it as a default group for netconsole_target
> > config_group, allowing the userdata configfs sub-tree to be presented
> > in the netconsole configfs tree under the userdata directory.
> >
> > Co-developed-by: Breno Leitao <leitao@debian.org>
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
>
> Hi Matthew,
>
> some minor feedback from my side, as it looks like there will be another
> revision of this patchset anyway.
>
> > ---
> >  drivers/net/netconsole.c | 143 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 139 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
>
> ...
>
> > @@ -596,6 +606,123 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
> >       return -EINVAL;
> >  }
> >
> > +struct userdatum {
> > +     struct config_item item;
> > +     char value[MAX_USERDATA_VALUE_LENGTH];
> > +};
> > +
> > +static inline struct userdatum *to_userdatum(struct config_item *item)
> > +{
> > +     return container_of(item, struct userdatum, item);
> > +}
>
> Please don't use the inline keyword in C files,
> unless there is a demonstrable reason to do so.
> Rather, please let the compiler inline code as is sees fit.
>
> ...
>
> > @@ -640,6 +767,14 @@ static const struct config_item_type netconsole_target_type = {
> >       .ct_owner               = THIS_MODULE,
> >  };
> >
> > +static void init_target_config_group(struct netconsole_target *nt, const char *name)
>
> nit: Networking still prefers code to be 80 columns wide or less.
>
> ...

Hi Simon,

I appreciate the review, thank you for the feedback. I've addressed
the comments here and in the other patches too. I'll be posting a v3
soon with the changes.

Re: [PATCH net-next v2 5/8] net: netconsole: add a userdata config_group member to netconsole_target

[Simon Horman]

On Fri, Feb 02, 2024 at 08:05:07AM -0800, Matthew Wood wrote:

...

> Hi Simon,
> 
> I appreciate the review, thank you for the feedback. I've addressed
> the comments here and in the other patches too. I'll be posting a v3
> soon with the changes.
> 

Thanks Matthew,

likewise, much appreciated.