igmp: fix ip_mc_sf_allow race

Almost all igmp functions accessing inet->mc_list are protected by rtnl_lock(), but there is one exception which is ip_mc_sf_allow(), so there is a chance of either ip_mc_drop_socket or ip_mc_leave_group remove an entry while ip_mc_sf_allow is running causing a crash.

Signed-off-by Flavio Leitner <[email protected]>

From: Flavio Leitner <[email protected]>
Date: Wed, 30 Dec 2009 12:23:25 -0200

Quoted 5 lines

Almost all igmp functions accessing inet->mc_list are protected by rtnl_lock(), but there is one exception which is ip_mc_sf_allow(), so there is a chance of either ip_mc_drop_socket or ip_mc_leave_group remove an entry while ip_mc_sf_allow is running causing a crash.

Quoted 1 line

Signed-off-by Flavio Leitner <[email protected]>

Have you triggered this in practice or is this due purely to code inspection?

That new synchronize_rcu() is very expensive and will decrease the rate at which groups can be joined and left, _especially_ on high cpu count machines.

I do not think it is therefore a suitable problem to this race, if it does in fact exist.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Then, I tried using call_rcu() to avoid the problem you are saying, but when you stop the reproducer, sk_free() will warn printing "optmem leakage.." because the rcu callback didn't run yet.

Quoted 5 lines · Flavio Leitner

Then, I tried using call_rcu() to avoid the problem you are saying, but when you stop the reproducer, sk_free() will warn printing "optmem leakage.." because the rcu callback didn't run yet.

This is probably because your call_rcu() callback was trying to call sock_kfree_s() ?

 		rtnl_unlock();
		call_rcu(&iml->lock, callback_func)

callback_func()

{
	sock_kfree_s(sk, iml, sizeof(*iml));
}

Take a look at sock_kfree_s() definition :

void sock_kfree_s(struct sock *sk, void *mem, int size)

{
        kfree(mem);
        atomic_sub(size, &sk->sk_omem_alloc);
}

You can certainly try :

	rtnl_unlock();
	atomic_sub(sizeof(*iml), sk->sk_omem_alloc);
	call_rcu(&iml->rcu, kfree);

(immediate sk_omem_alloc handling, but deferred kfree())

Quoted 9 lines · Eric Dumazet

Quoted 5 lines · Flavio Leitner

Then, I tried using call_rcu() to avoid the problem you are saying, but when you stop the reproducer, sk_free() will warn printing "optmem leakage.." because the rcu callback didn't run yet.

This is probably because your call_rcu() callback was trying to call sock_kfree_s() ?

yes, correct.

Quoted 26 lines

 		rtnl_unlock();
		call_rcu(&iml->lock, callback_func)

callback_func()

{
	sock_kfree_s(sk, iml, sizeof(*iml));
}

Take a look at sock_kfree_s() definition :

void sock_kfree_s(struct sock *sk, void *mem, int size)

{
        kfree(mem);
        atomic_sub(size, &sk->sk_omem_alloc);
}

You can certainly try :

	rtnl_unlock();
	atomic_sub(sizeof(*iml), sk->sk_omem_alloc);
	call_rcu(&iml->rcu, kfree);

Quoted 1 line

(immediate sk_omem_alloc handling, but deferred kfree())

Ok, below is the new version using call_rcu(). I'm still running my tests here, so I'm planning to resubmit it later if this version is okay with you.

For readability, instead of:

        ret = X;
        if (condition)
                goto out;

I prefer:

        if (condition) {
                ret = X;
                goto out;
        }

which makes it clear that ret is for the return and not some random state change unrelated to the condition.

Almost all igmp functions accessing inet->mc_list are protected by rtnl_lock(), but there is one exception which is ip_mc_sf_allow(), so there is a chance of either ip_mc_drop_socket or ip_mc_leave_group remove an entry while ip_mc_sf_allow is running causing a crash.

Signed-off-by Flavio Leitner <[email protected]>

net/ipv4/igmp.c +0 -0

