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