netdev - Re: [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR

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]

Date: Fri, 21 Jun 2024 17:21:48 +0100
From: James Chapman <jchapman@...alix.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, gnault@...hat.com, samuel.thibault@...-lyon.org,
 ridge.kennedy@...iedtelesis.co.nz
Subject: Re: [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR

On 21/06/2024 13:59, Simon Horman wrote:
> On Thu, Jun 20, 2024 at 12:22:38PM +0100, James Chapman wrote:
>> L2TPv3 sessions are currently held in one of two fixed-size hash
>> lists: either a per-net hashlist (IP-encap), or a per-tunnel hashlist
>> (UDP-encap), keyed by the L2TPv3 32-bit session_id.
>>
>> In order to lookup L2TPv3 sessions in UDP-encap tunnels efficiently
>> without finding the tunnel first via sk_user_data, UDP sessions are
>> now kept in a per-net session list, keyed by session ID. Convert the
>> existing per-net hashlist to use an IDR for better performance when
>> there are many sessions and have L2TPv3 UDP sessions use the same IDR.
>>
>> Although the L2TPv3 RFC states that the session ID alone identifies
>> the session, our implementation has allowed the same session ID to be
>> used in different L2TP UDP tunnels. To retain support for this, a new
>> per-net session hashtable is used, keyed by the sock and session
>> ID. If on creating a new session, a session already exists with that
>> ID in the IDR, the colliding sessions are added to the new hashtable
>> and the existing IDR entry is flagged. When looking up sessions, the
>> approach is to first check the IDR and if no unflagged match is found,
>> check the new hashtable. The sock is made available to session getters
>> where session ID collisions are to be considered. In this way, the new
>> hashtable is used only for session ID collisions so can be kept small.
>>
>> For managing session removal, we need a list of colliding sessions
>> matching a given ID in order to update or remove the IDR entry of the
>> ID. This is necessary to detect session ID collisions when future
>> sessions are created. The list head is allocated on first collision
>> of a given ID and refcounted.
>>
>> Signed-off-by: James Chapman <jchapman@...alix.com>
>> Reviewed-by: Tom Parkin <tparkin@...alix.com>
> 
> ...
> 
>> @@ -358,39 +460,45 @@ int l2tp_session_register(struct l2tp_session *session,
>>   		}
>>   
>>   	if (tunnel->version == L2TP_HDR_VER_3) {
>> -		pn = l2tp_pernet(tunnel->l2tp_net);
>> -		g_head = l2tp_session_id_hash_2(pn, session->session_id);
>> -
>> -		spin_lock_bh(&pn->l2tp_session_hlist_lock);
>> -
>> +		session_key = session->session_id;
>> +		spin_lock_bh(&pn->l2tp_session_idr_lock);
>> +		err = idr_alloc_u32(&pn->l2tp_v3_session_idr, NULL,
>> +				    &session_key, session_key, GFP_ATOMIC);
>>   		/* IP encap expects session IDs to be globally unique, while
>> -		 * UDP encap doesn't.
>> +		 * UDP encap doesn't. This isn't per the RFC, which says that
>> +		 * sessions are identified only by the session ID, but is to
>> +		 * support existing userspace which depends on it.
>>   		 */
>> -		hlist_for_each_entry(session_walk, g_head, global_hlist)
>> -			if (session_walk->session_id == session->session_id &&
>> -			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
>> -			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
>> -				err = -EEXIST;
>> -				goto err_tlock_pnlock;
>> -			}
>> +		if (err == -ENOSPC && tunnel->encap == L2TP_ENCAPTYPE_UDP) {
>> +			struct l2tp_session *session2;
>>   
>> -		l2tp_tunnel_inc_refcount(tunnel);
>> -		hlist_add_head_rcu(&session->global_hlist, g_head);
>> -
>> -		spin_unlock_bh(&pn->l2tp_session_hlist_lock);
>> -	} else {
>> -		l2tp_tunnel_inc_refcount(tunnel);
>> +			session2 = idr_find(&pn->l2tp_v3_session_idr,
>> +					    session_key);
>> +			err = l2tp_session_collision_add(pn, session, session2);
>> +		}
>> +		spin_unlock_bh(&pn->l2tp_session_idr_lock);
>> +		if (err == -ENOSPC)
>> +			err = -EEXIST;
>>   	}
>>   
> 
> Hi James,
> 
> I believe that when the if condition above is false, then err will be
> uninitialised here.
> 
> If so, as this series seems to have been applied,
> could you provide a follow-up to address this?
> 
>> +	if (err)
>> +		goto err_tlock;
>> +
>> +	l2tp_tunnel_inc_refcount(tunnel);
>> +
>>   	hlist_add_head_rcu(&session->hlist, head);
>>   	spin_unlock_bh(&tunnel->hlist_lock);
>>   
>> +	if (tunnel->version == L2TP_HDR_VER_3) {
>> +		spin_lock_bh(&pn->l2tp_session_idr_lock);
>> +		idr_replace(&pn->l2tp_v3_session_idr, session, session_key);
>> +		spin_unlock_bh(&pn->l2tp_session_idr_lock);
>> +	}
>> +
>>   	trace_register_session(session);
>>   
>>   	return 0;
>>   
>> -err_tlock_pnlock:
>> -	spin_unlock_bh(&pn->l2tp_session_hlist_lock);
>>   err_tlock:
>>   	spin_unlock_bh(&tunnel->hlist_lock);
>>   
> 
> ...

Hi Simon,

It's "fixed" by the next patch in the series: 2a3339f6c963 ("l2tp: store 
l2tpv2 sessions in per-net IDR") which adds an else clause to the if 
statement quoted above. Sorry I missed this when compile-testing the 
series! Would you prefer a separate patch to initialise err?

James