[PATCH net] netconsole: Acquire su_mutex before navigating configs hierarchy

[PATCH net] netconsole: Acquire su_mutex before navigating configs hierarchy

[Gustavo Luiz Duarte]

There is a race between operations that iterate over the userdata
cg_children list and concurrent add/remove of userdata items through
configfs. The update_userdata() function iterates over the
nt->userdata_group.cg_children list, and count_extradata_entries() also
iterates over this same list to count nodes.

Quoting from Documentation/filesystems/configfs.rst:
> A subsystem can navigate the cg_children list and the ci_parent pointer
> to see the tree created by the subsystem.  This can race with configfs'
> management of the hierarchy, so configfs uses the subsystem mutex to
> protect modifications.  Whenever a subsystem wants to navigate the
> hierarchy, it must do so under the protection of the subsystem
> mutex.

Without proper locking, if a userdata item is added or removed
concurrently while these functions are iterating, the list can be
accessed in an inconsistent state. For example, the list_for_each() loop
can reach a node that is being removed from the list by list_del_init()
which sets the nodes' .next pointer to point to itself, so the loop will
never end (or reach the WARN_ON_ONCE in update_userdata() ).

Fix this by holding the configfs subsystem mutex (su_mutex) during all
operations that iterate over cg_children.
This includes:
- userdatum_value_store() which calls update_userdata() to iterate over
  cg_children
- All sysdata_*_enabled_store() functions which call
  count_extradata_entries() to iterate over cg_children

The su_mutex must be acquired before dynamic_netconsole_mutex to avoid
potential lock ordering issues, as configfs operations may already hold
su_mutex when calling into our code.

Fixes: df03f830d099 ("net: netconsole: cache userdata formatted string in netconsole_target")
Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
---
 drivers/net/netconsole.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 194570443493..9e17632e0bc6 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -931,6 +931,7 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 	if (count > MAX_EXTRADATA_VALUE_LEN)
 		return -EMSGSIZE;
 
+	mutex_lock(&netconsole_subsys.su_mutex);
 	mutex_lock(&dynamic_netconsole_mutex);
 
 	ret = strscpy(udm->value, buf, sizeof(udm->value));
@@ -944,6 +945,7 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 	ret = count;
 out_unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
+	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
 }
 
@@ -969,6 +971,7 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
 	if (ret)
 		return ret;
 
+	mutex_lock(&netconsole_subsys.su_mutex);
 	mutex_lock(&dynamic_netconsole_mutex);
 	curr = !!(nt->sysdata_fields & SYSDATA_MSGID);
 	if (msgid_enabled == curr)
@@ -989,6 +992,7 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
 	ret = strnlen(buf, count);
 unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
+	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
 }
 
@@ -1003,6 +1007,7 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
 	if (ret)
 		return ret;
 
+	mutex_lock(&netconsole_subsys.su_mutex);
 	mutex_lock(&dynamic_netconsole_mutex);
 	curr = !!(nt->sysdata_fields & SYSDATA_RELEASE);
 	if (release_enabled == curr)
@@ -1023,6 +1028,7 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
 	ret = strnlen(buf, count);
 unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
+	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
 }
 
@@ -1037,6 +1043,7 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
 	if (ret)
 		return ret;
 
+	mutex_lock(&netconsole_subsys.su_mutex);
 	mutex_lock(&dynamic_netconsole_mutex);
 	curr = !!(nt->sysdata_fields & SYSDATA_TASKNAME);
 	if (taskname_enabled == curr)
@@ -1057,6 +1064,7 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
 	ret = strnlen(buf, count);
 unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
+	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
 }
 
@@ -1072,6 +1080,7 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 	if (ret)
 		return ret;
 
+	mutex_lock(&netconsole_subsys.su_mutex);
 	mutex_lock(&dynamic_netconsole_mutex);
 	curr = !!(nt->sysdata_fields & SYSDATA_CPU_NR);
 	if (cpu_nr_enabled == curr)
@@ -1100,6 +1109,7 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 	ret = strnlen(buf, count);
 unlock:
 	mutex_unlock(&dynamic_netconsole_mutex);
+	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
 }
 

---
base-commit: 210b35d6a7ea415494ce75490c4b43b4e717d935
change-id: 20251028-netconsole-fix-warn-c8f0cf294276

Best regards,
-- 
Gustavo Luiz Duarte <gustavold@gmail.com>

Re: [PATCH net] netconsole: Acquire su_mutex before navigating configs hierarchy

[Jakub Kicinski]

On Wed, 29 Oct 2025 13:50:24 -0700 Gustavo Luiz Duarte wrote:
> There is a race between operations that iterate over the userdata
> cg_children list and concurrent add/remove of userdata items through
> configfs. The update_userdata() function iterates over the
> nt->userdata_group.cg_children list, and count_extradata_entries() also
> iterates over this same list to count nodes.

Teaching syzbot how to create a netconsole would probably go a long way
towards catching these sort of issues. Looks like right now syzbot has
coverage only for the paths that don't need a target:
https://syzkaller.appspot.com/upstream/coverage?period=month

Re: [PATCH net] netconsole: Acquire su_mutex before navigating configs hierarchy

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 Oct 2025 13:50:24 -0700 you wrote:
> There is a race between operations that iterate over the userdata
> cg_children list and concurrent add/remove of userdata items through
> configfs. The update_userdata() function iterates over the
> nt->userdata_group.cg_children list, and count_extradata_entries() also
> iterates over this same list to count nodes.
> 
> Quoting from Documentation/filesystems/configfs.rst:
> > A subsystem can navigate the cg_children list and the ci_parent pointer
> > to see the tree created by the subsystem.  This can race with configfs'
> > management of the hierarchy, so configfs uses the subsystem mutex to
> > protect modifications.  Whenever a subsystem wants to navigate the
> > hierarchy, it must do so under the protection of the subsystem
> > mutex.
> 
> [...]

Here is the summary with links:
  - [net] netconsole: Acquire su_mutex before navigating configs hierarchy
    https://git.kernel.org/netdev/net/c/d7d2fcf7ae31

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html