From 13f172ff26563995049abe73f6eeba828de3c09d Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 22 Apr 2011 08:10:59 +0000 Subject: netconsole: fix deadlock when removing net driver that netconsole is using (v2) A deadlock was reported to me recently that occured when netconsole was being used in a virtual guest. If the virtio_net driver was removed while netconsole was setup to use an interface that was driven by that driver, the guest deadlocked. No backtrace was provided because netconsole was the only console configured, but it became clear pretty quickly what the problem was. In netconsole_netdev_event, if we get an unregister event, we call __netpoll_cleanup with the target_list_lock held and irqs disabled. __netpoll_cleanup can, if pending netpoll packets are waiting call cancel_delayed_work_sync, which is a sleeping path. the might_sleep call in that path gets triggered, causing a console warning to be issued. The netconsole write handler of course tries to take the target_list_lock again, which we already hold, causing deadlock. The fix is pretty striaghtforward. Simply drop the target_list_lock and re-enable irqs prior to calling __netpoll_cleanup, the re-acquire the lock, and restart the loop. Confirmed by myself to fix the problem reported. Signed-off-by: Neil Horman CC: "David S. Miller" Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index dfb67eb2a94b..eb41e44921e6 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -671,6 +671,7 @@ static int netconsole_netdev_event(struct notifier_block *this, goto done; spin_lock_irqsave(&target_list_lock, flags); +restart: list_for_each_entry(nt, &target_list, list) { netconsole_target_get(nt); if (nt->np.dev == dev) { @@ -683,9 +684,16 @@ static int netconsole_netdev_event(struct notifier_block *this, * rtnl_lock already held */ if (nt->np.dev) { + spin_unlock_irqrestore( + &target_list_lock, + flags); __netpoll_cleanup(&nt->np); + spin_lock_irqsave(&target_list_lock, + flags); dev_put(nt->np.dev); nt->np.dev = NULL; + netconsole_target_put(nt); + goto restart; } /* Fall through */ case NETDEV_GOING_DOWN: -- cgit v1.2.3 From 99f823f98fb981b55c663a3783c3d2293958ece4 Mon Sep 17 00:00:00 2001 From: Alexey Dobriyan Date: Sat, 7 May 2011 20:33:13 +0000 Subject: netconsole: switch to kstrto*() functions Signed-off-by: Alexey Dobriyan Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 62 +++++++++++------------------------------------- 1 file changed, 14 insertions(+), 48 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index eb41e44921e6..62fdbaa1fb60 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -241,34 +241,6 @@ static struct netconsole_target *to_target(struct config_item *item) NULL; } -/* - * Wrapper over simple_strtol (base 10) with sanity and range checking. - * We return (signed) long only because we may want to return errors. - * Do not use this to convert numbers that are allowed to be negative. - */ -static long strtol10_check_range(const char *cp, long min, long max) -{ - long ret; - char *p = (char *) cp; - - WARN_ON(min < 0); - WARN_ON(max < min); - - ret = simple_strtol(p, &p, 10); - - if (*p && (*p != '\n')) { - printk(KERN_ERR "netconsole: invalid input\n"); - return -EINVAL; - } - if ((ret < min) || (ret > max)) { - printk(KERN_ERR "netconsole: input %ld must be between " - "%ld and %ld\n", ret, min, max); - return -EINVAL; - } - - return ret; -} - /* * Attribute operations for netconsole_target. */ @@ -327,12 +299,14 @@ static ssize_t store_enabled(struct netconsole_target *nt, const char *buf, size_t count) { + int enabled; int err; - long enabled; - enabled = strtol10_check_range(buf, 0, 1); - if (enabled < 0) - return enabled; + err = kstrtoint(buf, 10, &enabled); + if (err < 0) + return err; + if (enabled < 0 || enabled > 1) + return -EINVAL; if (enabled) { /* 1 */ @@ -384,8 +358,7 @@ static ssize_t store_local_port(struct netconsole_target *nt, const char *buf, size_t count) { - long local_port; -#define __U16_MAX ((__u16) ~0U) + int rv; if (nt->enabled) { printk(KERN_ERR "netconsole: target (%s) is enabled, " @@ -394,12 +367,9 @@ static ssize_t store_local_port(struct netconsole_target *nt, return -EINVAL; } - local_port = strtol10_check_range(buf, 0, __U16_MAX); - if (local_port < 0) - return local_port; - - nt->np.local_port = local_port; - + rv = kstrtou16(buf, 10, &nt->np.local_port); + if (rv < 0) + return rv; return strnlen(buf, count); } @@ -407,8 +377,7 @@ static ssize_t store_remote_port(struct netconsole_target *nt, const char *buf, size_t count) { - long remote_port; -#define __U16_MAX ((__u16) ~0U) + int rv; if (nt->enabled) { printk(KERN_ERR "netconsole: target (%s) is enabled, " @@ -417,12 +386,9 @@ static ssize_t store_remote_port(struct netconsole_target *nt, return -EINVAL; } - remote_port = strtol10_check_range(buf, 0, __U16_MAX); - if (remote_port < 0) - return remote_port; - - nt->np.remote_port = remote_port; - + rv = kstrtou16(buf, 10, &nt->np.remote_port); + if (rv < 0) + return rv; return strnlen(buf, count); } -- cgit v1.2.3 From 4940fc889e1e63667a15243028ddcd84d471cd8e Mon Sep 17 00:00:00 2001 From: Alexey Dobriyan Date: Sat, 7 May 2011 23:00:07 +0000 Subject: net: add mac_pton() for parsing MAC address mac_pton() parses MAC address in form XX:XX:XX:XX:XX:XX and only in that form. mac_pton() doesn't dirty result until it's sure string representation is valid. mac_pton() doesn't care about characters _after_ last octet, it's up to caller to deal with it. mac_pton() diverges from 0/-E return value convention. Target usage: if (!mac_pton(str, whatever->mac)) return -EINVAL; /* ->mac being u8 [ETH_ALEN] is filled at this point. */ /* optionally check str[3 * ETH_ALEN - 1] for termination */ Use mac_pton() in pktgen and netconsole for start. Signed-off-by: Alexey Dobriyan Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 62fdbaa1fb60..a83e101440fd 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -429,8 +429,6 @@ static ssize_t store_remote_mac(struct netconsole_target *nt, size_t count) { u8 remote_mac[ETH_ALEN]; - char *p = (char *) buf; - int i; if (nt->enabled) { printk(KERN_ERR "netconsole: target (%s) is enabled, " @@ -439,23 +437,13 @@ static ssize_t store_remote_mac(struct netconsole_target *nt, return -EINVAL; } - for (i = 0; i < ETH_ALEN - 1; i++) { - remote_mac[i] = simple_strtoul(p, &p, 16); - if (*p != ':') - goto invalid; - p++; - } - remote_mac[ETH_ALEN - 1] = simple_strtoul(p, &p, 16); - if (*p && (*p != '\n')) - goto invalid; - + if (!mac_pton(buf, remote_mac)) + return -EINVAL; + if (buf[3 * ETH_ALEN - 1] && buf[3 * ETH_ALEN - 1] != '\n') + return -EINVAL; memcpy(nt->np.remote_mac, remote_mac, ETH_ALEN); return strnlen(buf, count); - -invalid: - printk(KERN_ERR "netconsole: invalid input\n"); - return -EINVAL; } /* -- cgit v1.2.3 From 8d8fc29d02a33e4bd5f4fa47823c1fd386346093 Mon Sep 17 00:00:00 2001 From: Amerigo Wang Date: Thu, 19 May 2011 21:39:10 +0000 Subject: netpoll: disable netpoll when enslave a device V3: rename NETDEV_ENSLAVE to NETDEV_JOIN Currently we do nothing when we enslave a net device which is running netconsole. Neil pointed out that we may get weird results in such case, so let's disable netpoll on the device being enslaved. I think it is too harsh to prevent the device being ensalved if it is running netconsole. By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole netdev notifier, because netpoll will check if the device is running or not and we don't handle NETDEV_PRE_UP neither. This patch is based on net-next-2.6. Signed-off-by: WANG Cong Cc: Neil Horman Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index a83e101440fd..4190786de403 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -621,11 +621,10 @@ static int netconsole_netdev_event(struct notifier_block *this, bool stopped = false; if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER || - event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN)) + event == NETDEV_BONDING_DESLAVE || event == NETDEV_JOIN)) goto done; spin_lock_irqsave(&target_list_lock, flags); -restart: list_for_each_entry(nt, &target_list, list) { netconsole_target_get(nt); if (nt->np.dev == dev) { @@ -633,6 +632,8 @@ restart: case NETDEV_CHANGENAME: strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ); break; + case NETDEV_BONDING_DESLAVE: + case NETDEV_JOIN: case NETDEV_UNREGISTER: /* * rtnl_lock already held @@ -647,11 +648,7 @@ restart: dev_put(nt->np.dev); nt->np.dev = NULL; netconsole_target_put(nt); - goto restart; } - /* Fall through */ - case NETDEV_GOING_DOWN: - case NETDEV_BONDING_DESLAVE: nt->enabled = 0; stopped = true; break; @@ -660,10 +657,21 @@ restart: netconsole_target_put(nt); } spin_unlock_irqrestore(&target_list_lock, flags); - if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE)) + if (stopped) { printk(KERN_INFO "netconsole: network logging stopped on " - "interface %s as it %s\n", dev->name, - event == NETDEV_UNREGISTER ? "unregistered" : "released slaves"); + "interface %s as it ", dev->name); + switch (event) { + case NETDEV_UNREGISTER: + printk(KERN_CONT "unregistered\n"); + break; + case NETDEV_BONDING_DESLAVE: + printk(KERN_CONT "released slaves\n"); + break; + case NETDEV_JOIN: + printk(KERN_CONT "is joining a master device\n"); + break; + } + } done: return NOTIFY_DONE; -- cgit v1.2.3 From daf9209bb2c8b07ca025eac82e3d175534086c77 Mon Sep 17 00:00:00 2001 From: Amerigo Wang Date: Thu, 19 May 2011 21:39:12 +0000 Subject: net: rename NETDEV_BONDING_DESLAVE to NETDEV_RELEASE s/NETDEV_BONDING_DESLAVE/NETDEV_RELEASE/ as Andy suggested. Signed-off-by: WANG Cong Cc: Andy Gospodarek Cc: Neil Horman Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 4190786de403..dfc82720065a 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -621,7 +621,7 @@ static int netconsole_netdev_event(struct notifier_block *this, bool stopped = false; if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER || - event == NETDEV_BONDING_DESLAVE || event == NETDEV_JOIN)) + event == NETDEV_RELEASE || event == NETDEV_JOIN)) goto done; spin_lock_irqsave(&target_list_lock, flags); @@ -632,7 +632,7 @@ static int netconsole_netdev_event(struct notifier_block *this, case NETDEV_CHANGENAME: strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ); break; - case NETDEV_BONDING_DESLAVE: + case NETDEV_RELEASE: case NETDEV_JOIN: case NETDEV_UNREGISTER: /* @@ -664,7 +664,7 @@ static int netconsole_netdev_event(struct notifier_block *this, case NETDEV_UNREGISTER: printk(KERN_CONT "unregistered\n"); break; - case NETDEV_BONDING_DESLAVE: + case NETDEV_RELEASE: printk(KERN_CONT "released slaves\n"); break; case NETDEV_JOIN: -- cgit v1.2.3