* [PATCH] net: Fix IP_MULTICAST_IF @ 2009-10-19 16:41 Eric Dumazet 2009-10-20 3:59 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-19 16:41 UTC (permalink / raw) To: David S. Miller; +Cc: Linux Netdev List ipv4/ipv6 setsockopt(IP_MULTICAST_IF) have dubious __dev_get_by_index() calls. This function should be called only with RTNL or dev_base_lock held, or reader could see a corrupt hash chain and eventually enter an endless loop. Fix is to call dev_get_by_index()/dev_put(). If this happens to be performance critical, we could define a new dev_exist_by_index() function to avoid touching dev refcount. Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- net/ipv4/ip_sockglue.c | 7 +++---- net/ipv6/ipv6_sockglue.c | 6 +++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c index 0c0b6e3..e982b5c 100644 --- a/net/ipv4/ip_sockglue.c +++ b/net/ipv4/ip_sockglue.c @@ -634,17 +634,16 @@ static int do_ip_setsockopt(struct sock *sk, int level, break; } dev = ip_dev_find(sock_net(sk), mreq.imr_address.s_addr); - if (dev) { + if (dev) mreq.imr_ifindex = dev->ifindex; - dev_put(dev); - } } else - dev = __dev_get_by_index(sock_net(sk), mreq.imr_ifindex); + dev = dev_get_by_index(sock_net(sk), mreq.imr_ifindex); err = -EADDRNOTAVAIL; if (!dev) break; + dev_put(dev); err = -EINVAL; if (sk->sk_bound_dev_if && diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c index 14f54eb..4f7aaf6 100644 --- a/net/ipv6/ipv6_sockglue.c +++ b/net/ipv6/ipv6_sockglue.c @@ -496,13 +496,17 @@ done: goto e_inval; if (val) { + struct net_device *dev; + if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != val) goto e_inval; - if (__dev_get_by_index(net, val) == NULL) { + dev = dev_get_by_index(net, val); + if (!dev) { retv = -ENODEV; break; } + dev_put(dev); } np->mcast_oif = val; retv = 0; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-19 16:41 [PATCH] net: Fix IP_MULTICAST_IF Eric Dumazet @ 2009-10-20 3:59 ` David Miller 2009-10-20 4:07 ` Eric Dumazet 0 siblings, 1 reply; 14+ messages in thread From: David Miller @ 2009-10-20 3:59 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 19 Oct 2009 18:41:58 +0200 > ipv4/ipv6 setsockopt(IP_MULTICAST_IF) have dubious __dev_get_by_index() calls. Dubious, how so? Yes, I know RTNL/dev_base_lock, but it's not using what it gets back at all. It's testing existence, a boolean, it doesn't dereference the 'dev' it gets back at all. This code is intentional and perfectly fine. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 3:59 ` David Miller @ 2009-10-20 4:07 ` Eric Dumazet 2009-10-20 4:16 ` Eric Dumazet 2009-10-20 4:20 ` David Miller 0 siblings, 2 replies; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 4:07 UTC (permalink / raw) To: David Miller; +Cc: netdev David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 19 Oct 2009 18:41:58 +0200 > >> ipv4/ipv6 setsockopt(IP_MULTICAST_IF) have dubious __dev_get_by_index() calls. > > Dubious, how so? > > Yes, I know RTNL/dev_base_lock, but it's not using what it gets > back at all. > > It's testing existence, a boolean, it doesn't dereference the > 'dev' it gets back at all. > > This code is intentional and perfectly fine. If this was intentional, something changed and made the prereq false. Final target might be fine, but an element in the chain, before target could be deleted while reader scans hash chain. /* Device list removal */ static void unlist_netdevice(struct net_device *dev) { ASSERT_RTNL(); /* Unlink dev from the device chain */ write_lock_bh(&dev_base_lock); list_del(&dev->dev_list); hlist_del(&dev->name_hlist); hlist_del(&dev->index_hlist); <<< HERE >>> write_unlock_bh(&dev_base_lock); } static inline void hlist_del(struct hlist_node *n) { __hlist_del(n); n->next = LIST_POISON1; <<< HERE >>> n->pprev = LIST_POISON2; } include/linux/poison.h:#define LIST_POISON1 ((void *) 0x00100100) reader tries to pass over this delete net_device, doing a dev->index_hlist->next #define hlist_for_each(pos, head) \ for (pos = (head)->first; pos && ({ prefetch(pos->next); 1; }); \ pos = pos->next) So it should visit a nice memory location ? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 4:07 ` Eric Dumazet @ 2009-10-20 4:16 ` Eric Dumazet 2009-10-20 4:21 ` David Miller 2009-10-20 4:20 ` David Miller 1 sibling, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 4:16 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev Eric Dumazet a écrit : > David Miller a écrit : >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Mon, 19 Oct 2009 18:41:58 +0200 >> >>> ipv4/ipv6 setsockopt(IP_MULTICAST_IF) have dubious __dev_get_by_index() calls. >> Dubious, how so? >> >> Yes, I know RTNL/dev_base_lock, but it's not using what it gets >> back at all. >> >> It's testing existence, a boolean, it doesn't dereference the >> 'dev' it gets back at all. >> >> This code is intentional and perfectly fine. > > If this was intentional, something changed and made the prereq false. > > Final target might be fine, but an element in the chain, before target > could be deleted while reader scans hash chain. > BTW, even an insertion can crash a lockless reader, since reader could see a corrupt n->next (hlist_add_head() has no barrier between n->next = first and h->first = n;) static inline void hlist_add_head(struct hlist_node *n, struct hlist_head *h) { struct hlist_node *first = h->first; n->next = first; if (first) first->pprev = &n->next; h->first = n; n->pprev = &h->first; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 4:16 ` Eric Dumazet @ 2009-10-20 4:21 ` David Miller 0 siblings, 0 replies; 14+ messages in thread From: David Miller @ 2009-10-20 4:21 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Oct 2009 06:16:02 +0200 > BTW, even an insertion can crash a lockless reader, since reader > could see a corrupt n->next (hlist_add_head() has no barrier > between n->next = first and h->first = n;) Ok, now that convinces it for me, I'll apply your patch, thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 4:07 ` Eric Dumazet 2009-10-20 4:16 ` Eric Dumazet @ 2009-10-20 4:20 ` David Miller 2009-10-20 4:23 ` Eric Dumazet 1 sibling, 1 reply; 14+ messages in thread From: David Miller @ 2009-10-20 4:20 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Oct 2009 06:07:48 +0200 > Final target might be fine, but an element in the chain, before target > could be deleted while reader scans hash chain. ... > So it should visit a nice memory location ? It should hit a NULL eventually and deterministically even if an unlink happens at the same time..... unless the object gets free'd meanwhile, hmmm... This code is definitely intentional, I remember when I added it to the tree, Alexey wrote it :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 4:20 ` David Miller @ 2009-10-20 4:23 ` Eric Dumazet 2009-10-20 4:28 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 4:23 UTC (permalink / raw) To: David Miller; +Cc: netdev David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 20 Oct 2009 06:07:48 +0200 > >> Final target might be fine, but an element in the chain, before target >> could be deleted while reader scans hash chain. > ... >> So it should visit a nice memory location ? > > It should hit a NULL eventually and deterministically even if an > unlink happens at the same time..... unless the object gets free'd > meanwhile, hmmm... > > This code is definitely intentional, I remember when I added it to > the tree, Alexey wrote it :-) I wonder if the whole thing could use RCU somehow, since some workloads hit this dev_base_lock rwlock pretty hard... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: Fix IP_MULTICAST_IF 2009-10-20 4:23 ` Eric Dumazet @ 2009-10-20 4:28 ` David Miller 2009-10-20 5:03 ` [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Eric Dumazet 2009-10-20 12:35 ` [PATCH] ifb: should not use __dev_get_by_index() without locks Eric Dumazet 0 siblings, 2 replies; 14+ messages in thread From: David Miller @ 2009-10-20 4:28 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Oct 2009 06:23:54 +0200 > I wonder if the whole thing could use RCU somehow, since some > workloads hit this dev_base_lock rwlock pretty hard... True, but for now we'll put your fix in :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() 2009-10-20 4:28 ` David Miller @ 2009-10-20 5:03 ` Eric Dumazet 2009-10-20 5:06 ` Stephen Hemminger 2009-10-20 12:35 ` [PATCH] ifb: should not use __dev_get_by_index() without locks Eric Dumazet 1 sibling, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 5:03 UTC (permalink / raw) To: David Miller; +Cc: netdev David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 20 Oct 2009 06:23:54 +0200 > >> I wonder if the whole thing could use RCU somehow, since some >> workloads hit this dev_base_lock rwlock pretty hard... > > True, but for now we'll put your fix in :-) [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Some workloads hit dev_base_lock rwlock pretty hard. We can use RCU lookups to avoid touching this rwlock. netdevices are already freed after a RCU grace period, so this patch adds no penalty at device dismantle time. Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- include/linux/netdevice.h | 1 net/core/dev.c | 40 ++++++++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 8380009..4eda680 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1127,6 +1127,7 @@ extern void netdev_resync_ops(struct net_device *dev); extern int call_netdevice_notifiers(unsigned long val, struct net_device *dev); extern struct net_device *dev_get_by_index(struct net *net, int ifindex); extern struct net_device *__dev_get_by_index(struct net *net, int ifindex); +extern struct net_device *dev_get_by_index_rcu(struct net *net, int ifindex); extern int dev_restart(struct net_device *dev); #ifdef CONFIG_NETPOLL_TRAP extern int netpoll_trap(void); diff --git a/net/core/dev.c b/net/core/dev.c index 28b0b9e..cb011b7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -217,12 +217,15 @@ static int list_netdevice(struct net_device *dev) write_lock_bh(&dev_base_lock); list_add_tail(&dev->dev_list, &net->dev_base_head); hlist_add_head(&dev->name_hlist, dev_name_hash(net, dev->name)); - hlist_add_head(&dev->index_hlist, dev_index_hash(net, dev->ifindex)); + hlist_add_head_rcu(&dev->index_hlist, + dev_index_hash(net, dev->ifindex)); write_unlock_bh(&dev_base_lock); return 0; } -/* Device list removal */ +/* Device list removal + * caller must respect a RCU grace period before freeing/reusing dev + */ static void unlist_netdevice(struct net_device *dev) { ASSERT_RTNL(); @@ -231,7 +234,7 @@ static void unlist_netdevice(struct net_device *dev) write_lock_bh(&dev_base_lock); list_del(&dev->dev_list); hlist_del(&dev->name_hlist); - hlist_del(&dev->index_hlist); + hlist_del_rcu(&dev->index_hlist); write_unlock_bh(&dev_base_lock); } @@ -649,6 +652,31 @@ struct net_device *__dev_get_by_index(struct net *net, int ifindex) } EXPORT_SYMBOL(__dev_get_by_index); +/** + * dev_get_by_index_rcu - find a device by its ifindex + * @net: the applicable net namespace + * @ifindex: index of device + * + * Search for an interface by index. Returns %NULL if the device + * is not found or a pointer to the device. The device has not + * had its reference counter increased so the caller must be careful + * about locking. The caller must hold RCU lock. + */ + +struct net_device *dev_get_by_index_rcu(struct net *net, int ifindex) +{ + struct hlist_node *p; + struct net_device *dev; + struct hlist_head *head = dev_index_hash(net, ifindex); + + hlist_for_each_entry_rcu(dev, p, head, index_hlist) + if (dev->ifindex == ifindex) + return dev; + + return NULL; +} +EXPORT_SYMBOL(dev_get_by_index_rcu); + /** * dev_get_by_index - find a device by its ifindex @@ -665,11 +693,11 @@ struct net_device *dev_get_by_index(struct net *net, int ifindex) { struct net_device *dev; - read_lock(&dev_base_lock); - dev = __dev_get_by_index(net, ifindex); + rcu_read_lock(); + dev = dev_get_by_index_rcu(net, ifindex); if (dev) dev_hold(dev); - read_unlock(&dev_base_lock); + rcu_read_unlock(); return dev; } EXPORT_SYMBOL(dev_get_by_index); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() 2009-10-20 5:03 ` [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Eric Dumazet @ 2009-10-20 5:06 ` Stephen Hemminger 2009-10-20 5:18 ` Eric Dumazet 0 siblings, 1 reply; 14+ messages in thread From: Stephen Hemminger @ 2009-10-20 5:06 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev On Tue, 20 Oct 2009 07:03:44 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote: > David Miller a écrit : > > From: Eric Dumazet <eric.dumazet@gmail.com> > > Date: Tue, 20 Oct 2009 06:23:54 +0200 > > > >> I wonder if the whole thing could use RCU somehow, since some > >> workloads hit this dev_base_lock rwlock pretty hard... > > > > True, but for now we'll put your fix in :-) > > [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() > > Some workloads hit dev_base_lock rwlock pretty hard. > We can use RCU lookups to avoid touching this rwlock. > > netdevices are already freed after a RCU grace period, so this patch > adds no penalty at device dismantle time. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> All usage dev_base_lock should be replaceable by using combination of rtnl_mutex and RCU? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() 2009-10-20 5:06 ` Stephen Hemminger @ 2009-10-20 5:18 ` Eric Dumazet 2009-10-29 8:43 ` David Miller 0 siblings, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 5:18 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, netdev Stephen Hemminger a écrit : > On Tue, 20 Oct 2009 07:03:44 +0200 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > >> David Miller a écrit : >>> From: Eric Dumazet <eric.dumazet@gmail.com> >>> Date: Tue, 20 Oct 2009 06:23:54 +0200 >>> >>>> I wonder if the whole thing could use RCU somehow, since some >>>> workloads hit this dev_base_lock rwlock pretty hard... >>> True, but for now we'll put your fix in :-) >> [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() >> >> Some workloads hit dev_base_lock rwlock pretty hard. >> We can use RCU lookups to avoid touching this rwlock. >> >> netdevices are already freed after a RCU grace period, so this patch >> adds no penalty at device dismantle time. >> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > All usage dev_base_lock should be replaceable by using combination of rtnl_mutex > and RCU? Yes probably, but I believe we should make step-by-step patches ? 1) __dev_get_by_index() is faster than dev_get_by_index_rcu() 2) I am not sure holding RTNL means we also have rcu_lock() implied. However dev_ifname() could use rcu_lock() in the same patch, here is an updated version. [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Some workloads hit dev_base_lock rwlock pretty hard. We can use RCU lookups to avoid touching this rwlock. netdevices are already freed after a RCU grace period, so this patch adds no penalty at device dismantle time. dev_ifname() converted to dev_get_by_index_rcu() Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- include/linux/netdevice.h | 1 net/core/dev.c | 48 ++++++++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 8380009..4eda680 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1127,6 +1127,7 @@ extern void netdev_resync_ops(struct net_device *dev); extern int call_netdevice_notifiers(unsigned long val, struct net_device *dev); extern struct net_device *dev_get_by_index(struct net *net, int ifindex); extern struct net_device *__dev_get_by_index(struct net *net, int ifindex); +extern struct net_device *dev_get_by_index_rcu(struct net *net, int ifindex); extern int dev_restart(struct net_device *dev); #ifdef CONFIG_NETPOLL_TRAP extern int netpoll_trap(void); diff --git a/net/core/dev.c b/net/core/dev.c index 28b0b9e..4564596 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -217,12 +217,15 @@ static int list_netdevice(struct net_device *dev) write_lock_bh(&dev_base_lock); list_add_tail(&dev->dev_list, &net->dev_base_head); hlist_add_head(&dev->name_hlist, dev_name_hash(net, dev->name)); - hlist_add_head(&dev->index_hlist, dev_index_hash(net, dev->ifindex)); + hlist_add_head_rcu(&dev->index_hlist, + dev_index_hash(net, dev->ifindex)); write_unlock_bh(&dev_base_lock); return 0; } -/* Device list removal */ +/* Device list removal + * caller must respect a RCU grace period before freeing/reusing dev + */ static void unlist_netdevice(struct net_device *dev) { ASSERT_RTNL(); @@ -231,7 +234,7 @@ static void unlist_netdevice(struct net_device *dev) write_lock_bh(&dev_base_lock); list_del(&dev->dev_list); hlist_del(&dev->name_hlist); - hlist_del(&dev->index_hlist); + hlist_del_rcu(&dev->index_hlist); write_unlock_bh(&dev_base_lock); } @@ -649,6 +652,31 @@ struct net_device *__dev_get_by_index(struct net *net, int ifindex) } EXPORT_SYMBOL(__dev_get_by_index); +/** + * dev_get_by_index_rcu - find a device by its ifindex + * @net: the applicable net namespace + * @ifindex: index of device + * + * Search for an interface by index. Returns %NULL if the device + * is not found or a pointer to the device. The device has not + * had its reference counter increased so the caller must be careful + * about locking. The caller must hold RCU lock. + */ + +struct net_device *dev_get_by_index_rcu(struct net *net, int ifindex) +{ + struct hlist_node *p; + struct net_device *dev; + struct hlist_head *head = dev_index_hash(net, ifindex); + + hlist_for_each_entry_rcu(dev, p, head, index_hlist) + if (dev->ifindex == ifindex) + return dev; + + return NULL; +} +EXPORT_SYMBOL(dev_get_by_index_rcu); + /** * dev_get_by_index - find a device by its ifindex @@ -665,11 +693,11 @@ struct net_device *dev_get_by_index(struct net *net, int ifindex) { struct net_device *dev; - read_lock(&dev_base_lock); - dev = __dev_get_by_index(net, ifindex); + rcu_read_lock(); + dev = dev_get_by_index_rcu(net, ifindex); if (dev) dev_hold(dev); - read_unlock(&dev_base_lock); + rcu_read_unlock(); return dev; } EXPORT_SYMBOL(dev_get_by_index); @@ -2930,15 +2958,15 @@ static int dev_ifname(struct net *net, struct ifreq __user *arg) if (copy_from_user(&ifr, arg, sizeof(struct ifreq))) return -EFAULT; - read_lock(&dev_base_lock); - dev = __dev_get_by_index(net, ifr.ifr_ifindex); + rcu_read_lock(); + dev = dev_get_by_index_rcu(net, ifr.ifr_ifindex); if (!dev) { - read_unlock(&dev_base_lock); + rcu_read_unlock(); return -ENODEV; } strcpy(ifr.ifr_name, dev->name); - read_unlock(&dev_base_lock); + rcu_read_unlock(); if (copy_to_user(arg, &ifr, sizeof(struct ifreq))) return -EFAULT; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() 2009-10-20 5:18 ` Eric Dumazet @ 2009-10-29 8:43 ` David Miller 0 siblings, 0 replies; 14+ messages in thread From: David Miller @ 2009-10-29 8:43 UTC (permalink / raw) To: eric.dumazet; +Cc: shemminger, netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Oct 2009 07:18:49 +0200 > [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() > > Some workloads hit dev_base_lock rwlock pretty hard. > We can use RCU lookups to avoid touching this rwlock. > > netdevices are already freed after a RCU grace period, so this patch > adds no penalty at device dismantle time. > > dev_ifname() converted to dev_get_by_index_rcu() > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied to net-next-2.6, thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ifb: should not use __dev_get_by_index() without locks 2009-10-20 4:28 ` David Miller 2009-10-20 5:03 ` [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Eric Dumazet @ 2009-10-20 12:35 ` Eric Dumazet 2009-10-23 4:54 ` David Miller 1 sibling, 1 reply; 14+ messages in thread From: Eric Dumazet @ 2009-10-20 12:35 UTC (permalink / raw) To: David Miller; +Cc: netdev David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 20 Oct 2009 06:23:54 +0200 > >> I wonder if the whole thing could use RCU somehow, since some >> workloads hit this dev_base_lock rwlock pretty hard... > > True, but for now we'll put your fix in :-) Here is another vulnerable point, needing following patch. Thanks [PATCH] ifb: should not use __dev_get_by_index() without locks At this point (ri_tasklet()), RTNL or dev_base_lock are not held, we must use dev_get_by_index() instead of __dev_get_by_index() Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index 801f088..030913f 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -98,12 +98,13 @@ static void ri_tasklet(unsigned long dev) stats->tx_packets++; stats->tx_bytes +=skb->len; - skb->dev = __dev_get_by_index(&init_net, skb->iif); + skb->dev = dev_get_by_index(&init_net, skb->iif); if (!skb->dev) { dev_kfree_skb(skb); stats->tx_dropped++; break; } + dev_put(skb->dev); skb->iif = _dev->ifindex; if (from & AT_EGRESS) { ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] ifb: should not use __dev_get_by_index() without locks 2009-10-20 12:35 ` [PATCH] ifb: should not use __dev_get_by_index() without locks Eric Dumazet @ 2009-10-23 4:54 ` David Miller 0 siblings, 0 replies; 14+ messages in thread From: David Miller @ 2009-10-23 4:54 UTC (permalink / raw) To: eric.dumazet; +Cc: netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Oct 2009 14:35:50 +0200 > [PATCH] ifb: should not use __dev_get_by_index() without locks > > At this point (ri_tasklet()), RTNL or dev_base_lock are not held, > we must use dev_get_by_index() instead of __dev_get_by_index() > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-10-29 8:42 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-10-19 16:41 [PATCH] net: Fix IP_MULTICAST_IF Eric Dumazet 2009-10-20 3:59 ` David Miller 2009-10-20 4:07 ` Eric Dumazet 2009-10-20 4:16 ` Eric Dumazet 2009-10-20 4:21 ` David Miller 2009-10-20 4:20 ` David Miller 2009-10-20 4:23 ` Eric Dumazet 2009-10-20 4:28 ` David Miller 2009-10-20 5:03 ` [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu() Eric Dumazet 2009-10-20 5:06 ` Stephen Hemminger 2009-10-20 5:18 ` Eric Dumazet 2009-10-29 8:43 ` David Miller 2009-10-20 12:35 ` [PATCH] ifb: should not use __dev_get_by_index() without locks Eric Dumazet 2009-10-23 4:54 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).