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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110512142641.GO2258@linux.vnet.ibm.com>
Date:	Thu, 12 May 2011 07:26:41 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	James Chapman <jchapman@...alix.com>
Subject: Re: [PATCH net-next-2.6] l2tp: fix potential rcu race

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@...ux.vnet.ibm.com>

> Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
> CC: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> CC: James Chapman <jchapman@...alix.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html