[Patch -next] Adapt s390 qeth & lcs driver code to use RCU

[Patch -next] Adapt s390 qeth & lcs driver code to use RCU

[Sachin Sant]

Commit 1d7138de878d1d4210727c1200193e69596f93b3
igmp: RCU conversion of in_dev->mc_list

converted rwlock to RCU.

Update the s390 network drivers(qeth & lcs) code to adapt to this change.

Signed-off-by : Sachin Sant <sachinp@in.ibm.com>
---

Only compile tested.

diff -Narup linux-2.6-next/drivers/s390/net/lcs.c linux-2.6-next-new/drivers/s390/net/lcs.c
--- linux-2.6-next/drivers/s390/net/lcs.c	2010-11-17 11:38:25.000000000 +0530
+++ linux-2.6-next-new/drivers/s390/net/lcs.c	2010-11-18 11:59:46.000000000 +0530
@@ -1269,10 +1269,10 @@ lcs_register_mc_addresses(void *data)
 	in4_dev = in_dev_get(card->dev);
 	if (in4_dev == NULL)
 		goto out;
-	read_lock(&in4_dev->mc_list_lock);
+	rcu_read_lock();
 	lcs_remove_mc_addresses(card,in4_dev);
 	lcs_set_mc_addresses(card, in4_dev);
-	read_unlock(&in4_dev->mc_list_lock);
+	rcu_read_unlock();
 	in_dev_put(in4_dev);
 
 	netif_carrier_off(card->dev);
diff -Narup linux-2.6-next/drivers/s390/net/qeth_l3_main.c linux-2.6-next-new/drivers/s390/net/qeth_l3_main.c
--- linux-2.6-next/drivers/s390/net/qeth_l3_main.c	2010-10-30 12:54:22.000000000 +0530
+++ linux-2.6-next-new/drivers/s390/net/qeth_l3_main.c	2010-11-18 11:59:13.000000000 +0530
@@ -1828,9 +1828,9 @@ static void qeth_l3_add_vlan_mc(struct q
 		in_dev = in_dev_get(netdev);
 		if (!in_dev)
 			continue;
-		read_lock(&in_dev->mc_list_lock);
+		rcu_read_lock();
 		qeth_l3_add_mc(card, in_dev);
-		read_unlock(&in_dev->mc_list_lock);
+		rcu_read_unlock();
 		in_dev_put(in_dev);
 	}
 }
@@ -1843,10 +1843,10 @@ static void qeth_l3_add_multicast_ipv4(s
 	in4_dev = in_dev_get(card->dev);
 	if (in4_dev == NULL)
 		return;
-	read_lock(&in4_dev->mc_list_lock);
+	rcu_read_lock();
 	qeth_l3_add_mc(card, in4_dev);
 	qeth_l3_add_vlan_mc(card);
-	read_unlock(&in4_dev->mc_list_lock);
+	rcu_read_unlock();
 	in_dev_put(in4_dev);
 }
 

Re: [Patch -next] Adapt s390 qeth & lcs driver code to use RCU

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 14:48 +0530, Sachin Sant a écrit :
> Commit 1d7138de878d1d4210727c1200193e69596f93b3
> igmp: RCU conversion of in_dev->mc_list
> 
> converted rwlock to RCU.
> 
> Update the s390 network drivers(qeth & lcs) code to adapt to this change.
> 
> Signed-off-by : Sachin Sant <sachinp@in.ibm.com>
> ---
> 
> Only compile tested.
> 

Hmm, sorry but this wont work.

> diff -Narup linux-2.6-next/drivers/s390/net/lcs.c linux-2.6-next-new/drivers/s390/net/lcs.c
> --- linux-2.6-next/drivers/s390/net/lcs.c	2010-11-17 11:38:25.000000000 +0530
> +++ linux-2.6-next-new/drivers/s390/net/lcs.c	2010-11-18 11:59:46.000000000 +0530
> @@ -1269,10 +1269,10 @@ lcs_register_mc_addresses(void *data)
>  	in4_dev = in_dev_get(card->dev);
>  	if (in4_dev == NULL)
>  		goto out;
> -	read_lock(&in4_dev->mc_list_lock);
> +	rcu_read_lock();

If you use rcu_read_lock(), then you also need to 
use the rcu list iterators in lcs_remove_mc_addresses() and
lcs_set_mc_addresses()

Then, its strange this driver is not protected by RTNL at this stage.

