* [PATCH net-next 1/8] l2tp: remove unused list_head member in l2tp_tunnel
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR James Chapman
` (7 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
2024-06-20 11:22 ` [PATCH net-next 1/8] l2tp: remove unused list_head member in l2tp_tunnel James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-21 12:59 ` Simon Horman
2024-06-20 11:22 ` [PATCH net-next 3/8] l2tp: store l2tpv2 " James Chapman
` (6 subsequent siblings)
8 siblings, 1 reply; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR
2024-06-20 11:22 ` [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR James Chapman
@ 2024-06-21 12:59 ` Simon Horman
2024-06-21 16:21 ` James Chapman
0 siblings, 1 reply; 13+ messages in thread
From: Simon Horman @ 2024-06-21 12:59 UTC (permalink / raw)
To: James Chapman; +Cc: netdev, gnault, samuel.thibault, ridge.kennedy
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);
>
...
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR
2024-06-21 12:59 ` Simon Horman
@ 2024-06-21 16:21 ` James Chapman
2024-06-23 7:42 ` Simon Horman
0 siblings, 1 reply; 13+ messages in thread
From: James Chapman @ 2024-06-21 16:21 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR
2024-06-21 16:21 ` James Chapman
@ 2024-06-23 7:42 ` Simon Horman
0 siblings, 0 replies; 13+ messages in thread
From: Simon Horman @ 2024-06-23 7:42 UTC (permalink / raw)
To: James Chapman; +Cc: netdev, gnault, samuel.thibault, ridge.kennedy
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 3/8] l2tp: store l2tpv2 sessions in per-net IDR
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
2024-06-20 11:22 ` [PATCH net-next 1/8] l2tp: remove unused list_head member in l2tp_tunnel James Chapman
2024-06-20 11:22 ` [PATCH net-next 2/8] l2tp: store l2tpv3 sessions in per-net IDR James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 4/8] l2tp: refactor udp recv to lookup to not use sk_user_data James Chapman
` (5 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 4/8] l2tp: refactor udp recv to lookup to not use sk_user_data
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (2 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 3/8] l2tp: store l2tpv2 " James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 5/8] l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv James Chapman
` (4 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 5/8] l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (3 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 4/8] l2tp: refactor udp recv to lookup to not use sk_user_data James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 6/8] l2tp: use IDR for all session lookups James Chapman
` (3 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 6/8] l2tp: use IDR for all session lookups
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (4 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 5/8] l2tp: don't use sk_user_data in l2tp_udp_encap_err_recv James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 7/8] l2tp: drop the now unused l2tp_tunnel_get_session James Chapman
` (2 subsequent siblings)
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 7/8] l2tp: drop the now unused l2tp_tunnel_get_session
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (5 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 6/8] l2tp: use IDR for all session lookups James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-20 11:22 ` [PATCH net-next 8/8] l2tp: replace hlist with simple list for per-tunnel session list James Chapman
2024-06-21 10:40 ` [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath patchwork-bot+netdevbpf
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH net-next 8/8] l2tp: replace hlist with simple list for per-tunnel session list
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (6 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 7/8] l2tp: drop the now unused l2tp_tunnel_get_session James Chapman
@ 2024-06-20 11:22 ` James Chapman
2024-06-21 10:40 ` [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath patchwork-bot+netdevbpf
8 siblings, 0 replies; 13+ messages in thread
From: James Chapman @ 2024-06-20 11:22 UTC (permalink / raw)
To: netdev; +Cc: gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath
2024-06-20 11:22 [PATCH net-next 0/8] l2tp: don't use the tunnel socket's sk_user_data in datapath James Chapman
` (7 preceding siblings ...)
2024-06-20 11:22 ` [PATCH net-next 8/8] l2tp: replace hlist with simple list for per-tunnel session list James Chapman
@ 2024-06-21 10:40 ` patchwork-bot+netdevbpf
8 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-06-21 10:40 UTC (permalink / raw)
To: James Chapman; +Cc: netdev, gnault, samuel.thibault, ridge.kennedy
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
^ permalink raw reply [flat|nested] 13+ messages in thread