[PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath

[PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath

[James Chapman]

This series refactors l2tp to not use the tunnel socket's sk_user_data
in the datapath. The main reasons for doing this are

 * to allow for simplifying internal socket cleanup code (to be done
   in a later series)
 * to support multiple L2TPv3 UDP tunnels using the same 5-tuple
   address

When handling received UDP frames, l2tp's current approach is to look
up a session in a per-tunnel list. l2tp uses the tunnel socket's
sk_user_data to derive the tunnel context from the receiving socket.

But this results in the socket and tunnel lifetimes being very tightly
coupled and the tunnel/socket cleanup paths being complicated. The
latter has historically been a source of l2tp bugs and makes the code
more difficult to maintain. Also, if sockets are aliased, we can't
trust that the socket's sk_user_data references the right tunnel
anyway. Hence the desire to not use sk_user_data in the datapath.

The new approach is to lookup sessions in a per-net session list
without first deriving the tunnel:

 * For L2TPv2, the l2tp header has separate tunnel ID and session ID
   fields which can be trivially combined to make a unique 32-bit key
   for per-net session lookup.

 * For L2TPv3, there is no tunnel ID in the packet header, only a
   session ID, which should be unique over all tunnels so can be used
   as a key for per-net session lookup. However, this only works when
   the L2TPv3 session ID really is unique over all tunnels. At least
   one L2TPv3 application is known to use the same session ID in
   different L2TPv3 UDP tunnels, relying on UDP address/ports to
   distinguish them. This worked previously because sessions in UDP
   tunnels were kept in a per-tunnel list. To retain support for this,
   L2TPv3 session ID collisions are managed using a separate per-net
   session hlist, keyed by ID and sk. When looking up a session by ID,
   if there's more than one match, sk is used to find the right one.

L2TPv3 sessions in IP-encap tunnels are already looked up by session
ID in a per-net list. This work has UDP sessions also use the per-net
session list, while allowing for session ID collisions. The existing
per-tunnel hlist becomes a plain list since it is used only in
management and cleanup paths to walk a list of sessions in a given
tunnel.

For better performance, the per-net session lists use IDR. Separate
IDRs are used for L2TPv2 and L2TPv3 sessions to avoid potential key
collisions.

These changes pass l2tp regression tests and improve data forwarding
performance by about 10% in some of my test setups.

James Chapman (8):
  l2tp: remove unused list_head member in l2tp_tunnel
  l2tp: store l2tpv3 sessions in per-net IDR
  l2tp: store l2tpv2 sessions in per-net IDR
  l2tp: refactor udp recv to lookup to not use sk_user_data
  l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv
  l2tp: use IDR for all session lookups
  l2tp: drop the now unused l2tp_tunnel_get_session
  l2tp: replace hlist with simple list for per-tunnel session list

 net/l2tp/l2tp_core.c    | 501 ++++++++++++++++++++++------------------
 net/l2tp/l2tp_core.h    |  43 ++--
 net/l2tp/l2tp_debugfs.c |  13 +-
 net/l2tp/l2tp_ip.c      |   2 +-
 net/l2tp/l2tp_ip6.c     |   2 +-
 net/l2tp/l2tp_netlink.c |   6 +-
 net/l2tp/l2tp_ppp.c     |   6 +-
 7 files changed, 308 insertions(+), 265 deletions(-)

-- 
2.34.1

[PATCH net-next 1/8] l2tp: remove unused list_head member in l2tp_tunnel

[James Chapman]

Remove an unused variable in struct l2tp_tunnel which was left behind
by commit c4d48a58f32c5 ("l2tp: convert l2tp_tunnel_list to idr").

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 --
 net/l2tp/l2tp_core.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 88a34db265d8..69f8c9f5cdc7 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1462,8 +1462,6 @@ int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
 	/* Init delete workqueue struct */
 	INIT_WORK(&tunnel->del_work, l2tp_tunnel_del_work);
 
-	INIT_LIST_HEAD(&tunnel->list);
-
 	err = 0;
 err:
 	if (tunnelp)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 91ebf0a3f499..54dfba1eb91c 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -174,7 +174,6 @@ struct l2tp_tunnel {
 	enum l2tp_encap_type	encap;
 	struct l2tp_stats	stats;
 
-	struct list_head	list;		/* list node on per-namespace list of tunnels */
 	struct net		*l2tp_net;	/* the net we belong to */
 
 	refcount_t		ref_count;
-- 
2.34.1

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

[James Chapman]

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>
---
 net/l2tp/l2tp_core.c | 240 +++++++++++++++++++++++++++++++------------
 net/l2tp/l2tp_core.h |  18 ++--
 net/l2tp/l2tp_ip.c   |   2 +-
 net/l2tp/l2tp_ip6.c  |   2 +-
 4 files changed, 188 insertions(+), 74 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 69f8c9f5cdc7..d6bffdb16466 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -107,11 +107,17 @@ struct l2tp_net {
 	/* Lock for write access to l2tp_tunnel_idr */
 	spinlock_t l2tp_tunnel_idr_lock;
 	struct idr l2tp_tunnel_idr;
-	struct hlist_head l2tp_session_hlist[L2TP_HASH_SIZE_2];
-	/* Lock for write access to l2tp_session_hlist */
-	spinlock_t l2tp_session_hlist_lock;
+	/* Lock for write access to l2tp_v3_session_idr/htable */
+	spinlock_t l2tp_session_idr_lock;
+	struct idr l2tp_v3_session_idr;
+	struct hlist_head l2tp_v3_session_htable[16];
 };
 
+static inline unsigned long l2tp_v3_session_hashkey(struct sock *sk, u32 session_id)
+{
+	return ((unsigned long)sk) + session_id;
+}
+
 #if IS_ENABLED(CONFIG_IPV6)
 static bool l2tp_sk_is_v6(struct sock *sk)
 {
@@ -125,17 +131,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)
 	return net_generic(net, l2tp_net_id);
 }
 
-/* Session hash global list for L2TPv3.
- * The session_id SHOULD be random according to RFC3931, but several
- * L2TP implementations use incrementing session_ids.  So we do a real
- * hash on the session_id, rather than a simple bitmask.
- */
-static inline struct hlist_head *
-l2tp_session_id_hash_2(struct l2tp_net *pn, u32 session_id)
-{
-	return &pn->l2tp_session_hlist[hash_32(session_id, L2TP_HASH_BITS_2)];
-}
-
 /* Session hash list.
  * The session_id SHOULD be random according to RFC2661, but several
  * L2TP implementations (Cisco and Microsoft) use incrementing
@@ -262,26 +257,40 @@ struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_get_session);
 
-struct l2tp_session *l2tp_session_get(const struct net *net, u32 session_id)
+struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id)
 {
-	struct hlist_head *session_list;
+	const struct l2tp_net *pn = l2tp_pernet(net);
 	struct l2tp_session *session;
 
-	session_list = l2tp_session_id_hash_2(l2tp_pernet(net), session_id);
-
 	rcu_read_lock_bh();
-	hlist_for_each_entry_rcu(session, session_list, global_hlist)
-		if (session->session_id == session_id) {
-			l2tp_session_inc_refcount(session);
-			rcu_read_unlock_bh();
+	session = idr_find(&pn->l2tp_v3_session_idr, session_id);
+	if (session && !hash_hashed(&session->hlist) &&
+	    refcount_inc_not_zero(&session->ref_count)) {
+		rcu_read_unlock_bh();
+		return session;
+	}
 
-			return session;
+	/* If we get here and session is non-NULL, the session_id
+	 * collides with one in another tunnel. If sk is non-NULL,
+	 * find the session matching sk.
+	 */
+	if (session && sk) {
+		unsigned long key = l2tp_v3_session_hashkey(sk, session->session_id);
+
+		hash_for_each_possible_rcu(pn->l2tp_v3_session_htable, session,
+					   hlist, key) {
+			if (session->tunnel->sock == sk &&
+			    refcount_inc_not_zero(&session->ref_count)) {
+				rcu_read_unlock_bh();
+				return session;
+			}
 		}
+	}
 	rcu_read_unlock_bh();
 
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(l2tp_session_get);
+EXPORT_SYMBOL_GPL(l2tp_v3_session_get);
 
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 {
@@ -313,12 +322,12 @@ struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname)
 {
 	struct l2tp_net *pn = l2tp_pernet(net);
-	int hash;
+	unsigned long session_id, tmp;
 	struct l2tp_session *session;
 
 	rcu_read_lock_bh();
-	for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++) {
-		hlist_for_each_entry_rcu(session, &pn->l2tp_session_hlist[hash], global_hlist) {
+	idr_for_each_entry_ul(&pn->l2tp_v3_session_idr, session, tmp, session_id) {
+		if (session) {
 			if (!strcmp(session->ifname, ifname)) {
 				l2tp_session_inc_refcount(session);
 				rcu_read_unlock_bh();
@@ -334,13 +343,106 @@ struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 }
 EXPORT_SYMBOL_GPL(l2tp_session_get_by_ifname);
 
+static void l2tp_session_coll_list_add(struct l2tp_session_coll_list *clist,
+				       struct l2tp_session *session)
+{
+	l2tp_session_inc_refcount(session);
+	WARN_ON_ONCE(session->coll_list);
+	session->coll_list = clist;
+	spin_lock(&clist->lock);
+	list_add(&session->clist, &clist->list);
+	spin_unlock(&clist->lock);
+}
+
+static int l2tp_session_collision_add(struct l2tp_net *pn,
+				      struct l2tp_session *session1,
+				      struct l2tp_session *session2)
+{
+	struct l2tp_session_coll_list *clist;
+
+	lockdep_assert_held(&pn->l2tp_session_idr_lock);
+
+	if (!session2)
+		return -EEXIST;
+
+	/* If existing session is in IP-encap tunnel, refuse new session */
+	if (session2->tunnel->encap == L2TP_ENCAPTYPE_IP)
+		return -EEXIST;
+
+	clist = session2->coll_list;
+	if (!clist) {
+		/* First collision. Allocate list to manage the collided sessions
+		 * and add the existing session to the list.
+		 */
+		clist = kmalloc(sizeof(*clist), GFP_ATOMIC);
+		if (!clist)
+			return -ENOMEM;
+
+		spin_lock_init(&clist->lock);
+		INIT_LIST_HEAD(&clist->list);
+		refcount_set(&clist->ref_count, 1);
+		l2tp_session_coll_list_add(clist, session2);
+	}
+
+	/* If existing session isn't already in the session hlist, add it. */
+	if (!hash_hashed(&session2->hlist))
+		hash_add(pn->l2tp_v3_session_htable, &session2->hlist,
+			 session2->hlist_key);
+
+	/* Add new session to the hlist and collision list */
+	hash_add(pn->l2tp_v3_session_htable, &session1->hlist,
+		 session1->hlist_key);
+	refcount_inc(&clist->ref_count);
+	l2tp_session_coll_list_add(clist, session1);
+
+	return 0;
+}
+
+static void l2tp_session_collision_del(struct l2tp_net *pn,
+				       struct l2tp_session *session)
+{
+	struct l2tp_session_coll_list *clist = session->coll_list;
+	unsigned long session_key = session->session_id;
+	struct l2tp_session *session2;
+
+	lockdep_assert_held(&pn->l2tp_session_idr_lock);
+
+	hash_del(&session->hlist);
+
+	if (clist) {
+		/* Remove session from its collision list. If there
+		 * are other sessions with the same ID, replace this
+		 * session's IDR entry with that session, otherwise
+		 * remove the IDR entry. If this is the last session,
+		 * the collision list data is freed.
+		 */
+		spin_lock(&clist->lock);
+		list_del_init(&session->clist);
+		session2 = list_first_entry_or_null(&clist->list, struct l2tp_session, clist);
+		if (session2) {
+			void *old = idr_replace(&pn->l2tp_v3_session_idr, session2, session_key);
+
+			WARN_ON_ONCE(IS_ERR_VALUE(old));
+		} else {
+			void *removed = idr_remove(&pn->l2tp_v3_session_idr, session_key);
+
+			WARN_ON_ONCE(removed != session);
+		}
+		session->coll_list = NULL;
+		spin_unlock(&clist->lock);
+		if (refcount_dec_and_test(&clist->ref_count))
+			kfree(clist);
+		l2tp_session_dec_refcount(session);
+	}
+}
+
 int l2tp_session_register(struct l2tp_session *session,
 			  struct l2tp_tunnel *tunnel)
 {
+	struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
 	struct l2tp_session *session_walk;
-	struct hlist_head *g_head;
 	struct hlist_head *head;
-	struct l2tp_net *pn;
+	u32 session_key;
 	int err;
 
 	head = l2tp_session_id_hash(tunnel, session->session_id);
@@ -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;
 	}
 
+	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);
 
@@ -1218,13 +1326,19 @@ static void l2tp_session_unhash(struct l2tp_session *session)
 		hlist_del_init_rcu(&session->hlist);
 		spin_unlock_bh(&tunnel->hlist_lock);
 
-		/* For L2TPv3 we have a per-net hash: remove from there, too */
-		if (tunnel->version != L2TP_HDR_VER_2) {
+		/* For L2TPv3 we have a per-net IDR: remove from there, too */
+		if (tunnel->version == L2TP_HDR_VER_3) {
 			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
-
-			spin_lock_bh(&pn->l2tp_session_hlist_lock);
-			hlist_del_init_rcu(&session->global_hlist);
-			spin_unlock_bh(&pn->l2tp_session_hlist_lock);
+			struct l2tp_session *removed = session;
+
+			spin_lock_bh(&pn->l2tp_session_idr_lock);
+			if (hash_hashed(&session->hlist))
+				l2tp_session_collision_del(pn, session);
+			else
+				removed = idr_remove(&pn->l2tp_v3_session_idr,
+						     session->session_id);
+			WARN_ON_ONCE(removed && removed != session);
+			spin_unlock_bh(&pn->l2tp_session_idr_lock);
 		}
 
 		synchronize_rcu();
@@ -1649,8 +1763,9 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 
 		skb_queue_head_init(&session->reorder_q);
 
+		session->hlist_key = l2tp_v3_session_hashkey(tunnel->sock, session->session_id);
 		INIT_HLIST_NODE(&session->hlist);
-		INIT_HLIST_NODE(&session->global_hlist);
+		INIT_LIST_HEAD(&session->clist);
 
 		if (cfg) {
 			session->pwtype = cfg->pw_type;
@@ -1683,15 +1798,12 @@ EXPORT_SYMBOL_GPL(l2tp_session_create);
 static __net_init int l2tp_init_net(struct net *net)
 {
 	struct l2tp_net *pn = net_generic(net, l2tp_net_id);
-	int hash;
 
 	idr_init(&pn->l2tp_tunnel_idr);
 	spin_lock_init(&pn->l2tp_tunnel_idr_lock);
 
-	for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++)
-		INIT_HLIST_HEAD(&pn->l2tp_session_hlist[hash]);
-
-	spin_lock_init(&pn->l2tp_session_hlist_lock);
+	idr_init(&pn->l2tp_v3_session_idr);
+	spin_lock_init(&pn->l2tp_session_idr_lock);
 
 	return 0;
 }
@@ -1701,7 +1813,6 @@ static __net_exit void l2tp_exit_net(struct net *net)
 	struct l2tp_net *pn = l2tp_pernet(net);
 	struct l2tp_tunnel *tunnel = NULL;
 	unsigned long tunnel_id, tmp;
-	int hash;
 
 	rcu_read_lock_bh();
 	idr_for_each_entry_ul(&pn->l2tp_tunnel_idr, tunnel, tmp, tunnel_id) {
@@ -1714,8 +1825,7 @@ static __net_exit void l2tp_exit_net(struct net *net)
 		flush_workqueue(l2tp_wq);
 	rcu_barrier();
 
-	for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++)
-		WARN_ON_ONCE(!hlist_empty(&pn->l2tp_session_hlist[hash]));
+	idr_destroy(&pn->l2tp_v3_session_idr);
 	idr_destroy(&pn->l2tp_tunnel_idr);
 }
 
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 54dfba1eb91c..bfccc4ca2644 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -23,10 +23,6 @@
 #define L2TP_HASH_BITS	4
 #define L2TP_HASH_SIZE	BIT(L2TP_HASH_BITS)
 
-/* System-wide session hash table size */
-#define L2TP_HASH_BITS_2	8
-#define L2TP_HASH_SIZE_2	BIT(L2TP_HASH_BITS_2)
-
 struct sk_buff;
 
 struct l2tp_stats {
@@ -61,6 +57,12 @@ struct l2tp_session_cfg {
 	char			*ifname;
 };
 
+struct l2tp_session_coll_list {
+	spinlock_t lock;	/* for access to list */
+	struct list_head list;
+	refcount_t ref_count;
+};
+
 /* Represents a session (pseudowire) instance.
  * Tracks runtime state including cookies, dataplane packet sequencing, and IO statistics.
  * Is linked into a per-tunnel session hashlist; and in the case of an L2TPv3 session into
@@ -88,8 +90,11 @@ struct l2tp_session {
 	u32			nr_oos;		/* NR of last OOS packet */
 	int			nr_oos_count;	/* for OOS recovery */
 	int			nr_oos_count_max;
-	struct hlist_node	hlist;		/* hash list node */
 	refcount_t		ref_count;
+	struct hlist_node	hlist;		/* per-net session hlist */
+	unsigned long		hlist_key;	/* key for session hlist */
+	struct l2tp_session_coll_list *coll_list; /* session collision list */
+	struct list_head	clist;		/* for coll_list */
 
 	char			name[L2TP_SESSION_NAME_MAX]; /* for logging */
 	char			ifname[IFNAMSIZ];
@@ -102,7 +107,6 @@ struct l2tp_session {
 	int			reorder_skip;	/* set if skip to next nr */
 	enum l2tp_pwtype	pwtype;
 	struct l2tp_stats	stats;
-	struct hlist_node	global_hlist;	/* global hash list node */
 
 	/* Session receive handler for data packets.
 	 * Each pseudowire implementation should implement this callback in order to
@@ -226,7 +230,7 @@ struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth);
 struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
 					     u32 session_id);
 
-struct l2tp_session *l2tp_session_get(const struct net *net, u32 session_id);
+struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id);
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth);
 struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname);
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 19c8cc5289d5..e48aa177d74c 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -140,7 +140,7 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 	}
 
 	/* Ok, this is a data packet. Lookup the session. */
-	session = l2tp_session_get(net, session_id);
+	session = l2tp_v3_session_get(net, NULL, session_id);
 	if (!session)
 		goto discard;
 
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 8780ec64f376..d217ff1f229e 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -150,7 +150,7 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 	}
 
 	/* Ok, this is a data packet. Lookup the session. */
-	session = l2tp_session_get(net, session_id);
+	session = l2tp_v3_session_get(net, NULL, session_id);
 	if (!session)
 		goto discard;
 
-- 
2.34.1

[PATCH net-next 3/8] l2tp: store l2tpv2 sessions in per-net IDR

[James Chapman]

L2TPv2 sessions are currently kept in a per-tunnel hashlist, keyed by
16-bit session_id. When handling received L2TPv2 packets, we need to
first derive the tunnel using the 16-bit tunnel_id or sock, then
lookup the session in a per-tunnel hlist using the 16-bit session_id.

We want to avoid using sk_user_data in the datapath and double lookups
on every packet. So instead, use a per-net IDR to hold L2TPv2
sessions, keyed by a 32-bit value derived from the 16-bit tunnel_id
and session_id. This will allow the L2TPv2 UDP receive datapath to
lookup a session with a single lookup without deriving the tunnel
first.

L2TPv2 sessions are held in their own IDR to avoid potential
key collisions with L2TPv3 sessions.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 70 ++++++++++++++++++++++++++++++++++----------
 net/l2tp/l2tp_core.h |  1 +
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index d6bffdb16466..6f30b347fd46 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -107,12 +107,18 @@ struct l2tp_net {
 	/* Lock for write access to l2tp_tunnel_idr */
 	spinlock_t l2tp_tunnel_idr_lock;
 	struct idr l2tp_tunnel_idr;
-	/* Lock for write access to l2tp_v3_session_idr/htable */
+	/* Lock for write access to l2tp_v[23]_session_idr/htable */
 	spinlock_t l2tp_session_idr_lock;
+	struct idr l2tp_v2_session_idr;
 	struct idr l2tp_v3_session_idr;
 	struct hlist_head l2tp_v3_session_htable[16];
 };
 
+static inline u32 l2tp_v2_session_key(u16 tunnel_id, u16 session_id)
+{
+	return ((u32)tunnel_id) << 16 | session_id;
+}
+
 static inline unsigned long l2tp_v3_session_hashkey(struct sock *sk, u32 session_id)
 {
 	return ((unsigned long)sk) + session_id;
@@ -292,6 +298,24 @@ struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk,
 }
 EXPORT_SYMBOL_GPL(l2tp_v3_session_get);
 
+struct l2tp_session *l2tp_v2_session_get(const struct net *net, u16 tunnel_id, u16 session_id)
+{
+	u32 session_key = l2tp_v2_session_key(tunnel_id, session_id);
+	const struct l2tp_net *pn = l2tp_pernet(net);
+	struct l2tp_session *session;
+
+	rcu_read_lock_bh();
+	session = idr_find(&pn->l2tp_v2_session_idr, session_key);
+	if (session && refcount_inc_not_zero(&session->ref_count)) {
+		rcu_read_unlock_bh();
+		return session;
+	}
+	rcu_read_unlock_bh();
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(l2tp_v2_session_get);
+
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 {
 	int hash;
@@ -477,23 +501,32 @@ int l2tp_session_register(struct l2tp_session *session,
 			err = l2tp_session_collision_add(pn, session, session2);
 		}
 		spin_unlock_bh(&pn->l2tp_session_idr_lock);
-		if (err == -ENOSPC)
-			err = -EEXIST;
+	} else {
+		session_key = l2tp_v2_session_key(tunnel->tunnel_id,
+						  session->session_id);
+		spin_lock_bh(&pn->l2tp_session_idr_lock);
+		err = idr_alloc_u32(&pn->l2tp_v2_session_idr, NULL,
+				    &session_key, session_key, GFP_ATOMIC);
+		spin_unlock_bh(&pn->l2tp_session_idr_lock);
 	}
 
-	if (err)
+	if (err) {
+		if (err == -ENOSPC)
+			err = -EEXIST;
 		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);
+	spin_lock_bh(&pn->l2tp_session_idr_lock);
+	if (tunnel->version == L2TP_HDR_VER_3)
 		idr_replace(&pn->l2tp_v3_session_idr, session, session_key);
-		spin_unlock_bh(&pn->l2tp_session_idr_lock);
-	}
+	else
+		idr_replace(&pn->l2tp_v2_session_idr, session, session_key);
+	spin_unlock_bh(&pn->l2tp_session_idr_lock);
 
 	trace_register_session(session);
 
@@ -1321,25 +1354,30 @@ static void l2tp_session_unhash(struct l2tp_session *session)
 
 	/* Remove the session from core hashes */
 	if (tunnel) {
+		struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
+		struct l2tp_session *removed = session;
+
 		/* Remove from the per-tunnel hash */
 		spin_lock_bh(&tunnel->hlist_lock);
 		hlist_del_init_rcu(&session->hlist);
 		spin_unlock_bh(&tunnel->hlist_lock);
 
-		/* For L2TPv3 we have a per-net IDR: remove from there, too */
+		/* Remove from per-net IDR */
+		spin_lock_bh(&pn->l2tp_session_idr_lock);
 		if (tunnel->version == L2TP_HDR_VER_3) {
-			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
-			struct l2tp_session *removed = session;
-
-			spin_lock_bh(&pn->l2tp_session_idr_lock);
 			if (hash_hashed(&session->hlist))
 				l2tp_session_collision_del(pn, session);
 			else
 				removed = idr_remove(&pn->l2tp_v3_session_idr,
 						     session->session_id);
-			WARN_ON_ONCE(removed && removed != session);
-			spin_unlock_bh(&pn->l2tp_session_idr_lock);
+		} else {
+			u32 session_key = l2tp_v2_session_key(tunnel->tunnel_id,
+							      session->session_id);
+			removed = idr_remove(&pn->l2tp_v2_session_idr,
+					     session_key);
 		}
+		WARN_ON_ONCE(removed && removed != session);
+		spin_unlock_bh(&pn->l2tp_session_idr_lock);
 
 		synchronize_rcu();
 	}
@@ -1802,6 +1840,7 @@ static __net_init int l2tp_init_net(struct net *net)
 	idr_init(&pn->l2tp_tunnel_idr);
 	spin_lock_init(&pn->l2tp_tunnel_idr_lock);
 
+	idr_init(&pn->l2tp_v2_session_idr);
 	idr_init(&pn->l2tp_v3_session_idr);
 	spin_lock_init(&pn->l2tp_session_idr_lock);
 
@@ -1825,6 +1864,7 @@ static __net_exit void l2tp_exit_net(struct net *net)
 		flush_workqueue(l2tp_wq);
 	rcu_barrier();
 
+	idr_destroy(&pn->l2tp_v2_session_idr);
 	idr_destroy(&pn->l2tp_v3_session_idr);
 	idr_destroy(&pn->l2tp_tunnel_idr);
 }
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index bfccc4ca2644..d80f15f5b9fc 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -231,6 +231,7 @@ struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
 					     u32 session_id);
 
 struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id);
+struct l2tp_session *l2tp_v2_session_get(const struct net *net, u16 tunnel_id, u16 session_id);
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth);
 struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname);
-- 
2.34.1

[PATCH net-next 4/8] l2tp: refactor udp recv to lookup to not use sk_user_data

[James Chapman]

Modify UDP decap to not use the tunnel pointer which comes from the
sock's sk_user_data when parsing the L2TP header. By looking up the
destination session using only the packet contents we avoid potential
UDP 5-tuple aliasing issues which arise from depending on the socket
that received the packet.

Drop the useless error messages on short packet or on failing to find
a session since the tunnel pointer might point to a different tunnel
if multiple sockets use the same 5-tuple.

Short packets (those not big enough to contain an L2TP header) are no
longer counted in the tunnel's invalid counter because we can't derive
the tunnel until we parse the l2tp header to lookup the session.

l2tp_udp_encap_recv was a small wrapper around l2tp_udp_recv_core which
used sk_user_data to derive a tunnel pointer in an RCU-safe way. But
we no longer need the tunnel pointer, so remove that code and combine
the two functions.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 96 ++++++++++----------------------------------
 1 file changed, 21 insertions(+), 75 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6f30b347fd46..2c6378a9f384 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -926,19 +926,14 @@ static void l2tp_session_queue_purge(struct l2tp_session *session)
 	}
 }
 
