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 | 84 +++++++++++++++++++++++++++++++-----------------
1 file changed, 54 insertions(+), 30 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 1bd811714322..12fbc303a824 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -155,7 +155,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];
@@ -875,45 +875,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,
@@ -937,7 +953,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);
@@ -1193,7 +1211,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 = {
@@ -1874,6 +1895,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.3On Thu, Nov 13, 2025 at 08:42:20AM -0800, Gustavo Luiz Duarte wrote:
> @@ -875,45 +875,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 */
Please create a function for this one.
> 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);
If we trigger this WARN_ON_ONCE, please return, and do not proceed with
the buffer replacement.
> +
> + /* 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);
if we hit the warning above, then offset < len, and we are wrapping some
item, right?
> +
> + /* 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;
This should be nt->userdata_length = offset, supposing the scnprintf got
trimmed, and the WARN_ON_ONCE above got triggered. Offset is the lenght
that was appened to new_buf.
> spin_unlock_irqrestore(&target_list_lock, flags);
> +
> + kfree(old_buf);
> +
> + return 0;
> }
This seems all safe. update_userdata() is called with never called in
parallel, given it should be called with dynamic_netconsole_mutex, and
nt-> operations are protected by target_list_lock.
The only concern is nt->userdata_length = offset (instead of len).
>
> static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
> @@ -937,7 +953,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);
> @@ -1193,7 +1211,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);
Thinking about this now, I suppose netconsole might be reading this in
parallel, and then we are freeing userdata mid-air.
Don't we need the target_list_lock in here ?
--
pw-bot: cr
On Fri, Nov 14, 2025 at 1:04 PM Breno Leitao <leitao@debian.org> wrote:
>
> On Thu, Nov 13, 2025 at 08:42:20AM -0800, Gustavo Luiz Duarte wrote:
> > @@ -875,45 +875,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 */
>
> Please create a function for this one.
will do in v3
>
> > 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);
>
> If we trigger this WARN_ON_ONCE, please return, and do not proceed with
> the buffer replacement.
will do in v3.
>
> > +
> > + /* 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);
>
> if we hit the warning above, then offset < len, and we are wrapping some
> item, right?
>
> > +
> > + /* 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;
>
> This should be nt->userdata_length = offset, supposing the scnprintf got
> trimmed, and the WARN_ON_ONCE above got triggered. Offset is the lenght
> that was appened to new_buf.
Agree. Will use offset instead of len here in v3.
>
> > spin_unlock_irqrestore(&target_list_lock, flags);
> > +
> > + kfree(old_buf);
> > +
> > + return 0;
> > }
>
> This seems all safe. update_userdata() is called with never called in
> parallel, given it should be called with dynamic_netconsole_mutex, and
> nt-> operations are protected by target_list_lock.
>
> The only concern is nt->userdata_length = offset (instead of len).
>
> >
> > static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
> > @@ -937,7 +953,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);
> > @@ -1193,7 +1211,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);
>
> Thinking about this now, I suppose netconsole might be reading this in
> parallel, and then we are freeing userdata mid-air.
>
> Don't we need the target_list_lock in here ?
This method is called after drop_netconsole_target(), which removes
the target from target_list. This guarantees that we won't race with
write_ext_msg().
Also, a config_group cannot be removed while it still has child items.
This guarantees that we won't race with userdata or attribute
operations.
So I believe this is safe.
>
> --
> pw-bot: cr
> > > static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
> > > @@ -937,7 +953,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);
> > > @@ -1193,7 +1211,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);
> >
> > Thinking about this now, I suppose netconsole might be reading this in
> > parallel, and then we are freeing userdata mid-air.
> >
> > Don't we need the target_list_lock in here ?
>
> This method is called after drop_netconsole_target(), which removes
> the target from target_list. This guarantees that we won't race with
> write_ext_msg().
> Also, a config_group cannot be removed while it still has child items.
> This guarantees that we won't race with userdata or attribute
> operations.
> So I believe this is safe.
Thanks for checking it!
--breno
© 2016 - 2026 Red Hat, Inc.