On Fri, Nov 14, 2025 at 1:04 PM Breno Leitao <[email protected]> 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
