[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240623074259.GJ1098275@kernel.org>
Date: Sun, 23 Jun 2024 08:42:59 +0100
From: Simon Horman <horms@...nel.org>
To: James Chapman <jchapman@...alix.com>
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 Fri, Jun 21, 2024 at 05:21:48PM +0100, James Chapman wrote:
> 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?
Hi James,
Thanks for your response and sorry for missing that.
I see it now and I don't think any further action is required.
Powered by blists - more mailing lists