@@ -2245,13 +2269,17 @@ void ip_mc_drop_socket(struct sock *sk)
 	struct ip_mc_socklist *iml;
 	struct net *net = sock_net(sk);
 

Almost all igmp functions accessing inet->mc_list are protected by rtnl_lock(), but there is one exception which is ip_mc_sf_allow(), so there is a chance of either ip_mc_drop_socket or ip_mc_leave_group remove an entry while ip_mc_sf_allow is running causing a crash.

Signed-off-by Flavio Leitner <[email protected]>

---
 include/linux/igmp.h |    1 +
 net/ipv4/igmp.c      |   58 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 45 insertions(+), 14 deletions(-)

include/linux/igmp.h +1 -0

index 724c27e..9cccd16 100644
@@ -170,6 +170,7 @@ struct ip_mc_socklist {
 	struct ip_mreqn		multi;
 	unsigned int		sfmode;		/* MCAST_{INCLUDE,EXCLUDE} */
 	struct ip_sf_socklist	*sflist;
+	struct rcu_head		rcu;
 };
 
 struct ip_sf_list {

net/ipv4/igmp.c +12 -1

index 76c0840..61ff685 100644
@@ -1799,7 +1799,7 @@ int ip_mc_join_group(struct sock *sk , struct ip_mreqn *imr)
 	iml->next = inet->mc_list;
 	iml->sflist = NULL;
 	iml->sfmode = MCAST_EXCLUDE;
-	inet->mc_list = iml;
+	rcu_assign_pointer(inet->mc_list, iml);
 	ip_mc_inc_group(in_dev, addr);
 	err = 0;
 done:

@@ -1825,6 +1825,17 @@ static int ip_mc_leave_src(struct sock *sk, struct ip_mc_socklist *iml,
 	return err;
 }
 
+
+static void ip_mc_socklist_reclaim(struct rcu_head *rp)
+{
+	struct ip_mc_socklist *iml;
+
+	iml = container_of(rp, struct ip_mc_socklist, rcu);
+	/* sk_omem_alloc should have been decreased by the caller*/
+	kfree(iml);
+}
+
+
 /*
  *	Ask a socket to leave a group.
  */

net/ipv4/igmp.c +22 -9

-		*imlp = iml->next;
+		rcu_assign_pointer(*imlp, iml->next);
 
 		if (in_dev)
 			ip_mc_dec_group(in_dev, group);
+
 		rtnl_unlock();
-		sock_kfree_s(sk, iml, sizeof(*iml));
+		/* decrease mem now to avoid the memleak warning */
+		atomic_sub(sizeof(*iml), &sk->sk_omem_alloc);
+		call_rcu(&iml->rcu, ip_mc_socklist_reclaim);
 		return 0;
 	}
 	if (!in_dev)

@@ -2209,30 +2223,40 @@ int ip_mc_sf_allow(struct sock *sk, __be32 loc_addr, __be32 rmt_addr, int dif)
 	struct ip_mc_socklist *pmc;
 	struct ip_sf_socklist *psl;
 	int i;
+	int ret;
 
+	ret = 1;
 	if (!ipv4_is_multicast(loc_addr))
-		return 1;
+		goto out;
 
