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 dfb67eb2a94..eb41e44921e 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-70-g09d2 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 eb41e44921e..62fdbaa1fb6 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-70-g09d2 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 ++++-------------- include/linux/if_ether.h | 1 + net/core/netpoll.c | 26 +----------------------- net/core/pktgen.c | 53 ++++++++---------------------------------------- net/core/utils.c | 24 ++++++++++++++++++++++ 5 files changed, 38 insertions(+), 86 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 62fdbaa1fb6..a83e101440f 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; } /* diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h index be69043d289..0f1325d9829 100644 --- a/include/linux/if_ether.h +++ b/include/linux/if_ether.h @@ -136,6 +136,7 @@ int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr); extern struct ctl_table ether_table[]; #endif +int mac_pton(const char *s, u8 *mac); extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len); #endif diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 46d9c3a4de2..2d7d6d47378 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -698,32 +698,8 @@ int netpoll_parse_options(struct netpoll *np, char *opt) if (*cur != 0) { /* MAC address */ - if ((delim = strchr(cur, ':')) == NULL) + if (!mac_pton(cur, np->remote_mac)) goto parse_failed; - *delim = 0; - np->remote_mac[0] = simple_strtol(cur, NULL, 16); - cur = delim + 1; - if ((delim = strchr(cur, ':')) == NULL) - goto parse_failed; - *delim = 0; - np->remote_mac[1] = simple_strtol(cur, NULL, 16); - cur = delim + 1; - if ((delim = strchr(cur, ':')) == NULL) - goto parse_failed; - *delim = 0; - np->remote_mac[2] = simple_strtol(cur, NULL, 16); - cur = delim + 1; - if ((delim = strchr(cur, ':')) == NULL) - goto parse_failed; - *delim = 0; - np->remote_mac[3] = simple_strtol(cur, NULL, 16); - cur = delim + 1; - if ((delim = strchr(cur, ':')) == NULL) - goto parse_failed; - *delim = 0; - np->remote_mac[4] = simple_strtol(cur, NULL, 16); - cur = delim + 1; - np->remote_mac[5] = simple_strtol(cur, NULL, 16); } netpoll_print_options(np); diff --git a/net/core/pktgen.c b/net/core/pktgen.c index d41d88b53e1..379270f1477 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1420,11 +1420,6 @@ static ssize_t pktgen_if_write(struct file *file, return count; } if (!strcmp(name, "dst_mac")) { - char *v = valstr; - unsigned char old_dmac[ETH_ALEN]; - unsigned char *m = pkt_dev->dst_mac; - memcpy(old_dmac, pkt_dev->dst_mac, ETH_ALEN); - len = strn_len(&user_buffer[i], sizeof(valstr) - 1); if (len < 0) return len; @@ -1432,35 +1427,16 @@ static ssize_t pktgen_if_write(struct file *file, memset(valstr, 0, sizeof(valstr)); if (copy_from_user(valstr, &user_buffer[i], len)) return -EFAULT; - i += len; - - for (*m = 0; *v && m < pkt_dev->dst_mac + 6; v++) { - int value; - - value = hex_to_bin(*v); - if (value >= 0) - *m = *m * 16 + value; - - if (*v == ':') { - m++; - *m = 0; - } - } + if (!mac_pton(valstr, pkt_dev->dst_mac)) + return -EINVAL; /* Set up Dest MAC */ - if (compare_ether_addr(old_dmac, pkt_dev->dst_mac)) - memcpy(&(pkt_dev->hh[0]), pkt_dev->dst_mac, ETH_ALEN); + memcpy(&pkt_dev->hh[0], pkt_dev->dst_mac, ETH_ALEN); - sprintf(pg_result, "OK: dstmac"); + sprintf(pg_result, "OK: dstmac %pM", pkt_dev->dst_mac); return count; } if (!strcmp(name, "src_mac")) { - char *v = valstr; - unsigned char old_smac[ETH_ALEN]; - unsigned char *m = pkt_dev->src_mac; - - memcpy(old_smac, pkt_dev->src_mac, ETH_ALEN); - len = strn_len(&user_buffer[i], sizeof(valstr) - 1); if (len < 0) return len; @@ -1468,26 +1444,13 @@ static ssize_t pktgen_if_write(struct file *file, memset(valstr, 0, sizeof(valstr)); if (copy_from_user(valstr, &user_buffer[i], len)) return -EFAULT; - i += len; - - for (*m = 0; *v && m < pkt_dev->src_mac + 6; v++) { - int value; - - value = hex_to_bin(*v); - if (value >= 0) - *m = *m * 16 + value; - - if (*v == ':') { - m++; - *m = 0; - } - } + if (!mac_pton(valstr, pkt_dev->src_mac)) + return -EINVAL; /* Set up Src MAC */ - if (compare_ether_addr(old_smac, pkt_dev->src_mac)) - memcpy(&(pkt_dev->hh[6]), pkt_dev->src_mac, ETH_ALEN); + memcpy(&pkt_dev->hh[6], pkt_dev->src_mac, ETH_ALEN); - sprintf(pg_result, "OK: srcmac"); + sprintf(pg_result, "OK: srcmac %pM", pkt_dev->src_mac); return count; } diff --git a/net/core/utils.c b/net/core/utils.c index 5fea0ab2190..2012bc797f9 100644 --- a/net/core/utils.c +++ b/net/core/utils.c @@ -296,3 +296,27 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb, csum_unfold(*sum))); } EXPORT_SYMBOL(inet_proto_csum_replace4); + +int mac_pton(const char *s, u8 *mac) +{ + int i; + + /* XX:XX:XX:XX:XX:XX */ + if (strlen(s) < 3 * ETH_ALEN - 1) + return 0; + + /* Don't dirty result unless string is valid MAC. */ + for (i = 0; i < ETH_ALEN; i++) { + if (!strchr("0123456789abcdefABCDEF", s[i * 3])) + return 0; + if (!strchr("0123456789abcdefABCDEF", s[i * 3 + 1])) + return 0; + if (i != ETH_ALEN - 1 && s[i * 3 + 2] != ':') + return 0; + } + for (i = 0; i < ETH_ALEN; i++) { + mac[i] = (hex_to_bin(s[i * 3]) << 4) | hex_to_bin(s[i * 3 + 1]); + } + return 1; +} +EXPORT_SYMBOL(mac_pton); -- cgit v1.2.3-70-g09d2 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/bonding/bond_main.c | 2 ++ drivers/net/netconsole.c | 26 +++++++++++++++++--------- include/linux/notifier.h | 1 + 3 files changed, 20 insertions(+), 9 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 088fd845ffd..f4960f516c3 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) } } + call_netdevice_notifiers(NETDEV_JOIN, slave_dev); + /* If this is the first slave, then we need to set the master's hardware * address to be the same as the slave's. */ if (is_zero_ether_addr(bond->dev->dev_addr)) diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index a83e101440f..4190786de40 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; diff --git a/include/linux/notifier.h b/include/linux/notifier.h index 621dfa16acc..a577762afbe 100644 --- a/include/linux/notifier.h +++ b/include/linux/notifier.h @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret) #define NETDEV_UNREGISTER_BATCH 0x0011 #define NETDEV_BONDING_DESLAVE 0x0012 #define NETDEV_NOTIFY_PEERS 0x0013 +#define NETDEV_JOIN 0x0014 #define SYS_DOWN 0x0001 /* Notify of system down */ #define SYS_RESTART SYS_DOWN -- cgit v1.2.3-70-g09d2 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/bonding/bond_main.c | 2 +- drivers/net/netconsole.c | 6 +++--- include/linux/notifier.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index f4960f516c3..6dc42846154 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1974,7 +1974,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) } block_netpoll_tx(); - netdev_bonding_change(bond_dev, NETDEV_BONDING_DESLAVE); + netdev_bonding_change(bond_dev, NETDEV_RELEASE); write_lock_bh(&bond->lock); slave = bond_get_slave_by_dev(bond, slave_dev); diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 4190786de40..dfc82720065 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: diff --git a/include/linux/notifier.h b/include/linux/notifier.h index a577762afbe..c0688b0168b 100644 --- a/include/linux/notifier.h +++ b/include/linux/notifier.h @@ -209,7 +209,7 @@ static inline int notifier_to_errno(int ret) #define NETDEV_POST_TYPE_CHANGE 0x000F #define NETDEV_POST_INIT 0x0010 #define NETDEV_UNREGISTER_BATCH 0x0011 -#define NETDEV_BONDING_DESLAVE 0x0012 +#define NETDEV_RELEASE 0x0012 #define NETDEV_NOTIFY_PEERS 0x0013 #define NETDEV_JOIN 0x0014 -- cgit v1.2.3-70-g09d2