[PATCH net-next-2.6] l2tp: fix potential rcu race

[PATCH net-next-2.6] l2tp: fix potential rcu race

[Eric Dumazet]

While trying to remove useless synchronize_rcu() calls, I found l2tp is
indeed incorrectly using two of such calls, but also bumps tunnel
refcount after list insertion.

tunnel refcount must be incremented before being made publically visible
by rcu readers.

This fix can be applied to 2.6.35+ and might need a backport for older
kernels, since things were shuffled in commit fd558d186df2c
(l2tp: Split pppol2tp patch into separate l2tp and ppp parts)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
CC: James Chapman <jchapman@katalix.com>
---

I based this patch on net-next-2.6 because of recent commits in this
file and linux-2.6 being on late RC phase. But its a stable candidate.

 net/l2tp/l2tp_core.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 9be095e..ed8a233 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1435,16 +1435,15 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 
 	/* Add tunnel to our list */
 	INIT_LIST_HEAD(&tunnel->list);
-	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
-	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
-	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
-	synchronize_rcu();
 	atomic_inc(&l2tp_tunnel_count);
 
 	/* Bump the reference count. The tunnel context is deleted
-	 * only when this drops to zero.
+	 * only when this drops to zero. Must be done before list insertion
 	 */
 	l2tp_tunnel_inc_refcount(tunnel);
+	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
+	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
+	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
 
 	err = 0;
 err:
@@ -1636,7 +1635,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 			hlist_add_head_rcu(&session->global_hlist,
 					   l2tp_session_id_hash_2(pn, session_id));
 			spin_unlock_bh(&pn->l2tp_session_hlist_lock);
-			synchronize_rcu();
 		}
 
 		/* Ignore management session in session count value */

Re: [PATCH net-next-2.6] l2tp: fix potential rcu race

[Paul E. McKenney]

On Thu, May 12, 2011 at 06:22:36AM +0200, Eric Dumazet wrote:
> While trying to remove useless synchronize_rcu() calls, I found l2tp is
> indeed incorrectly using two of such calls, but also bumps tunnel
> refcount after list insertion.
> 
> tunnel refcount must be incremented before being made publically visible
> by rcu readers.
> 
> This fix can be applied to 2.6.35+ and might need a backport for older
> kernels, since things were shuffled in commit fd558d186df2c
> (l2tp: Split pppol2tp patch into separate l2tp and ppp parts)

Ouch!  Good catch, Eric!

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> CC: James Chapman <jchapman@katalix.com>
> ---
> 
> I based this patch on net-next-2.6 because of recent commits in this
> file and linux-2.6 being on late RC phase. But its a stable candidate.
> 
>  net/l2tp/l2tp_core.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 9be095e..ed8a233 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1435,16 +1435,15 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> 
>  	/* Add tunnel to our list */
>  	INIT_LIST_HEAD(&tunnel->list);
> -	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
> -	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
> -	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
> -	synchronize_rcu();
>  	atomic_inc(&l2tp_tunnel_count);
> 
>  	/* Bump the reference count. The tunnel context is deleted
> -	 * only when this drops to zero.
> +	 * only when this drops to zero. Must be done before list insertion
>  	 */
>  	l2tp_tunnel_inc_refcount(tunnel);
> +	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
> +	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
> +	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
> 
>  	err = 0;
>  err:
> @@ -1636,7 +1635,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
>  			hlist_add_head_rcu(&session->global_hlist,
>  					   l2tp_session_id_hash_2(pn, session_id));
>  			spin_unlock_bh(&pn->l2tp_session_hlist_lock);
> -			synchronize_rcu();
>  		}
> 
>  		/* Ignore management session in session count value */
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next-2.6] l2tp: fix potential rcu race

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Thu, 12 May 2011 07:26:41 -0700

> On Thu, May 12, 2011 at 06:22:36AM +0200, Eric Dumazet wrote:
>> While trying to remove useless synchronize_rcu() calls, I found l2tp is
>> indeed incorrectly using two of such calls, but also bumps tunnel
>> refcount after list insertion.
>> 
>> tunnel refcount must be incremented before being made publically visible
>> by rcu readers.
>> 
>> This fix can be applied to 2.6.35+ and might need a backport for older
>> kernels, since things were shuffled in commit fd558d186df2c
>> (l2tp: Split pppol2tp patch into separate l2tp and ppp parts)
> 
> Ouch!  Good catch, Eric!
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Applied, thanks Eric.