-	for (pmc=inet->mc_list; pmc; pmc=pmc->next) {
+	rcu_read_lock();
+	for (pmc=rcu_dereference(inet->mc_list); pmc; pmc=rcu_dereference(pmc->next)) {
 		if (pmc->multi.imr_multiaddr.s_addr == loc_addr &&
 		    pmc->multi.imr_ifindex == dif)
 			break;
 	}
+	ret = inet->mc_all;
 	if (!pmc)
-		return inet->mc_all;
+		goto unlock;
 	psl = pmc->sflist;
+	ret = (pmc->sfmode == MCAST_EXCLUDE);
 	if (!psl)
-		return pmc->sfmode == MCAST_EXCLUDE;
+		goto unlock;
 
 	for (i=0; i<psl->sl_count; i++) {
 		if (psl->sl_addr[i] == rmt_addr)
 			break;
 	}
+	ret = 0;
 	if (pmc->sfmode == MCAST_INCLUDE && i >= psl->sl_count)
-		return 0;
+		goto unlock;
 	if (pmc->sfmode == MCAST_EXCLUDE && i < psl->sl_count)
-		return 0;
-	return 1;
+		goto unlock;
+	ret = 1;
+unlock:
+	rcu_read_unlock();
+out:
+	return ret;
 }
 
 /*

net/ipv4/igmp.c +5 -3

 	rtnl_lock();
-	while ((iml = inet->mc_list) != NULL) {
+	while ((iml = rcu_dereference(inet->mc_list)) != NULL) {
 		struct in_device *in_dev;
-		inet->mc_list = iml->next;
+		rcu_assign_pointer(inet->mc_list, iml->next);
 
 		in_dev = inetdev_by_index(net, iml->multi.imr_ifindex);
 		(void) ip_mc_leave_src(sk, iml, in_dev);

@@ -2259,7 +2287,9 @@ void ip_mc_drop_socket(struct sock *sk)
 			ip_mc_dec_group(in_dev, iml->multi.imr_multiaddr.s_addr);
 			in_dev_put(in_dev);
 		}
-		sock_kfree_s(sk, iml, sizeof(*iml));
+		/* decrease mem now to avoid the memleak warning */
+		atomic_sub(sizeof(*iml), &sk->sk_omem_alloc);
+		call_rcu(&iml->rcu, ip_mc_socklist_reclaim);
 	}
 	rtnl_unlock();
 }

Signature · 6 lines

1.6.2.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

net/ipv4/igmp.c void ip_mc_drop_socket(struct sock *sk) · line 2277

Quoted 7 lines

net/ipv4/igmp.c +5 -1

-	if (inet->mc_list == NULL)
+	rcu_read_lock();
+	if (rcu_dereference(inet->mc_list) == NULL) {
+		rcu_read_unlock();
 		return;
+	}
+	rcu_read_unlock();  

I don't understand what rcu_read_lock() is protecting here. The test is still unstable -- just after finding inet->mc_list non-NULL, ip_mc_leave_group() might cause it to become NULL.

Is there a need to protect sock_net(sk)? (I don't believe so, but then again, I don't claim to understand locking in Linux networking.) If there is no need, it should be possible to drop the rcu_read_lock(), rcu_read_unlock(), and rcu_dereference() above. (You might want them for documentation purposes, as they aren't hurting anything, just wondering what the intent is.)

Quoted 12 lines · On Wed, 6 Jan 2010 08:40:27 -0800 "Paul E. McKenney"…

net/ipv4/igmp.c void ip_mc_drop_socket(struct sock *sk) · line 2277

Quoted 7 lines

net/ipv4/igmp.c +5 -1

-	if (inet->mc_list == NULL)
+	rcu_read_lock();
+	if (rcu_dereference(inet->mc_list) == NULL) {
+		rcu_read_unlock();
 		return;
+	}
+	rcu_read_unlock();  

I don't understand what rcu_read_lock() is protecting here. The test is still unstable -- just after finding inet->mc_list non-NULL, ip_mc_leave_group() might cause it to become NULL.

Quoted 6 lines

Is there a need to protect sock_net(sk)? (I don't believe so, but then again, I don't claim to understand locking in Linux networking.) If there is no need, it should be possible to drop the rcu_read_lock(), rcu_read_unlock(), and rcu_dereference() above. (You might want them for documentation purposes, as they aren't hurting anything, just wondering what the intent is.)

I think code is trying to avoid looking at mc_list if no multicast addresses. But it is an unsafe check.

If mc_list was just converted to list_head this would all be clearer

Hello,

This is another version considering the suggestions. Thanks! I didn't convert to list_head yet because a lot more code needs to be changed so I'd rather leave that to another patch. 8<---------

Almost all igmp functions accessing inet->mc_list are protected by rtnl_lock(), but there is one exception which is ip_mc_sf_allow(), so there is a chance of either ip_mc_drop_socket or ip_mc_leave_group remove an entry while ip_mc_sf_allow is running causing a crash.

Signed-off-by Flavio Leitner <[email protected]>