[PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Hengyu Liang]

Commit 5f77c18ea556 ("l2tp: use IDR for all session lookups") made
pppol2tp_connect() and the netlink session commands find sessions with
l2tp_session_get(), which takes a tunnel ID and a session ID.

However, for L2TPv3, l2tp_session_get() does not use the tunnel ID. If
no other session has the same session ID, it returns that session,
whichever tunnel it belongs to. As of now, a session command that names
the wrong tunnel acts on a session of another tunnel, and a PPPoL2TP
socket for a second L2TPv3 tunnel fails with EEXIST, because it finds
session 0 of the first tunnel.

The issue can be reproduced with:

    ip l2tp add tunnel tunnel_id 3 peer_tunnel_id 3 encap udp \
        local 127.0.0.1 remote 127.0.0.1 udp_sport 5003 udp_dport 6003
    ip l2tp add tunnel tunnel_id 4 peer_tunnel_id 4 encap udp \
        local 127.0.0.1 remote 127.0.0.1 udp_sport 5004 udp_dport 6004
    ip l2tp add session tunnel_id 3 session_id 7 peer_session_id 7
    ip l2tp del session tunnel_id 4 session_id 7
    ip l2tp show session

Before that commit, the delete fails with "No such device", and session
7 of tunnel 3 is still listed. After that commit, the delete succeeds,
and session 7 of tunnel 3 is gone.

This patch will make l2tp_session_get() return NULL when the L2TPv3
session it finds belongs to another tunnel.

Fixes: 5f77c18ea556 ("l2tp: use IDR for all session lookups")
Cc: stable@vger.kernel.org
Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
The first commit that shows the problem is d18d3f0a24fc ("l2tp: replace
hlist with simple list for per-tunnel session list"), two patches later
in the same series. Before it, these L2TPv3 lookups found no session at
all.

The receive path still looks up L2TPv3 sessions by session ID only. Only
the lookups that name a tunnel change.

Tested with the commands above, the same session ID in two UDP tunnels
(still allowed) and in two IP tunnels (still refused), and two PPPoL2TP
tunnel sockets.

 net/l2tp/l2tp_core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7e042a3e648c..f509750f80d5 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -315,10 +315,19 @@ 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)
 {
+	struct l2tp_session *session;
+
 	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);
+
+	/* An L2TPv3 session ID that has not collided may be in any tunnel */
+	session = l2tp_v3_session_get(net, sk, session_id);
+	if (session && session->tunnel->tunnel_id != tunnel_id) {
+		l2tp_session_put(session);
+		return NULL;
+	}
+
+	return session;
 }
 EXPORT_SYMBOL_GPL(l2tp_session_get);
 

base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
-- 
2.53.0

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Hengyu Liang]

Hi,

I found this while comparing the behaviour of different kernel versions:
connect() on a PPPoL2TP socket for a second L2TPv3 tunnel succeeds on
v6.8 but fails with EEXIST on v6.18. I then bisected it to the IDR
session lookup series (5f77c18ea556, d18d3f0a24fc) and checked the
"ip l2tp" commands in the commit message by hand on both sides.

Thanks,
Hengyu

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Guillaume Nault]

On Sun, Oct 25, 2026 at 10:58:31AM -0400, Hengyu Liang wrote:
> @@ -315,10 +315,19 @@ 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)
>  {
> +	struct l2tp_session *session;
> +
>  	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);
> +
> +	/* An L2TPv3 session ID that has not collided may be in any tunnel */
> +	session = l2tp_v3_session_get(net, sk, session_id);
> +	if (session && session->tunnel->tunnel_id != tunnel_id) {
> +		l2tp_session_put(session);
> +		return NULL;
> +	}
> +
> +	return session;
>  }

The original code looks correct to me. L2TPv3 sessions, unlike L2TPv2
ones, are global to all tunnels sharing the same socket.

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Hengyu Liang]

Hi Guillaume,

Thanks for looking at this.

I wasn't sure about the intended semantics. l2tp.rst says "L2TPv3
session ids are unique across all tunnels", but it also says Linux
allows the same ID in different tunnels, told apart by sk, and
l2tp_session_register() still allows that for UDP encap.