Ah yes, it uses a kthread from its ndo_set_multicast_list() handler.

This seems not safe at all.


>  	lcs_remove_mc_addresses(card,in4_dev);
>  	lcs_set_mc_addresses(card, in4_dev);
> -	read_unlock(&in4_dev->mc_list_lock);
> +	rcu_read_unlock();
>  	in_dev_put(in4_dev);
>  
>  	netif_carrier_off(card->dev);
> diff -Narup linux-2.6-next/drivers/s390/net/qeth_l3_main.c linux-2.6-next-new/drivers/s390/net/qeth_l3_main.c
> --- linux-2.6-next/drivers/s390/net/qeth_l3_main.c	2010-10-30 12:54:22.000000000 +0530
> +++ linux-2.6-next-new/drivers/s390/net/qeth_l3_main.c	2010-11-18 11:59:13.000000000 +0530
> @@ -1828,9 +1828,9 @@ static void qeth_l3_add_vlan_mc(struct q
>  		in_dev = in_dev_get(netdev);
>  		if (!in_dev)
>  			continue;
> -		read_lock(&in_dev->mc_list_lock);
> +		rcu_read_lock();
>  		qeth_l3_add_mc(card, in_dev);
> -		read_unlock(&in_dev->mc_list_lock);
> +		rcu_read_unlock();
>  		in_dev_put(in_dev);
>  	}
>  }
> @@ -1843,10 +1843,10 @@ static void qeth_l3_add_multicast_ipv4(s
>  	in4_dev = in_dev_get(card->dev);
>  	if (in4_dev == NULL)
>  		return;
> -	read_lock(&in4_dev->mc_list_lock);
> +	rcu_read_lock();
>  	qeth_l3_add_mc(card, in4_dev);
>  	qeth_l3_add_vlan_mc(card);
> -	read_unlock(&in4_dev->mc_list_lock);
> +	rcu_read_unlock();
>  	in_dev_put(in4_dev);
>  }
>  

Re: [Patch -next] Adapt s390 qeth & lcs driver code to use RCU

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 10:33 +0100, Eric Dumazet a écrit :
> Le jeudi 18 novembre 2010 à 14:48 +0530, Sachin Sant a écrit :
> > Commit 1d7138de878d1d4210727c1200193e69596f93b3
> > igmp: RCU conversion of in_dev->mc_list
> > 
> > converted rwlock to RCU.
> > 
> > Update the s390 network drivers(qeth & lcs) code to adapt to this change.
> > 
> > Signed-off-by : Sachin Sant <sachinp@in.ibm.com>
> > ---
> > 
> > Only compile tested.
> > 
> 
> Hmm, sorry but this wont work.
> 
> > diff -Narup linux-2.6-next/drivers/s390/net/lcs.c linux-2.6-next-new/drivers/s390/net/lcs.c
> > --- linux-2.6-next/drivers/s390/net/lcs.c	2010-11-17 11:38:25.000000000 +0530
> > +++ linux-2.6-next-new/drivers/s390/net/lcs.c	2010-11-18 11:59:46.000000000 +0530
> > @@ -1269,10 +1269,10 @@ lcs_register_mc_addresses(void *data)
> >  	in4_dev = in_dev_get(card->dev);
> >  	if (in4_dev == NULL)
> >  		goto out;
> > -	read_lock(&in4_dev->mc_list_lock);
> > +	rcu_read_lock();
> 
> If you use rcu_read_lock(), then you also need to 
> use the rcu list iterators in lcs_remove_mc_addresses() and
> lcs_set_mc_addresses()
> 
> Then, its strange this driver is not protected by RTNL at this stage.
> 
> Ah yes, it uses a kthread from its ndo_set_multicast_list() handler.
> 
> This seems not safe at all.

Please check following patch to give you the idea of what is needed :


diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
index 0f19d54..05755b7 100644
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -1188,7 +1188,9 @@ lcs_remove_mc_addresses(struct lcs_card *card, struct in_device *in4_dev)
 	spin_lock_irqsave(&card->ipm_lock, flags);
 	list_for_each(l, &card->ipm_list) {
 		ipm = list_entry(l, struct lcs_ipm_list, list);
-		for (im4 = in4_dev->mc_list; im4 != NULL; im4 = im4->next) {
+		for (im4 = rcu_dereference(in4_dev->mc_list);
+		     im4 != NULL;
+		     im4 = rcu_dereference(im4->next_rcu)) {
 			lcs_get_mac_for_ipm(im4->multiaddr, buf, card->dev);
 			if ( (ipm->ipm.ip_addr == im4->multiaddr) &&
 			     (memcmp(buf, &ipm->ipm.mac_addr,
@@ -1233,7 +1235,9 @@ lcs_set_mc_addresses(struct lcs_card *card, struct in_device *in4_dev)
 	unsigned long flags;
 
 	LCS_DBF_TEXT(4, trace, "setmclst");
-	for (im4 = in4_dev->mc_list; im4; im4 = im4->next) {
+	for (im4 = rcu_dereference(in4_dev->mc_list);
+	     im4 != NULL;
+	     im4 = rcu_dereference(im4->next_rcu)) {
 		lcs_get_mac_for_ipm(im4->multiaddr, buf, card->dev);
 		ipm = lcs_check_addr_entry(card, im4, buf);
 		if (ipm != NULL)
@@ -1269,10 +1273,10 @@ lcs_register_mc_addresses(void *data)
 	in4_dev = in_dev_get(card->dev);
 	if (in4_dev == NULL)
 		goto out;
-	read_lock(&in4_dev->mc_list_lock);
+	rcu_read_lock();
 	lcs_remove_mc_addresses(card,in4_dev);
 	lcs_set_mc_addresses(card, in4_dev);
-	read_unlock(&in4_dev->mc_list_lock);
+	rcu_read_unlock();
 	in_dev_put(in4_dev);
 
 	netif_carrier_off(card->dev);

[PATCH net-2.6] bonding: fix a race in IGMP handling

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 10:43 +0100, Eric Dumazet a écrit :
> Le jeudi 18 novembre 2010 à 10:33 +0100, Eric Dumazet a écrit :
> > 
> > 
> > 
> > Hmm, sorry but this wont work.
> > 
> > > diff -Narup linux-2.6-next/drivers/s390/net/lcs.c linux-2.6-next-new/drivers/s390/net/lcs.c
> > > --- linux-2.6-next/drivers/s390/net/lcs.c	2010-11-17 11:38:25.000000000 +0530
> > > +++ linux-2.6-next-new/drivers/s390/net/lcs.c	2010-11-18 11:59:46.000000000 +0530
> > > @@ -1269,10 +1269,10 @@ lcs_register_mc_addresses(void *data)
> > >  	in4_dev = in_dev_get(card->dev);
> > >  	if (in4_dev == NULL)
> > >  		goto out;
> > > -	read_lock(&in4_dev->mc_list_lock);
> > > +	rcu_read_lock();
> > 
> > If you use rcu_read_lock(), then you also need to 
> > use the rcu list iterators in lcs_remove_mc_addresses() and
> > lcs_set_mc_addresses()
> > 
> > Then, its strange this driver is not protected by RTNL at this stage.
> > 
> > Ah yes, it uses a kthread from its ndo_set_multicast_list() handler.
> > 
> > This seems not safe at all.

Actually this raises an interesting case for bonding as well.

Before my RCU conversion __bond_resend_igmp_join_requests() was unsafe.

For net-next-2.6, it is now safe (RCU is held), but needs a cleanup
patch to avoid sparse errors.

Thanks

[PATCH net-2.6] bonding: fix a race in IGMP handling

RCU conversion in IGMP code done in net-next-2.6 raised a race in
__bond_resend_igmp_join_requests().

It iterates in_dev->mc_list without appropriate protection (RTNL, or
read_lock on in_dev->mc_list_lock).

Another cpu might delete an entry while we use it and trigger a fault.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_main.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bdb68a6..71a1697 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -878,8 +878,10 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
 	rcu_read_lock();
 	in_dev = __in_dev_get_rcu(dev);
 	if (in_dev) {
+		read_lock(&in_dev->mc_list_lock);
 		for (im = in_dev->mc_list; im; im = im->next)
 			ip_mc_rejoin_group(im);
+		read_unlock(&in_dev->mc_list_lock);
 	}
 
 	rcu_read_unlock();

[PATCH net-2.6] bonding: fix a race in IGMP handling

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 10:43 +0100, Eric Dumazet a écrit :
> Le jeudi 18 novembre 2010 à 10:33 +0100, Eric Dumazet a écrit :
> > 
> > 
> > 
> > Hmm, sorry but this wont work.
> > 
> > > diff -Narup linux-2.6-next/drivers/s390/net/lcs.c linux-2.6-next-new/drivers/s390/net/lcs.c
> > > --- linux-2.6-next/drivers/s390/net/lcs.c	2010-11-17 11:38:25.000000000 +0530
> > > +++ linux-2.6-next-new/drivers/s390/net/lcs.c	2010-11-18 11:59:46.000000000 +0530
> > > @@ -1269,10 +1269,10 @@ lcs_register_mc_addresses(void *data)
> > >  	in4_dev = in_dev_get(card->dev);
> > >  	if (in4_dev == NULL)
> > >  		goto out;
> > > -	read_lock(&in4_dev->mc_list_lock);
> > > +	rcu_read_lock();
> > 
> > If you use rcu_read_lock(), then you also need to 
> > use the rcu list iterators in lcs_remove_mc_addresses() and
> > lcs_set_mc_addresses()
> > 
> > Then, its strange this driver is not protected by RTNL at this stage.
> > 
> > Ah yes, it uses a kthread from its ndo_set_multicast_list() handler.
> > 
> > This seems not safe at all.

Actually this raises an interesting case for bonding as well.

Before my RCU conversion __bond_resend_igmp_join_requests() was unsafe.

For net-next-2.6, it is now safe (RCU is held), but needs a cleanup
patch to avoid sparse errors.

Thanks

[PATCH net-2.6] bonding: fix a race in IGMP handling

RCU conversion in IGMP code done in net-next-2.6 raised a race in
__bond_resend_igmp_join_requests().

It iterates in_dev->mc_list without appropriate protection (RTNL, or
read_lock on in_dev->mc_list_lock).

Another cpu might delete an entry while we use it and trigger a fault.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_main.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bdb68a6..71a1697 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -878,8 +878,10 @@ static void __bond_resend_igmp_join_requests(struct net_device *dev)
 	rcu_read_lock();
 	in_dev = __in_dev_get_rcu(dev);
 	if (in_dev) {
+		read_lock(&in_dev->mc_list_lock);
 		for (im = in_dev->mc_list; im; im = im->next)
 			ip_mc_rejoin_group(im);
+		read_unlock(&in_dev->mc_list_lock);
 	}
 
 	rcu_read_unlock();

[PATCH net-next-2.6] bonding: IGMP handling cleanup

[Eric Dumazet]

Le jeudi 18 novembre 2010 à 11:26 +0100, Eric Dumazet a écrit :

> Actually this raises an interesting case for bonding as well.
> 
> Before my RCU conversion __bond_resend_igmp_join_requests() was unsafe.
> 
> For net-next-2.6, it is now safe (RCU is held), but needs a cleanup
> patch to avoid sparse errors.

[PATCH net-next-2.6] bonding: IGMP handling cleanup

Instead of iterating in_dev->mc_list from bonding driver, its better
to call a helper function provided by igmp.c
Details of implementation (locking) are private to igmp code.

ip_mc_rejoin_group(struct ip_mc_list *im) becomes
ip_mc_rejoin_groups(struct in_device *in_dev);

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Sachin Sant <sachinp@in.ibm.com>
---
 drivers/net/bonding/bond_main.c |    8 +------
 include/linux/igmp.h            |    2 -
 net/ipv4/igmp.c                 |   34 +++++++++++++++++-------------
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5188448..e588b2e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -873,15 +873,11 @@ static void bond_mc_del(struct bonding *bond, void *addr)
 static void __bond_resend_igmp_join_requests(struct net_device *dev)
 {
 	struct in_device *in_dev;
-	struct ip_mc_list *im;
 
 	rcu_read_lock();
 	in_dev = __in_dev_get_rcu(dev);
-	if (in_dev) {
-		for (im = in_dev->mc_list; im; im = im->next)
-			ip_mc_rejoin_group(im);
-	}
-
+	if (in_dev)
+			ip_mc_rejoin_groups(in_dev);
 	rcu_read_unlock();
 }
 
diff --git a/include/linux/igmp.h b/include/linux/igmp.h
index 7d16467..c4987f2 100644
--- a/include/linux/igmp.h
+++ b/include/linux/igmp.h
@@ -238,7 +238,7 @@ extern void ip_mc_unmap(struct in_device *);
 extern void ip_mc_remap(struct in_device *);
 extern void ip_mc_dec_group(struct in_device *in_dev, __be32 addr);
 extern void ip_mc_inc_group(struct in_device *in_dev, __be32 addr);
-extern void ip_mc_rejoin_group(struct ip_mc_list *im);
+extern void ip_mc_rejoin_groups(struct in_device *in_dev);
 
 #endif
 #endif
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index afb1e82..35f0231 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1267,26 +1267,32 @@ EXPORT_SYMBOL(ip_mc_inc_group);
 
 /*
  *	Resend IGMP JOIN report; used for bonding.
+ *	Called with rcu_read_lock()
  */
-void ip_mc_rejoin_group(struct ip_mc_list *im)
+void ip_mc_rejoin_groups(struct in_device *in_dev)
 {
 #ifdef CONFIG_IP_MULTICAST
-	struct in_device *in_dev = im->interface;
-
-	if (im->multiaddr == IGMP_ALL_HOSTS)
-		return;
+	struct ip_mc_list *im;
+	int type;
+	
+	for_each_pmc_rcu(in_dev, im) {
+		if (im->multiaddr == IGMP_ALL_HOSTS)
+			continue;
 
-	/* a failover is happening and switches
-	 * must be notified immediately */
-	if (IGMP_V1_SEEN(in_dev))
-		igmp_send_report(in_dev, im, IGMP_HOST_MEMBERSHIP_REPORT);
-	else if (IGMP_V2_SEEN(in_dev))
-		igmp_send_report(in_dev, im, IGMPV2_HOST_MEMBERSHIP_REPORT);
-	else
-		igmp_send_report(in_dev, im, IGMPV3_HOST_MEMBERSHIP_REPORT);
+		/* a failover is happening and switches
+		 * must be notified immediately
+		 */
+		if (IGMP_V1_SEEN(in_dev))
+			type = IGMP_HOST_MEMBERSHIP_REPORT;
+		else if (IGMP_V2_SEEN(in_dev))
+			type = IGMPV2_HOST_MEMBERSHIP_REPORT;
+		else
+			type = IGMPV3_HOST_MEMBERSHIP_REPORT;
+		igmp_send_report(in_dev, im, type);
+	}
 #endif
 }
-EXPORT_SYMBOL(ip_mc_rejoin_group);
+EXPORT_SYMBOL(ip_mc_rejoin_groups);
 
 /*
  *	A socket has left a multicast group on device dev

Re: [PATCH net-2.6] bonding: fix a race in IGMP handling

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 18 Nov 2010 11:26:18 +0100

> Actually this raises an interesting case for bonding as well.
> 
> Before my RCU conversion __bond_resend_igmp_join_requests() was unsafe.
> 
> For net-next-2.6, it is now safe (RCU is held), but needs a cleanup
> patch to avoid sparse errors.
> 
> Thanks
> 
> [PATCH net-2.6] bonding: fix a race in IGMP handling
> 
> RCU conversion in IGMP code done in net-next-2.6 raised a race in
> __bond_resend_igmp_join_requests().
> 
> It iterates in_dev->mc_list without appropriate protection (RTNL, or
> read_lock on in_dev->mc_list_lock).
> 
> Another cpu might delete an entry while we use it and trigger a fault.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Jay Vosburgh <fubar@us.ibm.com>

Applied, but I'm going to have to be careful and make sure I undo
this the next time I pull net-2.6 into net-next-2.6

Thanks.

Re: [PATCH net-next-2.6] bonding: IGMP handling cleanup

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 18 Nov 2010 11:49:12 +0100

> Le jeudi 18 novembre 2010 à 11:26 +0100, Eric Dumazet a écrit :
> 
>> Actually this raises an interesting case for bonding as well.
>> 
>> Before my RCU conversion __bond_resend_igmp_join_requests() was unsafe.
>> 
>> For net-next-2.6, it is now safe (RCU is held), but needs a cleanup
>> patch to avoid sparse errors.
> 
> [PATCH net-next-2.6] bonding: IGMP handling cleanup
> 
> Instead of iterating in_dev->mc_list from bonding driver, its better
> to call a helper function provided by igmp.c
> Details of implementation (locking) are private to igmp code.
> 
> ip_mc_rejoin_group(struct ip_mc_list *im) becomes
> ip_mc_rejoin_groups(struct in_device *in_dev);
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Sachin Sant <sachinp@in.ibm.com>

Applied, thanks Eric.