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