mirror of
https://github.com/torvalds/linux.git
synced 2026-05-27 00:22:00 +02:00
Merge branch 'net-netconsole-fix-netconsole-unsafe-locking'
Breno Leitao says:
====================
net: netconsole: Fix netconsole unsafe locking
Problem:
=======
The current locking mechanism in netconsole is unsafe and suboptimal due
to the following issues:
1) Lock Release and Reacquisition Mid-Loop:
In netconsole_netdev_event(), the target_list_lock is released and
reacquired within a loop, potentially causing collisions and cleaning up
targets that are being enabled.
int netconsole_netdev_event()
{
...
spin_lock_irqsave(&target_list_lock, flags);
list_for_each_entry(nt, &target_list, list) {
spin_unlock_irqrestore(&target_list_lock, flags);
__netpoll_cleanup(&nt->np);
spin_lock_irqsave(&target_list_lock, flags);
}
spin_lock_irqsave(&target_list_lock, flags);
...
}
2) Non-Atomic Cleanup Operations:
In enabled_store(), the cleanup of structures is not atomic, risking
cleanup of structures that are in the process of being enabled.
size_t enabled_store()
{
...
spin_lock_irqsave(&target_list_lock, flags);
nt->enabled = false;
spin_unlock_irqrestore(&target_list_lock, flags);
netpoll_cleanup(&nt->np);
...
}
These issues stem from the following limitations in netconsole's locking
design:
1) write_{ext_}msg() functions:
a) Cannot sleep
b) Must iterate through targets and send messages to all enabled entries.
c) List iteration is protected by target_list_lock spinlock.
2) Network event handling in netconsole_netdev_event():
a) Needs to sleep
b) Requires iteration over the target list (holding
target_list_lock spinlock).
c) Some events necessitate netpoll struct cleanup, which *needs*
to sleep.
The target_list_lock needs to be used by non-sleepable functions while
also protecting operations that may sleep, leading to the current unsafe
design.
Solution:
========
1) Dual Locking Mechanism:
- Retain current target_list_lock for non-sleepable use cases.
- Introduce target_cleanup_list_lock (mutex) for sleepable
operations.
2) Deferred Cleanup:
- Implement atomic, deferred cleanup of structures using the new
mutex (target_cleanup_list_lock).
- Avoid the `goto` in the middle of the list_for_each_entry
3) Separate Cleanup List:
- Create target_cleanup_list for deferred cleanup, protected by
target_cleanup_list_lock.
- This allows cleanup() to sleep without affecting message
transmission.
- When iterating over targets, move devices needing cleanup to
target_cleanup_list.
- Handle cleanup under the target_cleanup_list_lock mutex.
4) Make a clear locking hierarchy
- The target_cleanup_list_lock takes precedence over target_list_lock.
- Major Workflow Locking Sequences:
a) Network Event Affecting Netpoll (netconsole_netdev_event):
rtnl -> target_cleanup_list_lock -> target_list_lock
b) Message Writing (write_msg()):
console_lock -> target_list_lock
c) Configfs Target Enable/Disable (enabled_store()):
dynamic_netconsole_mutex -> target_cleanup_list_lock -> target_list_lock
This hierarchy ensures consistent lock acquisition order across
different operations, preventing deadlocks and maintaining proper
synchronization. The target_cleanup_list_lock's higher priority allows
for safe deferred cleanup operations without interfering with regular
message transmission protected by target_list_lock. Each workflow
follows a specific locking sequence, ensuring that operations like
network event handling, message writing, and target management are
properly synchronized and do not conflict with each other.
Changelog:
v3:
* Move netconsole_process_cleanups() function to inside
CONFIG_NETCONSOLE_DYNAMIC block, avoiding Werror=unused-function
(Jakub)
v2:
* The selftest has been removed from the patchset because veth is now
IFF_DISABLE_NETPOLL. A new test will be sent separately.
* https://lore.kernel.org/all/20240807091657.4191542-1-leitao@debian.org/
v1:
* https://lore.kernel.org/all/20240801161213.2707132-1-leitao@debian.org/
====================
Link: https://patch.msgid.link/20240808122518.498166-1-leitao@debian.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
This commit is contained in:
Paolo Abeni
commit
2c9c2a3d1a
|
|
@ -37,6 +37,7 @@
|
|||
#include <linux/configfs.h>
|
||||
#include <linux/etherdevice.h>
|
||||
#include <linux/utsname.h>
|
||||
#include <linux/rtnetlink.h>
|
||||
|
||||
MODULE_AUTHOR("Matt Mackall <mpm@selenic.com>");
|
||||
MODULE_DESCRIPTION("Console driver for network interfaces");
|
||||
|
|
@ -72,9 +73,16 @@ __setup("netconsole=", option_setup);
|
|||
|
||||
/* Linked list of all configured targets */
|
||||
static LIST_HEAD(target_list);
|
||||
/* target_cleanup_list is used to track targets that need to be cleaned outside
|
||||
* of target_list_lock. It should be cleaned in the same function it is
|
||||
* populated.
|
||||
*/
|
||||
static LIST_HEAD(target_cleanup_list);
|
||||
|
||||
/* This needs to be a spinlock because write_msg() cannot sleep */
|
||||
static DEFINE_SPINLOCK(target_list_lock);
|
||||
/* This needs to be a mutex because netpoll_cleanup might sleep */
|
||||
static DEFINE_MUTEX(target_cleanup_list_lock);
|
||||
|
||||
/*
|
||||
* Console driver for extended netconsoles. Registered on the first use to
|
||||
|
|
@ -210,6 +218,33 @@ static struct netconsole_target *alloc_and_init(void)
|
|||
return nt;
|
||||
}
|
||||
|
||||
/* Clean up every target in the cleanup_list and move the clean targets back to
|
||||
* the main target_list.
|
||||
*/
|
||||
static void netconsole_process_cleanups_core(void)
|
||||
{
|
||||
struct netconsole_target *nt, *tmp;
|
||||
unsigned long flags;
|
||||
|
||||
/* The cleanup needs RTNL locked */
|
||||
ASSERT_RTNL();
|
||||
|
||||
mutex_lock(&target_cleanup_list_lock);
|
||||
list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list) {
|
||||
/* all entries in the cleanup_list needs to be disabled */
|
||||
WARN_ON_ONCE(nt->enabled);
|
||||
do_netpoll_cleanup(&nt->np);
|
||||
/* moved the cleaned target to target_list. Need to hold both
|
||||
* locks
|
||||
*/
|
||||
spin_lock_irqsave(&target_list_lock, flags);
|
||||
list_move(&nt->list, &target_list);
|
||||
spin_unlock_irqrestore(&target_list_lock, flags);
|
||||
}
|
||||
WARN_ON_ONCE(!list_empty(&target_cleanup_list));
|
||||
mutex_unlock(&target_cleanup_list_lock);
|
||||
}
|
||||
|
||||
#ifdef CONFIG_NETCONSOLE_DYNAMIC
|
||||
|
||||
/*
|
||||
|
|
@ -246,6 +281,19 @@ static struct netconsole_target *to_target(struct config_item *item)
|
|||
struct netconsole_target, group);
|
||||
}
|
||||
|
||||
/* Do the list cleanup with the rtnl lock hold. rtnl lock is necessary because
|
||||
* netdev might be cleaned-up by calling __netpoll_cleanup(),
|
||||
*/
|
||||
static void netconsole_process_cleanups(void)
|
||||
{
|
||||
/* rtnl lock is called here, because it has precedence over
|
||||
* target_cleanup_list_lock mutex and target_cleanup_list
|
||||
*/
|
||||
rtnl_lock();
|
||||
netconsole_process_cleanups_core();
|
||||
rtnl_unlock();
|
||||
}
|
||||
|
||||
/* Get rid of possible trailing newline, returning the new length */
|
||||
static void trim_newline(char *s, size_t maxlen)
|
||||
{
|
||||
|
|
@ -336,14 +384,14 @@ static ssize_t enabled_store(struct config_item *item,
|
|||
struct netconsole_target *nt = to_target(item);
|
||||
unsigned long flags;
|
||||
bool enabled;
|
||||
int err;
|
||||
ssize_t ret;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
err = kstrtobool(buf, &enabled);
|
||||
if (err)
|
||||
ret = kstrtobool(buf, &enabled);
|
||||
if (ret)
|
||||
goto out_unlock;
|
||||
|
||||
err = -EINVAL;
|
||||
ret = -EINVAL;
|
||||
if (enabled == nt->enabled) {
|
||||
pr_info("network logging has already %s\n",
|
||||
nt->enabled ? "started" : "stopped");
|
||||
|
|
@ -365,8 +413,8 @@ static ssize_t enabled_store(struct config_item *item,
|
|||
*/
|
||||
netpoll_print_options(&nt->np);
|
||||
|
||||
err = netpoll_setup(&nt->np);
|
||||
if (err)
|
||||
ret = netpoll_setup(&nt->np);
|
||||
if (ret)
|
||||
goto out_unlock;
|
||||
|
||||
nt->enabled = true;
|
||||
|
|
@ -376,17 +424,23 @@ static ssize_t enabled_store(struct config_item *item,
|
|||
* otherwise we might end up in write_msg() with
|
||||
* nt->np.dev == NULL and nt->enabled == true
|
||||
*/
|
||||
mutex_lock(&target_cleanup_list_lock);
|
||||
spin_lock_irqsave(&target_list_lock, flags);
|
||||
nt->enabled = false;
|
||||
/* Remove the target from the list, while holding
|
||||
* target_list_lock
|
||||
*/
|
||||
list_move(&nt->list, &target_cleanup_list);
|
||||
spin_unlock_irqrestore(&target_list_lock, flags);
|
||||
netpoll_cleanup(&nt->np);
|
||||
mutex_unlock(&target_cleanup_list_lock);
|
||||
}
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
/* Deferred cleanup */
|
||||
netconsole_process_cleanups();
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return err;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t release_store(struct config_item *item, const char *buf,
|
||||
|
|
@ -394,27 +448,26 @@ static ssize_t release_store(struct config_item *item, const char *buf,
|
|||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
bool release;
|
||||
int err;
|
||||
ssize_t ret;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
pr_err("target (%s) is enabled, disable to update parameters\n",
|
||||
config_item_name(&nt->group.cg_item));
|
||||
err = -EINVAL;
|
||||
ret = -EINVAL;
|
||||
goto out_unlock;
|
||||
}
|
||||
|
||||
err = kstrtobool(buf, &release);
|
||||
if (err)
|
||||
ret = kstrtobool(buf, &release);
|
||||
if (ret)
|
||||
goto out_unlock;
|
||||
|
||||
nt->release = release;
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return err;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t extended_store(struct config_item *item, const char *buf,
|
||||
|
|
@ -422,27 +475,25 @@ static ssize_t extended_store(struct config_item *item, const char *buf,
|
|||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
bool extended;
|
||||
int err;
|
||||
ssize_t ret;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
pr_err("target (%s) is enabled, disable to update parameters\n",
|
||||
config_item_name(&nt->group.cg_item));
|
||||
err = -EINVAL;
|
||||
ret = -EINVAL;
|
||||
goto out_unlock;
|
||||
}
|
||||
|
||||
err = kstrtobool(buf, &extended);
|
||||
if (err)
|
||||
ret = kstrtobool(buf, &extended);
|
||||
if (ret)
|
||||
goto out_unlock;
|
||||
|
||||
nt->extended = extended;
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return err;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t dev_name_store(struct config_item *item, const char *buf,
|
||||
|
|
@ -469,7 +520,7 @@ static ssize_t local_port_store(struct config_item *item, const char *buf,
|
|||
size_t count)
|
||||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
int rv = -EINVAL;
|
||||
ssize_t ret = -EINVAL;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
|
|
@ -478,21 +529,20 @@ static ssize_t local_port_store(struct config_item *item, const char *buf,
|
|||
goto out_unlock;
|
||||
}
|
||||
|
||||
rv = kstrtou16(buf, 10, &nt->np.local_port);
|
||||
if (rv < 0)
|
||||
ret = kstrtou16(buf, 10, &nt->np.local_port);
|
||||
if (ret < 0)
|
||||
goto out_unlock;
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return rv;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t remote_port_store(struct config_item *item,
|
||||
const char *buf, size_t count)
|
||||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
int rv = -EINVAL;
|
||||
ssize_t ret = -EINVAL;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
|
|
@ -501,20 +551,20 @@ static ssize_t remote_port_store(struct config_item *item,
|
|||
goto out_unlock;
|
||||
}
|
||||
|
||||
rv = kstrtou16(buf, 10, &nt->np.remote_port);
|
||||
if (rv < 0)
|
||||
ret = kstrtou16(buf, 10, &nt->np.remote_port);
|
||||
if (ret < 0)
|
||||
goto out_unlock;
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return rv;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t local_ip_store(struct config_item *item, const char *buf,
|
||||
size_t count)
|
||||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
ssize_t ret = -EINVAL;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
|
|
@ -541,17 +591,17 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
|
|||
goto out_unlock;
|
||||
}
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return -EINVAL;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t remote_ip_store(struct config_item *item, const char *buf,
|
||||
size_t count)
|
||||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
ssize_t ret = -EINVAL;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
|
|
@ -578,11 +628,10 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
|
|||
goto out_unlock;
|
||||
}
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return -EINVAL;
|
||||
return ret;
|
||||
}
|
||||
|
||||
static ssize_t remote_mac_store(struct config_item *item, const char *buf,
|
||||
|
|
@ -590,6 +639,7 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
|
|||
{
|
||||
struct netconsole_target *nt = to_target(item);
|
||||
u8 remote_mac[ETH_ALEN];
|
||||
ssize_t ret = -EINVAL;
|
||||
|
||||
mutex_lock(&dynamic_netconsole_mutex);
|
||||
if (nt->enabled) {
|
||||
|
|
@ -604,11 +654,10 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
|
|||
goto out_unlock;
|
||||
memcpy(nt->np.remote_mac, remote_mac, ETH_ALEN);
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return strnlen(buf, count);
|
||||
ret = strnlen(buf, count);
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return -EINVAL;
|
||||
return ret;
|
||||
}
|
||||
|
||||
struct userdatum {
|
||||
|
|
@ -685,7 +734,7 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
|
|||
struct userdatum *udm = to_userdatum(item);
|
||||
struct netconsole_target *nt;
|
||||
struct userdata *ud;
|
||||
int ret;
|
||||
ssize_t ret;
|
||||
|
||||
if (count > MAX_USERDATA_VALUE_LENGTH)
|
||||
return -EMSGSIZE;
|
||||
|
|
@ -700,9 +749,7 @@ 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);
|
||||
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return count;
|
||||
ret = count;
|
||||
out_unlock:
|
||||
mutex_unlock(&dynamic_netconsole_mutex);
|
||||
return ret;
|
||||
|
|
@ -950,7 +997,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
|
|||
unsigned long event, void *ptr)
|
||||
{
|
||||
unsigned long flags;
|
||||
struct netconsole_target *nt;
|
||||
struct netconsole_target *nt, *tmp;
|
||||
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
|
||||
bool stopped = false;
|
||||
|
||||
|
|
@ -958,9 +1005,9 @@ static int netconsole_netdev_event(struct notifier_block *this,
|
|||
event == NETDEV_RELEASE || event == NETDEV_JOIN))
|
||||
goto done;
|
||||
|
||||
mutex_lock(&target_cleanup_list_lock);
|
||||
spin_lock_irqsave(&target_list_lock, flags);
|
||||
restart:
|
||||
list_for_each_entry(nt, &target_list, list) {
|
||||
list_for_each_entry_safe(nt, tmp, &target_list, list) {
|
||||
netconsole_target_get(nt);
|
||||
if (nt->np.dev == dev) {
|
||||
switch (event) {
|
||||
|
|
@ -970,25 +1017,16 @@ static int netconsole_netdev_event(struct notifier_block *this,
|
|||
case NETDEV_RELEASE:
|
||||
case NETDEV_JOIN:
|
||||
case NETDEV_UNREGISTER:
|
||||
/* rtnl_lock already held
|
||||
* we might sleep in __netpoll_cleanup()
|
||||
*/
|
||||
nt->enabled = false;
|
||||
spin_unlock_irqrestore(&target_list_lock, flags);
|
||||
|
||||
__netpoll_cleanup(&nt->np);
|
||||
|
||||
spin_lock_irqsave(&target_list_lock, flags);
|
||||
netdev_put(nt->np.dev, &nt->np.dev_tracker);
|
||||
nt->np.dev = NULL;
|
||||
list_move(&nt->list, &target_cleanup_list);
|
||||
stopped = true;
|
||||
netconsole_target_put(nt);
|
||||
goto restart;
|
||||
}
|
||||
}
|
||||
netconsole_target_put(nt);
|
||||
}
|
||||
spin_unlock_irqrestore(&target_list_lock, flags);
|
||||
mutex_unlock(&target_cleanup_list_lock);
|
||||
|
||||
if (stopped) {
|
||||
const char *msg = "had an event";
|
||||
|
||||
|
|
@ -1007,6 +1045,11 @@ static int netconsole_netdev_event(struct notifier_block *this,
|
|||
dev->name, msg);
|
||||
}
|
||||
|
||||
/* Process target_cleanup_list entries. By the end, target_cleanup_list
|
||||
* should be empty
|
||||
*/
|
||||
netconsole_process_cleanups_core();
|
||||
|
||||
done:
|
||||
return NOTIFY_DONE;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -64,6 +64,7 @@ int netpoll_setup(struct netpoll *np);
|
|||
void __netpoll_cleanup(struct netpoll *np);
|
||||
void __netpoll_free(struct netpoll *np);
|
||||
void netpoll_cleanup(struct netpoll *np);
|
||||
void do_netpoll_cleanup(struct netpoll *np);
|
||||
netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb);
|
||||
|
||||
#ifdef CONFIG_NETPOLL
|
||||
|
|
|
|||
|
|
@ -853,14 +853,20 @@ void __netpoll_free(struct netpoll *np)
|
|||
}
|
||||
EXPORT_SYMBOL_GPL(__netpoll_free);
|
||||
|
||||
void do_netpoll_cleanup(struct netpoll *np)
|
||||
{
|
||||
__netpoll_cleanup(np);
|
||||
netdev_put(np->dev, &np->dev_tracker);
|
||||
np->dev = NULL;
|
||||
}
|
||||
EXPORT_SYMBOL(do_netpoll_cleanup);
|
||||
|
||||
void netpoll_cleanup(struct netpoll *np)
|
||||
{
|
||||
rtnl_lock();
|
||||
if (!np->dev)
|
||||
goto out;
|
||||
__netpoll_cleanup(np);
|
||||
netdev_put(np->dev, &np->dev_tracker);
|
||||
np->dev = NULL;
|
||||
do_netpoll_cleanup(np);
|
||||
out:
|
||||
rtnl_unlock();
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user