On Thu, 16 Jan 2020, Guillaume Nault wrote:

On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:

quoted

In the past it was possible to create multiple L2TPv3 sessions with the
same session id as long as the sessions belonged to different tunnels.
The resulting sessions had issues when used with IP encapsulated tunnels,
but worked fine with UDP encapsulated ones. Some applications began to
rely on this behaviour to avoid having to negotiate unique session ids.

Some time ago a change was made to require session ids to be unique across
all tunnels, breaking the applications making use of this "feature".

This change relaxes the duplicate session id check to allow duplicates
if both of the colliding sessions belong to UDP encapsulated tunnels.

Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
Signed-off-by: Ridge Kennedy <redacted>
---
 net/l2tp/l2tp_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index f82ea12bac37..0cc86227c618 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c

@@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
 		spin_lock_bh(&pn->l2tp_session_hlist_lock);

 		hlist_for_each_entry(session_walk, g_head, global_hlist)
-			if (session_walk->session_id == session->session_id) {
+			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;
 			}

Well, I think we have a more fundamental problem here. By adding
L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
sessions. That is, if we have an L2TPv3 session X running over UDP and
we receive an L2TP_IP packet targetted at session ID X, then
l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().

I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
look up the session in the context of its socket, like in the UDP case.

But for the moment, what about just not adding L2TP_UDP sessions to the
global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
cross-talks.

I did consider not adding the L2TP_UDP sessions to the global list, but 
that change looked a little more involved as the netlink interface was 
also making use of the list to lookup sessions by ifname. At a minimum
it looks like this would require rework of l2tp_session_get_by_ifname().