Skip to contentSkip to conversation

net: Fix IP_MULTICAST_IF

PATCH In review Eric Dumazet 3 participants 14 messages Oct 19, 2009 – Oct 29, 2009

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 net: Fix IP_MULTICAST_IF

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 <[email protected]>
---
 net/ipv4/ip_sockglue.c   |    7 +++----
 net/ipv6/ipv6_sockglue.c |    6 +++++-
 2 files changed, 8 insertions(+), 5 deletions(-)
net/ipv4/ip_sockglue.c +3 -4
index 0c0b6e3..e982b5c 100644
@@ -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 &&
net/ipv6/ipv6_sockglue.c +5 -1
index 14f54eb..4f7aaf6 100644
@@ -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;
maintainer
#1 lore raw

From: Eric Dumazet <[email protected]>
Date: Mon, 19 Oct 2009 18:41:58 +0200

Quoted 1 line

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.

author
#2 lore raw
Quoted 7 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Mon, 19 Oct 2009 18:41:58 +0200

Quoted 1 line

ipv4/ipv6 setsockopt(IP_MULTICAST_IF) have dubious __dev_get_by_index() calls.

Dubious, how so?

Quoted 7 lines

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 ?

author
#3 lore raw
Quoted 13 lines · Eric Dumazet
Quoted 12 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Mon, 19 Oct 2009 18:41:58 +0200

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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;
}
maintainer
#4 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:16:02 +0200

Quoted 3 lines

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!

maintainer
#5 lore raw
In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:07:48 +0200

Quoted 2 lines

Final target might be fine, but an element in the chain, before target could be deleted while reader scans hash chain.

...

Quoted 1 line

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 :-)

author
#6 lore raw
Quoted 6 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:07:48 +0200

Quoted 2 lines

Final target might be fine, but an element in the chain, before target could be deleted while reader scans hash chain.

...

Quoted 8 lines
Quoted 1 line

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...

maintainer
#7 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:23:54 +0200

Quoted 2 lines

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 :-)

author
#8 lore raw

PATCH 1/1 net: Introduce dev_get_by_index_rcu()

Quoted 7 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:23:54 +0200

Quoted 2 lines

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 <[email protected]>
---
 include/linux/netdevice.h |    1
 net/core/dev.c            |   40 ++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 6 deletions(-)
include/linux/netdevice.h +1 -0
index 8380009..4eda680 100644
@@ -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);
net/core/dev.c +34 -6
index 28b0b9e..cb011b7 100644
@@ -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);
#9 lore raw

Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu()

Quoted 11 lines · On Tue, 20 Oct 2009 07:03:44 +0200 Eric Dumazet <[email protected]> wrote:
Quoted 7 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:23:54 +0200

Quoted 2 lines

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()

Quoted 7 lines

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 <[email protected]>

All usage dev_base_lock should be replaceable by using combination of rtnl_mutex and RCU?

author
#10 lore raw

Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu()

Quoted 15 lines · Stephen Hemminger
Quoted 12 lines · On Tue, 20 Oct 2009 07:03:44 +0200 Eric Dumazet <[email protected]> wrote:
Quoted 6 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:23:54 +0200

Quoted 2 lines

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.

Quoted 7 lines
Quoted 4 lines

netdevices are already freed after a RCU grace period, so this patch adds no penalty at device dismantle time.

Signed-off-by Eric Dumazet <[email protected]>

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 <[email protected]>
---
 include/linux/netdevice.h |    1
 net/core/dev.c            |   48 ++++++++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 10 deletions(-)
include/linux/netdevice.h +1 -0
index 8380009..4eda680 100644
@@ -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);
net/core/dev.c +38 -10

89 lines not on this page · raw message

maintainer Applied
#11 lore raw

Re: [PATCH net-next-2.6] net: Introduce dev_get_by_index_rcu()

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 07:18:49 +0200

Quoted 11 lines

[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 <[email protected]>

Applied to net-next-2.6, thanks!

author
#12 lore raw

PATCH 1/1 ifb: should not use __dev_get_by_index() without locks

In reply to David Miller
Quoted 7 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 06:23:54 +0200

Quoted 2 lines

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 <[email protected]>

---

drivers/net/ifb.c +2 -1
index 801f088..030913f 100644
@@ -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) {
maintainer Applied
#13 lore raw

Re: [PATCH] ifb: should not use __dev_get_by_index() without locks

From: Eric Dumazet <[email protected]>
Date: Tue, 20 Oct 2009 14:35:50 +0200

Quoted 6 lines

[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 <[email protected]>

Applied, thanks.