-/* Internal UDP receive frame. Do the real work of receiving an L2TP data frame
- * here. The skb is not on a list when we get here.
- * Returns 0 if the packet was a data packet and was successfully passed on.
- * Returns 1 if the packet was not a good data packet and could not be
- * forwarded.  All such packets are passed up to userspace to deal with.
- */
-static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
+/* UDP encapsulation receive handler. See net/ipv4/udp.c for details. */
+int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct l2tp_session *session = NULL;
-	struct l2tp_tunnel *orig_tunnel = tunnel;
+	struct l2tp_tunnel *tunnel = NULL;
+	struct net *net = sock_net(sk);
 	unsigned char *ptr, *optr;
 	u16 hdrflags;
-	u32 tunnel_id, session_id;
 	u16 version;
 	int length;
 
@@ -948,11 +943,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	__skb_pull(skb, sizeof(struct udphdr));
 
 	/* Short packet? */
-	if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX)) {
-		pr_debug_ratelimited("%s: recv short packet (len=%d)\n",
-				     tunnel->name, skb->len);
-		goto invalid;
-	}
+	if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX))
+		goto pass;
 
 	/* Point to L2TP header */
 	optr = skb->data;
@@ -975,6 +967,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	ptr += 2;
 
 	if (version == L2TP_HDR_VER_2) {
+		u16 tunnel_id, session_id;
+
 		/* If length is present, skip it */
 		if (hdrflags & L2TP_HDRFLAG_L)
 			ptr += 2;
@@ -982,49 +976,35 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 		/* Extract tunnel and session ID */
 		tunnel_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
-
-		if (tunnel_id != tunnel->tunnel_id) {
-			/* We are receiving trafic for another tunnel, probably
-			 * because we have several tunnels between the same
-			 * IP/port quadruple, look it up.
-			 */
-			struct l2tp_tunnel *alt_tunnel;
-
-			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
-			if (!alt_tunnel)
-				goto pass;
-			tunnel = alt_tunnel;
-		}
-
 		session_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
+
+		session = l2tp_v2_session_get(net, tunnel_id, session_id);
 	} else {
+		u32 session_id;
+
 		ptr += 2;	/* skip reserved bits */
-		tunnel_id = tunnel->tunnel_id;
 		session_id = ntohl(*(__be32 *)ptr);
 		ptr += 4;
-	}
 
