Re: [PATCH] l2tp: Support several sockets with same IP/port quadruple

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Hello,

James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
> > @@ -845,6 +846,20 @@ 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 && tunnel->l2tp_net) {
> Can tunnel->l2tp_net be NULL?

l2tp_tunnel_sock_create's comment says

 * Since we don't want these sockets to keep a namespace alive by
 * themselves, we drop the socket's namespace refcount after creation.
 * These sockets are freed when the namespace exits using the pernet
 * exit hook.

and l2tp_tunnel_create does not set l2tp_net, only l2tp_tunnel_register
does, so I assumed it might be NULL and preferred to stay on
the safe side. But it's l2tp_tunnel_register which adds it to
pn->l2tp_tunnel_idr, so AIUI it indeed cannot be NULL since we got it
from pn->l2tp_tunnel_idr, we can probably drop the test indeed.

> > +			/* 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);
> This misses a check that alt_tunnel's protocol version matches the header.
> Move the existing header version check to after this fragment?

We need to check the version before getting the tunnel id, which we need
to look up the struct l2tp_tunnel :)

I'll add another version check.

Samuel

> > +			if (!alt_tunnel)
> > +				goto pass;
> > +			tunnel = alt_tunnel;
> > +		}
> > +
> >   		session_id = ntohs(*(__be16 *)ptr);
> >   		ptr += 2;
> >   	} else {