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.
> 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