-	/* Check protocol version */
-	if (version != tunnel->version) {
-		pr_debug_ratelimited("%s: recv protocol version mismatch: got %d expected %d\n",
-				     tunnel->name, version, tunnel->version);
-		goto invalid;
+		session = l2tp_v3_session_get(net, sk, session_id);
 	}
 
-	/* Find the session context */
-	session = l2tp_tunnel_get_session(tunnel, session_id);
 	if (!session || !session->recv_skb) {
 		if (session)
 			l2tp_session_dec_refcount(session);
 
 		/* Not found? Pass to userspace to deal with */
-		pr_debug_ratelimited("%s: no session found (%u/%u). Passing up.\n",
-				     tunnel->name, tunnel_id, session_id);
 		goto pass;
 	}
 
-	if (tunnel->version == L2TP_HDR_VER_3 &&
+	tunnel = session->tunnel;
+
+	/* Check protocol version */
+	if (version != tunnel->version)
+		goto invalid;
+
+	if (version == L2TP_HDR_VER_3 &&
 	    l2tp_v3_ensure_opt_in_linear(session, skb, &ptr, &optr)) {
 		l2tp_session_dec_refcount(session);
 		goto invalid;
@@ -1033,9 +1013,6 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
 	l2tp_session_dec_refcount(session);
 
-	if (tunnel != orig_tunnel)
-		l2tp_tunnel_dec_refcount(tunnel);
-
 	return 0;
 
 invalid:
@@ -1045,42 +1022,11 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	/* Put UDP header back */
 	__skb_push(skb, sizeof(struct udphdr));
 
-	if (tunnel != orig_tunnel)
-		l2tp_tunnel_dec_refcount(tunnel);
-
-	return 1;
-}
-
-/* UDP encapsulation receive and error receive handlers.
- * See net/ipv4/udp.c for details.
- *
- * Note that these functions are called from inside an
- * RCU-protected region, but without the socket being locked.
- *
- * Hence we use rcu_dereference_sk_user_data to access the
- * tunnel data structure rather the usual l2tp_sk_to_tunnel
- * accessor function.
- */
-int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
-{
-	struct l2tp_tunnel *tunnel;
-
-	tunnel = rcu_dereference_sk_user_data(sk);
-	if (!tunnel)
-		goto pass_up;
-	if (WARN_ON(tunnel->magic != L2TP_TUNNEL_MAGIC))
-		goto pass_up;
-
-	if (l2tp_udp_recv_core(tunnel, skb))
-		goto pass_up;
-
-	return 0;
-
-pass_up:
 	return 1;
 }
 EXPORT_SYMBOL_GPL(l2tp_udp_encap_recv);
 
+/* UDP encapsulation receive error handler. See net/ipv4/udp.c for details. */
 static void l2tp_udp_encap_err_recv(struct sock *sk, struct sk_buff *skb, int err,
 				    __be16 port, u32 info, u8 *payload)
 {
-- 
2.34.1

[PATCH net-next 5/8] l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv

[James Chapman]

If UDP sockets are aliased, sk might be the wrong socket. There's no
benefit to using sk_user_data to do some checks on the associated
tunnel context. Just report the error anyway, like udp core does.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 2c6378a9f384..cbc5de1373cd 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1030,12 +1030,6 @@ EXPORT_SYMBOL_GPL(l2tp_udp_encap_recv);
 static void l2tp_udp_encap_err_recv(struct sock *sk, struct sk_buff *skb, int err,
 				    __be16 port, u32 info, u8 *payload)
 {
-	struct l2tp_tunnel *tunnel;
-
-	tunnel = rcu_dereference_sk_user_data(sk);
-	if (!tunnel || tunnel->fd < 0)
-		return;
-
 	sk->sk_err = err;
 	sk_error_report(sk);
 
-- 
2.34.1

[PATCH net-next 6/8] l2tp: use IDR for all session lookups

[James Chapman]

Add generic session getter which uses IDR. Replace all users of
l2tp_tunnel_get_session which uses the per-tunnel session list to use
the generic getter.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 10 ++++++++++
 net/l2tp/l2tp_core.h    |  2 ++
 net/l2tp/l2tp_netlink.c |  6 ++++--
 net/l2tp/l2tp_ppp.c     |  6 ++++--
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index cbc5de1373cd..0e826a0260fe 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -316,6 +316,16 @@ struct l2tp_session *l2tp_v2_session_get(const struct net *net, u16 tunnel_id, u
 }
 EXPORT_SYMBOL_GPL(l2tp_v2_session_get);
 
+struct l2tp_session *l2tp_session_get(const struct net *net, struct sock *sk, int pver,
+				      u32 tunnel_id, u32 session_id)
+{
+	if (pver == L2TP_HDR_VER_2)
+		return l2tp_v2_session_get(net, tunnel_id, session_id);
+	else
+		return l2tp_v3_session_get(net, sk, session_id);
+}
+EXPORT_SYMBOL_GPL(l2tp_session_get);
+
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 {
 	int hash;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index d80f15f5b9fc..0e7c9b0bcc1e 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -232,6 +232,8 @@ struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
 
 struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id);
 struct l2tp_session *l2tp_v2_session_get(const struct net *net, u16 tunnel_id, u16 session_id);
+struct l2tp_session *l2tp_session_get(const struct net *net, struct sock *sk, int pver,
+				      u32 tunnel_id, u32 session_id);
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth);
 struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname);
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index a901fd14fe3b..d105030520f9 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -61,7 +61,8 @@ static struct l2tp_session *l2tp_nl_session_get(struct genl_info *info)
 		session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
 		tunnel = l2tp_tunnel_get(net, tunnel_id);
 		if (tunnel) {
-			session = l2tp_tunnel_get_session(tunnel, session_id);
+			session = l2tp_session_get(net, tunnel->sock, tunnel->version,
+						   tunnel_id, session_id);
 			l2tp_tunnel_dec_refcount(tunnel);
 		}
 	}
