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

[Simon Horman <horms@kernel.org>]

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@katalix.com>
> Reviewed-by: Tom Parkin <tparkin@katalix.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);
>  

...