The patch only changes callers of l2tp_session_get() that pass a
tunnel_id, and the result differs from v6.8:

 - pppol2tp_connect() for session 5 in tunnel 2 fails with EEXIST if
   tunnel 1 (another UDP socket) already has a connected session 5.
 - "ip l2tp del session tunnel_id 4 session_id 7" deletes tunnel 3's
   session 7 when only tunnel 3 has it. On v6.8 it fails. l2tp.rst
   describes CONN_ID there as the parent tunnel of the session.

Am I misreading the documentation, is the documentation out of date, or
is this behaviour not intended?

Thanks,
Hengyu

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Guillaume Nault]

On Mon, Oct 26, 2026 at 08:57:02AM -0400, Hengyu Liang wrote:
> Hi Guillaume,
> 
> Thanks for looking at this.
> 
> I wasn't sure about the intended semantics. l2tp.rst says "L2TPv3
> session ids are unique across all tunnels", but it also says Linux
> allows the same ID in different tunnels, told apart by sk, and
> l2tp_session_register() still allows that for UDP encap.

My understanding is that L2TPv3 session IDs are unique across their
network namespace. There's an exception for those that are transported
over UDP, as they're now unique for a given UDP socket (the one of
their parent's tunnel).

As a result, looking up a UDP L2TPv3 session requires a socket and a
session ID. That works when demultiplexing an incomming packet, as we
have both information in this case. But that's a problem when selecting
a session from user space.

The correct approach would be to pass both a session ID and a socket
for any L2TPv3 UDP session lookup. But the netlink interface currently
doesn't support passing a socket fd for SESSION_GET, SESSION_MODIFY
and SESSION_DELETE operations. That could be done for managed tunnels
though.

However, for unmanaged tunnels, as used by iproute2, user space doesn't
know anything about the UDP socket. In that case, all we can do is to
replace the socket parameter with a tunnel ID. Then we'd have to loop
through all the sessions that match the requested session ID and return
the one whose tunnel's socket matches the socket of the tunnel ID
passed as parameter. This last operation could maybe be simplified to a
simple tunnel ID check, as, in my understanding, we have an 1:1
relationship between tunnel IDs and their sockets (but I'm a bit wary
of such shortcuts given that code have proved to be brittle in the past).

> The patch only changes callers of l2tp_session_get() that pass a
> tunnel_id, and the result differs from v6.8:
> 
>  - pppol2tp_connect() for session 5 in tunnel 2 fails with EEXIST if
>    tunnel 1 (another UDP socket) already has a connected session 5.
>  - "ip l2tp del session tunnel_id 4 session_id 7" deletes tunnel 3's
>    session 7 when only tunnel 3 has it. On v6.8 it fails. l2tp.rst
>    describes CONN_ID there as the parent tunnel of the session.
> Am I misreading the documentation, is the documentation out of date, or
> is this behaviour not intended?
> 
> Thanks,
> Hengyu
> 

Re: [PATCH net] l2tp: only return sessions of the given tunnel in l2tp_session_get()

[Hengyu Liang]

Hi Guillaume,

Thanks, that makes the intended semantics clear.

On Mon, Oct 26, 2026 at 05:55:07PM +0100, Guillaume Nault wrote:
> The correct approach would be to pass both a session ID and a socket
> for any L2TPv3 UDP session lookup. But the netlink interface currently
> doesn't support passing a socket fd for SESSION_GET, SESSION_MODIFY
> and SESSION_DELETE operations.

In the kernel the callers already have both. They look up the tunnel by
ID and pass tunnel->sock to l2tp_session_get(). The case that is missed
is a session ID that has not collided: l2tp_v3_session_get() returns the
IDR entry without checking sk.

> This last operation could maybe be simplified to a simple tunnel ID
> check, as, in my understanding, we have an 1:1 relationship between
> tunnel IDs and their sockets (but I'm a bit wary of such shortcuts
> given that code have proved to be brittle in the past).

In v2 I will compare the session's tunnel socket with sk instead of
comparing tunnel IDs, so the check does not depend on that mapping. It
does hold today: tunnel IDs are unique per netns, and
l2tp_validate_socket() returns -EBUSY for a socket that already has a
tunnel.

I will also reword the commit message to describe session IDs as you
did: unique per netns, and per UDP socket for UDP encap.

Do you have any other comments? If not, I will send v2 with these
changes.

Thanks,
Hengyu