@@ -635,7 +636,8 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 							   &cfg);
 
 	if (ret >= 0) {
-		session = l2tp_tunnel_get_session(tunnel, session_id);
+		session = l2tp_session_get(net, tunnel->sock, tunnel->version,
+					   tunnel_id, session_id);
 		if (session) {
 			ret = l2tp_session_notify(&l2tp_nl_family, info, session,
 						  L2TP_CMD_SESSION_CREATE);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 6146e4e67bbb..3596290047b2 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -753,7 +753,8 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 	if (tunnel->peer_tunnel_id == 0)
 		tunnel->peer_tunnel_id = info.peer_tunnel_id;
 
-	session = l2tp_tunnel_get_session(tunnel, info.session_id);
+	session = l2tp_session_get(sock_net(sk), tunnel->sock, tunnel->version,
+				   info.tunnel_id, info.session_id);
 	if (session) {
 		drop_refcnt = true;
 
@@ -1045,7 +1046,8 @@ static int pppol2tp_tunnel_copy_stats(struct pppol2tp_ioc_stats *stats,
 	/* If session_id is set, search the corresponding session in the
 	 * context of this tunnel and record the session's statistics.
 	 */
-	session = l2tp_tunnel_get_session(tunnel, stats->session_id);
+	session = l2tp_session_get(tunnel->l2tp_net, tunnel->sock, tunnel->version,
+				   tunnel->tunnel_id, stats->session_id);
 	if (!session)
 		return -EBADR;
 
-- 
2.34.1

[PATCH net-next 7/8] l2tp: drop the now unused l2tp_tunnel_get_session

[James Chapman]

All users of l2tp_tunnel_get_session are now gone so it can be
removed.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 22 ----------------------
 net/l2tp/l2tp_core.h |  2 --
 2 files changed, 24 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 0e826a0260fe..3ce689331542 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -241,28 +241,6 @@ struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth)
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_get_nth);
 
-struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
-					     u32 session_id)
-{
-	struct hlist_head *session_list;
-	struct l2tp_session *session;
-
-	session_list = l2tp_session_id_hash(tunnel, session_id);
-
-	rcu_read_lock_bh();
-	hlist_for_each_entry_rcu(session, session_list, hlist)
-		if (session->session_id == session_id) {
-			l2tp_session_inc_refcount(session);
-			rcu_read_unlock_bh();
-
-			return session;
-		}
-	rcu_read_unlock_bh();
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(l2tp_tunnel_get_session);
-
 struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id)
 {
 	const struct l2tp_net *pn = l2tp_pernet(net);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 0e7c9b0bcc1e..bfff69f2e0a2 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -227,8 +227,6 @@ void l2tp_session_dec_refcount(struct l2tp_session *session);
  */
 struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
 struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth);
-struct l2tp_session *l2tp_tunnel_get_session(struct l2tp_tunnel *tunnel,
-					     u32 session_id);
 
 struct l2tp_session *l2tp_v3_session_get(const struct net *net, struct sock *sk, u32 session_id);
 struct l2tp_session *l2tp_v2_session_get(const struct net *net, u16 tunnel_id, u16 session_id);
-- 
2.34.1

[PATCH net-next 8/8] l2tp: replace hlist with simple list for per-tunnel session list

[James Chapman]

The per-tunnel session list is no longer used by the
datapath. However, we still need a list of sessions in the tunnel for
l2tp_session_get_nth, which is used by management code. (An
alternative might be to walk each session IDR list, matching only
sessions of a given tunnel.)

Replace the per-tunnel hlist with a per-tunnel list. In functions
which walk a list of sessions of a tunnel, walk this list instead.

Signed-off-by: James Chapman <jchapman@katalix.com>
Reviewed-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c    | 109 ++++++++++++++--------------------------
 net/l2tp/l2tp_core.h    |  19 +++----
 net/l2tp/l2tp_debugfs.c |  13 ++---
 3 files changed, 50 insertions(+), 91 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 3ce689331542..be4bcbf291a1 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -39,7 +39,6 @@
 #include <linux/ip.h>
 #include <linux/udp.h>
 #include <linux/l2tp.h>
-#include <linux/hash.h>
 #include <linux/sort.h>
 #include <linux/file.h>
 #include <linux/nsproxy.h>
@@ -137,18 +136,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)
 	return net_generic(net, l2tp_net_id);
 }
 
