From 69cc64d8d92bf852f933e90c888dfff083bd4fc9 Mon Sep 17 00:00:00 2001 From: "David S. Miller" Date: Mon, 11 Feb 2008 21:45:44 -0800 Subject: [NDISC]: Fix race in generic address resolution Frank Blaschka provided the bug report and the initial suggested fix for this bug. He also validated this version of this fix. The problem is that the access to neigh->arp_queue is inconsistent, we grab references when dropping the lock lock to call neigh->ops->solicit() but this does not prevent other threads of control from trying to send out that packet at the same time causing corruptions because both code paths believe they have exclusive access to the skb. The best option seems to be to hold the write lock on neigh->lock during the ->solicit() call. I looked at all of the ndisc_ops implementations and this seems workable. The only case that needs special care is the IPV4 ARP implementation of arp_solicit(). It wants to take neigh->lock as a reader to protect the header entry in neigh->ha during the emission of the soliciation. We can simply remove the read lock calls to take care of that since holding the lock as a writer at the caller providers a superset of the protection afforded by the existing read locking. The rest of the ->solicit() implementations don't care whether the neigh is locked or not. Signed-off-by: David S. Miller --- net/core/neighbour.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'net/core') diff --git a/net/core/neighbour.c b/net/core/neighbour.c index a16cf1ec5e5..7bb6a9a1256 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -834,18 +834,12 @@ static void neigh_timer_handler(unsigned long arg) } if (neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) { struct sk_buff *skb = skb_peek(&neigh->arp_queue); - /* keep skb alive even if arp_queue overflows */ - if (skb) - skb_get(skb); - write_unlock(&neigh->lock); + neigh->ops->solicit(neigh, skb); atomic_inc(&neigh->probes); - if (skb) - kfree_skb(skb); - } else { -out: - write_unlock(&neigh->lock); } +out: + write_unlock(&neigh->lock); if (notify) neigh_update_notify(neigh); -- cgit v1.2.3 From fee54fa517bef1de2c10a1a3e918228cc59dce90 Mon Sep 17 00:00:00 2001 From: Urs Thuermann Date: Tue, 12 Feb 2008 22:03:25 -0800 Subject: [NET]: Fix comment for skb_pull_rcsum Fix comment for skb_pull_rcsum Signed-off-by: Urs Thuermann Signed-off-by: David S. Miller --- net/core/skbuff.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'net/core') diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 4e354221ec2..40dddcc6dc3 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2106,11 +2106,10 @@ int skb_append_datato_frags(struct sock *sk, struct sk_buff *skb, /** * skb_pull_rcsum - pull skb and update receive checksum * @skb: buffer to update - * @start: start of data before pull * @len: length of data pulled * * This function performs an skb_pull on the packet and updates - * update the CHECKSUM_COMPLETE checksum. It should be used on + * the CHECKSUM_COMPLETE checksum. It should be used on * receive path processing instead of skb_pull unless you know * that the checksum difference is zero (e.g., a valid IP header) * or you are setting ip_summed to CHECKSUM_NONE. -- cgit v1.2.3 From 45b503548210fe6f23e92b856421c2a3f05fd034 Mon Sep 17 00:00:00 2001 From: Laszlo Attila Toth Date: Tue, 12 Feb 2008 22:42:09 -0800 Subject: [RTNETLINK]: Send a single notification on device state changes. In do_setlink() a single notification is sent at the end of the function if any modification occured. If the address has been changed, another notification is sent. Both of them is required because originally only the NETDEV_CHANGEADDR notification was sent and although device state change implies address change, some programs may expect the original notification. It remains for compatibity. If set_operstate() is called from do_setlink(), it doesn't send a notification, only if it is called from rtnl_create_link() as earlier. Signed-off-by: Laszlo Attila Toth Signed-off-by: David S. Miller --- net/core/rtnetlink.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) (limited to 'net/core') diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 61ac8d06292..ecb02afd52d 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -504,7 +504,7 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id, EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo); -static void set_operstate(struct net_device *dev, unsigned char transition) +static int set_operstate(struct net_device *dev, unsigned char transition, bool send_notification) { unsigned char operstate = dev->operstate; @@ -527,8 +527,12 @@ static void set_operstate(struct net_device *dev, unsigned char transition) write_lock_bh(&dev_base_lock); dev->operstate = operstate; write_unlock_bh(&dev_base_lock); - netdev_state_change(dev); - } + + if (send_notification) + netdev_state_change(dev); + return 1; + } else + return 0; } static void copy_rtnl_link_stats(struct rtnl_link_stats *a, @@ -822,6 +826,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, if (tb[IFLA_BROADCAST]) { nla_memcpy(dev->broadcast, tb[IFLA_BROADCAST], dev->addr_len); send_addr_notify = 1; + modified = 1; } if (ifm->ifi_flags || ifm->ifi_change) { @@ -834,16 +839,23 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, dev_change_flags(dev, flags); } - if (tb[IFLA_TXQLEN]) - dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]); + if (tb[IFLA_TXQLEN]) { + if (dev->tx_queue_len != nla_get_u32(tb[IFLA_TXQLEN])) { + dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]); + modified = 1; + } + } if (tb[IFLA_OPERSTATE]) - set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE])); + modified |= set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]), false); if (tb[IFLA_LINKMODE]) { - write_lock_bh(&dev_base_lock); - dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]); - write_unlock_bh(&dev_base_lock); + if (dev->link_mode != nla_get_u8(tb[IFLA_LINKMODE])) { + write_lock_bh(&dev_base_lock); + dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]); + write_lock_bh(&dev_base_lock); + modified = 1; + } } err = 0; @@ -857,6 +869,10 @@ errout: if (send_addr_notify) call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); + + if (modified) + netdev_state_change(dev); + return err; } @@ -974,7 +990,7 @@ struct net_device *rtnl_create_link(struct net *net, char *ifname, if (tb[IFLA_TXQLEN]) dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]); if (tb[IFLA_OPERSTATE]) - set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE])); + set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]), true); if (tb[IFLA_LINKMODE]) dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]); -- cgit v1.2.3 From d8b2a4d21e0b37b9669b202867bfef19f68f786a Mon Sep 17 00:00:00 2001 From: Matti Linnanvuori Date: Tue, 12 Feb 2008 23:10:11 -0800 Subject: [NET]: Fix race in dev_close(). (Bug 9750) There is a race in Linux kernel file net/core/dev.c, function dev_close. The function calls function dev_deactivate, which calls function dev_watchdog_down that deletes the watchdog timer. However, after that, a driver can call netif_carrier_ok, which calls function __netdev_watchdog_up that can add the watchdog timer again. Function unregister_netdevice calls function dev_shutdown that traps the bug !timer_pending(&dev->watchdog_timer). Moving dev_deactivate after netif_running() has been cleared prevents function netif_carrier_on from calling __netdev_watchdog_up and adding the watchdog timer again. Signed-off-by: Matti Linnanvuori Signed-off-by: David S. Miller --- net/core/dev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/core') diff --git a/net/core/dev.c b/net/core/dev.c index 9549417250b..6cfc1238c4a 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1071,8 +1071,6 @@ int dev_close(struct net_device *dev) */ call_netdevice_notifiers(NETDEV_GOING_DOWN, dev); - dev_deactivate(dev); - clear_bit(__LINK_STATE_START, &dev->state); /* Synchronize to scheduled poll. We cannot touch poll list, @@ -1083,6 +1081,8 @@ int dev_close(struct net_device *dev) */ smp_mb__after_clear_bit(); /* Commit netif_running(). */ + dev_deactivate(dev); + /* * Call the device specific close. This cannot fail. * Only if device is UP -- cgit v1.2.3