-/* Session hash list.
- * The session_id SHOULD be random according to RFC2661, but several
- * L2TP implementations (Cisco and Microsoft) use incrementing
- * session_ids.  So we do a real hash on the session_id, rather than a
- * simple bitmask.
- */
-static inline struct hlist_head *
-l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id)
-{
-	return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];
-}
-
 static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 {
 	trace_free_tunnel(tunnel);
@@ -306,21 +293,17 @@ EXPORT_SYMBOL_GPL(l2tp_session_get);
 
 struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 {
-	int hash;
 	struct l2tp_session *session;
 	int count = 0;
 
 	rcu_read_lock_bh();
-	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
-		hlist_for_each_entry_rcu(session, &tunnel->session_hlist[hash], hlist) {
-			if (++count > nth) {
-				l2tp_session_inc_refcount(session);
-				rcu_read_unlock_bh();
-				return session;
-			}
+	list_for_each_entry_rcu(session, &tunnel->session_list, list) {
+		if (++count > nth) {
+			l2tp_session_inc_refcount(session);
+			rcu_read_unlock_bh();
+			return session;
 		}
 	}
-
 	rcu_read_unlock_bh();
 
 	return NULL;
@@ -334,21 +317,23 @@ struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 						const char *ifname)
 {
 	struct l2tp_net *pn = l2tp_pernet(net);
-	unsigned long session_id, tmp;
+	unsigned long tunnel_id, tmp;
 	struct l2tp_session *session;
+	struct l2tp_tunnel *tunnel;
 
 	rcu_read_lock_bh();
-	idr_for_each_entry_ul(&pn->l2tp_v3_session_idr, session, tmp, session_id) {
-		if (session) {
-			if (!strcmp(session->ifname, ifname)) {
-				l2tp_session_inc_refcount(session);
-				rcu_read_unlock_bh();
-
-				return session;
+	idr_for_each_entry_ul(&pn->l2tp_tunnel_idr, tunnel, tmp, tunnel_id) {
+		if (tunnel) {
+			list_for_each_entry_rcu(session, &tunnel->session_list, list) {
+				if (!strcmp(session->ifname, ifname)) {
+					l2tp_session_inc_refcount(session);
+					rcu_read_unlock_bh();
+
+					return session;
+				}
 			}
 		}
 	}
-
 	rcu_read_unlock_bh();
 
 	return NULL;
@@ -452,25 +437,15 @@ int l2tp_session_register(struct l2tp_session *session,
 			  struct l2tp_tunnel *tunnel)
 {
 	struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
-	struct l2tp_session *session_walk;
-	struct hlist_head *head;
 	u32 session_key;
 	int err;
 
-	head = l2tp_session_id_hash(tunnel, session->session_id);
-
-	spin_lock_bh(&tunnel->hlist_lock);
+	spin_lock_bh(&tunnel->list_lock);
 	if (!tunnel->acpt_newsess) {
 		err = -ENODEV;
 		goto err_tlock;
 	}
 
-	hlist_for_each_entry(session_walk, head, hlist)
-		if (session_walk->session_id == session->session_id) {
-			err = -EEXIST;
-			goto err_tlock;
-		}
-
 	if (tunnel->version == L2TP_HDR_VER_3) {
 		session_key = session->session_id;
 		spin_lock_bh(&pn->l2tp_session_idr_lock);
@@ -506,8 +481,8 @@ int l2tp_session_register(struct l2tp_session *session,
 
 	l2tp_tunnel_inc_refcount(tunnel);
 
-	hlist_add_head_rcu(&session->hlist, head);
-	spin_unlock_bh(&tunnel->hlist_lock);
+	list_add(&session->list, &tunnel->session_list);
+	spin_unlock_bh(&tunnel->list_lock);
 
 	spin_lock_bh(&pn->l2tp_session_idr_lock);
 	if (tunnel->version == L2TP_HDR_VER_3)
@@ -521,7 +496,7 @@ int l2tp_session_register(struct l2tp_session *session,
 	return 0;
 
 err_tlock:
-	spin_unlock_bh(&tunnel->hlist_lock);
+	spin_unlock_bh(&tunnel->list_lock);
 
 	return err;
 }
@@ -1275,20 +1250,19 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 	return;
 }
 
-/* Remove an l2tp session from l2tp_core's hash lists. */
+/* Remove an l2tp session from l2tp_core's lists. */
 static void l2tp_session_unhash(struct l2tp_session *session)
 {
 	struct l2tp_tunnel *tunnel = session->tunnel;
 
-	/* Remove the session from core hashes */
 	if (tunnel) {
 		struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
 		struct l2tp_session *removed = session;
 
-		/* Remove from the per-tunnel hash */
-		spin_lock_bh(&tunnel->hlist_lock);
-		hlist_del_init_rcu(&session->hlist);
-		spin_unlock_bh(&tunnel->hlist_lock);
+		/* Remove from the per-tunnel list */
+		spin_lock_bh(&tunnel->list_lock);
+		list_del_init(&session->list);
+		spin_unlock_bh(&tunnel->list_lock);
 
 		/* Remove from per-net IDR */
 		spin_lock_bh(&pn->l2tp_session_idr_lock);
@@ -1316,28 +1290,19 @@ static void l2tp_session_unhash(struct l2tp_session *session)
 static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 {
 	struct l2tp_session *session;
-	int hash;
+	struct list_head __rcu *pos;
+	struct list_head *tmp;
 
-	spin_lock_bh(&tunnel->hlist_lock);
+	spin_lock_bh(&tunnel->list_lock);
 	tunnel->acpt_newsess = false;
-	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
-again:
-		hlist_for_each_entry_rcu(session, &tunnel->session_hlist[hash], hlist) {
-			hlist_del_init_rcu(&session->hlist);
-
-			spin_unlock_bh(&tunnel->hlist_lock);
-			l2tp_session_delete(session);
-			spin_lock_bh(&tunnel->hlist_lock);
-
-			/* Now restart from the beginning of this hash
-			 * chain.  We always remove a session from the
-			 * list so we are guaranteed to make forward
-			 * progress.
-			 */
-			goto again;
-		}
+	list_for_each_safe(pos, tmp, &tunnel->session_list) {
+		session = list_entry(pos, struct l2tp_session, list);
+		list_del_init(&session->list);
+		spin_unlock_bh(&tunnel->list_lock);
+		l2tp_session_delete(session);
+		spin_lock_bh(&tunnel->list_lock);
 	}
-	spin_unlock_bh(&tunnel->hlist_lock);
+	spin_unlock_bh(&tunnel->list_lock);
 }
 
 /* Tunnel socket destroy hook for UDP encapsulation */
@@ -1531,8 +1496,9 @@ int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
 
 	tunnel->magic = L2TP_TUNNEL_MAGIC;
 	sprintf(&tunnel->name[0], "tunl %u", tunnel_id);
-	spin_lock_init(&tunnel->hlist_lock);
+	spin_lock_init(&tunnel->list_lock);
 	tunnel->acpt_newsess = true;
+	INIT_LIST_HEAD(&tunnel->session_list);
 
 	tunnel->encap = encap;
 
@@ -1732,6 +1698,7 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 		session->hlist_key = l2tp_v3_session_hashkey(tunnel->sock, session->session_id);
 		INIT_HLIST_NODE(&session->hlist);
 		INIT_LIST_HEAD(&session->clist);
+		INIT_LIST_HEAD(&session->list);
 
 		if (cfg) {
 			session->pwtype = cfg->pw_type;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index bfff69f2e0a2..8ac81bc1bc6f 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -19,10 +19,6 @@
 #define L2TP_TUNNEL_MAGIC	0x42114DDA
 #define L2TP_SESSION_MAGIC	0x0C04EB7D
 
-/* Per tunnel session hash table size */
-#define L2TP_HASH_BITS	4
-#define L2TP_HASH_SIZE	BIT(L2TP_HASH_BITS)
-
 struct sk_buff;
 
 struct l2tp_stats {
@@ -65,8 +61,7 @@ struct l2tp_session_coll_list {
 
 /* Represents a session (pseudowire) instance.
  * Tracks runtime state including cookies, dataplane packet sequencing, and IO statistics.
- * Is linked into a per-tunnel session hashlist; and in the case of an L2TPv3 session into
- * an additional per-net ("global") hashlist.
+ * Is linked into a per-tunnel session list and a per-net ("global") IDR tree.
  */
 #define L2TP_SESSION_NAME_MAX 32
 struct l2tp_session {
@@ -90,6 +85,7 @@ struct l2tp_session {
 	u32			nr_oos;		/* NR of last OOS packet */
 	int			nr_oos_count;	/* for OOS recovery */
 	int			nr_oos_count_max;
+	struct list_head	list;		/* per-tunnel list node */
 	refcount_t		ref_count;
 	struct hlist_node	hlist;		/* per-net session hlist */
 	unsigned long		hlist_key;	/* key for session hlist */
@@ -118,7 +114,7 @@ struct l2tp_session {
 	/* Session close handler.
 	 * Each pseudowire implementation may implement this callback in order to carry
 	 * out pseudowire-specific shutdown actions.
-	 * The callback is called by core after unhashing the session and purging its
+	 * The callback is called by core after unlisting the session and purging its
 	 * reorder queue.
 	 */
 	void (*session_close)(struct l2tp_session *session);
@@ -154,7 +150,7 @@ struct l2tp_tunnel_cfg {
 /* Represents a tunnel instance.
  * Tracks runtime state including IO statistics.
  * Holds the tunnel socket (either passed from userspace or directly created by the kernel).
- * Maintains a hashlist of sessions belonging to the tunnel instance.
+ * Maintains a list of sessions belonging to the tunnel instance.
  * Is linked into a per-net list of tunnels.
  */
 #define L2TP_TUNNEL_NAME_MAX 20
@@ -164,12 +160,11 @@ struct l2tp_tunnel {
 	unsigned long		dead;
 
 	struct rcu_head rcu;
-	spinlock_t		hlist_lock;	/* write-protection for session_hlist */
+	spinlock_t		list_lock;	/* write-protection for session_list */
 	bool			acpt_newsess;	/* indicates whether this tunnel accepts
-						 * new sessions. Protected by hlist_lock.
+						 * new sessions. Protected by list_lock.
 						 */
-	struct hlist_head	session_hlist[L2TP_HASH_SIZE];
-						/* hashed list of sessions, hashed by id */
+	struct list_head	session_list;	/* list of sessions */
 	u32			tunnel_id;
 	u32			peer_tunnel_id;
 	int			version;	/* 2=>L2TPv2, 3=>L2TPv3 */
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index 4595b56d175d..8755ae521154 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -123,17 +123,14 @@ static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)
 	struct l2tp_tunnel *tunnel = v;
 	struct l2tp_session *session;
 	int session_count = 0;
-	int hash;
 
 	rcu_read_lock_bh();
-	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
-		hlist_for_each_entry_rcu(session, &tunnel->session_hlist[hash], hlist) {
-			/* Session ID of zero is a dummy/reserved value used by pppol2tp */
-			if (session->session_id == 0)
-				continue;
+	list_for_each_entry_rcu(session, &tunnel->session_list, list) {
+		/* Session ID of zero is a dummy/reserved value used by pppol2tp */
+		if (session->session_id == 0)
+			continue;
 
-			session_count++;
-		}
+		session_count++;
 	}
 	rcu_read_unlock_bh();
 
-- 
2.34.1

Re: [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 20 Jun 2024 12:22:36 +0100 you wrote:
> This series refactors l2tp to not use the tunnel socket's sk_user_data
> in the datapath. The main reasons for doing this are
> 
>  * to allow for simplifying internal socket cleanup code (to be done
>    in a later series)
>  * to support multiple L2TPv3 UDP tunnels using the same 5-tuple
>    address
> 
> [...]

Here is the summary with links:
  - [net-next,1/8] l2tp: remove unused list_head member in l2tp_tunnel
    https://git.kernel.org/netdev/net-next/c/a744e2d03a91
  - [net-next,2/8] l2tp: store l2tpv3 sessions in per-net IDR
    https://git.kernel.org/netdev/net-next/c/aa5e17e1f5ec
  - [net-next,3/8] l2tp: store l2tpv2 sessions in per-net IDR
    https://git.kernel.org/netdev/net-next/c/2a3339f6c963
  - [net-next,4/8] l2tp: refactor udp recv to lookup to not use sk_user_data
    https://git.kernel.org/netdev/net-next/c/ff6a2ac23cb0
  - [net-next,5/8] l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv
    https://git.kernel.org/netdev/net-next/c/c37e0138ca5f
  - [net-next,6/8] l2tp: use IDR for all session lookups
    https://git.kernel.org/netdev/net-next/c/5f77c18ea556
  - [net-next,7/8] l2tp: drop the now unused l2tp_tunnel_get_session
    https://git.kernel.org/netdev/net-next/c/8c6245af4fc5
  - [net-next,8/8] l2tp: replace hlist with simple list for per-tunnel session list
    https://git.kernel.org/netdev/net-next/c/d18d3f0a24fc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Simon Horman]

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);
>  

...

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

[James Chapman]

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@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);
>>   
> 
> ...

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

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

[Simon Horman]

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@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);
> > 
> > ...
> 
> 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.