Re: [PATCH net] net: l2tp: reduce log level when passing up invalid packets

[Tom Parkin]

On  Sat, Feb 20, 2021 at 10:56:33 +0100, Matthias Schiffer wrote:
> On 2/19/21 9:12 PM, Tom Parkin wrote:
> > On  Fri, Feb 19, 2021 at 20:06:15 +0100, Matthias Schiffer wrote:
> > > Before commit 5ee759cda51b ("l2tp: use standard API for warning log
> > > messages"), it was possible for userspace applications to use their own
> > > control protocols on the backing sockets of an L2TP kernel device, and as
> > > long as a packet didn't look like a proper L2TP data packet, it would be
> > > passed up to userspace just fine.
> > 
> > Hum.  I appreciate we're now logging where we previously were not, but
> > I would say these warning messages are valid.
> > 
> > It's still perfectly possible to use the L2TP socket for the L2TP control
> > protocol: packets per the RFCs won't trigger these warnings to the
> > best of my knowledge, although I'm happy to be proven wrong!
> > 
> > I wonder whether your application is sending non-L2TP packets over the
> > L2TP socket?  Could you describe the usecase?
> 
> I'm the developer of the UDP-based VPN/tunnel application fastd [1]. This is
> currently a pure userspace implementation, supporting both encrypted and
> unencrypted tunnels, with a protocol that doesn't look anything like L2TP.
> 
> To improve performance of unencrypted tunnels, I'm looking into using L2TP
> to offload the data plane to the kernel. Whether to make use of this would
> be negotiated in a session handshake (that is also used for key exchange in
> encrypted mode).
> 
> As the (IPv4) internet is stupid and everything is NATted, and I even want
> to support broken NAT routers that somehow break UDP hole punching, I use
> only a single socket for both control (handshake) and data packets.

Thanks for describing the usecase a bit more.  It looks an interesting
project.

To be honest I'm not sure the L2TP subsystem is a good match for
fastd's datapath offload though.

This is for the following reasons:

Firstly, the warnings referenced in your patch are early in the L2TP recv
path, called shortly after our hook into the UDP recv code.

So at this point, I don't believe the L2TP subsystem is really buying you
anything over a plain UPD recv.

Now, I'm perhaps reading too much into what you've said, but I imagine
that fastd using the L2TP tunnel context is just a first step.  I
assume the end goal for datapath offload would be to use the L2TP
pseudowire code in order to have session data appear from e.g. a
virtual Ethernet interface.  That way you get to avoid copying data
to userspace, and then stuffing it back into the kernel via. tun/tap.
And that makes sense to me as a goal!

However, if that is indeed the goal, I really can't see it working
without fastd's protocol being modified to look like L2TP.  (I also,
btw, can't see it working without some kind of kernel-space L2TP
subsystem support for fastd's various encryption protocols, but that's
another matter).

If you take a look at the session recv datapath from l2tp_recv_common
onwards you'll see that there is a lot of code you have to avoid
confusing in l2tp_core.c alone, even before you get into any
pseudowire-specifics.  I can't see that being possible with fastd's
current data packet format.

In summary, I think at this point in the L2TP recv path a normal UDP
socket would serve you just as well, and I can't see the L2TP subsystem
being useful as a data offload mechanism for fastd in the future
without effectively changing fastd's packet format to look like L2TP.

Secondly, given the above (and apologies if I've missed the mark); I
really wouldn't want to encourage you to use the L2TP subsystem for
future fastd improvements.

From fastd's perspective it is IMO a bad idea, since it would be easy
for a future (perfectly valid) change in the L2TP code to accidentally
break fastd.  And next time it might not be some easily-tweaked thing
like logging levels, but rather e.g. a security fix or bug fix which
cannot be undone.

From the L2TP subsystem's perspective it is a bad idea, since by
encouraging fastd to use the L2TP code, we end up hampering future L2TP
development in order to support a project which doesn't actually use
the L2TP protocol at all.

In the hope of being more constructive -- have you considered whether
tc and/or ebpf could be used for fastd?  As more generic mechanisms I
think you might get on better with these than trying to twist the L2TP
code's arm :-)

> > > After the mentioned change, this approach would lead to significant log
> > > spam, as the previously hidden warnings are now shown by default. Not
> > > even setting the T flag on the custom control packets is sufficient to
> > > surpress these warnings, as packet length and L2TP version are checked
> > > before the T flag.
> > 
> > Possibly we could sidestep some of these warnings by moving the T flag
> > check further up in the function.
> > 
> > The code would need to pull the first byte of the header, check the type
> > bit, and skip further processing if the bit was set.  Otherwise go on to
> > pull the rest of the header.
> > 
> > I think I'd prefer this approach assuming the warnings are not
> > triggered by valid L2TP messages.
> 
> This will not be sufficient for my usecase: To stay compatible with older
> versions of fastd, I can't set the T flag in the first packet of the
> handshake, as it won't be known whether the peer has a new enough fastd
> version to understand packets that have this bit set. Luckily, the second
> handshake byte is always 0 in fastd's protocol, so these packets fail the
> tunnel version check and are passed to userspace regardless.
> 
> I'm aware that this usecase is far outside of the original intentions of the
> code and can only be described as a hack, but I still consider this a
> regression in the kernel, as it was working fine in the past, without
> visible warnings.
> 

I'm sorry, but for the reasons stated above I disagree about it being
a regression.

> 
> [1] https://github.com/NeoRaider/fastd/
> 
> 
> > 
> > > 
> > > Reduce all warnings debug level when packets are passed to userspace.
> > > 
> > > Fixes: 5ee759cda51b ("l2tp: use standard API for warning log messages")
> > > Signed-off-by: Matthias Schiffer <mschiffer@xxxxxxxxxxxxxxxxxxxx>
> 
> 
> 
> > > ---
> > > 
> > > I'm unsure what to do about the pr_warn_ratelimited() in
> > > l2tp_recv_common(). It feels wrong to me that an incoming network packet
> > > can trigger a kernel message above debug level at all, so maybe they
> > > should be downgraded as well? I believe the only reason these were ever
> > > warnings is that they were not shown by default.
> > > 
> > > 
> > >   net/l2tp/l2tp_core.c | 12 ++++++------
> > >   1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index 7be5103ff2a8..40852488c62a 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -809,8 +809,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> > >   	/* Short packet? */
> > >   	if (!pskb_may_pull(skb, L2TP_HDR_SIZE_MAX)) {
> > > -		pr_warn_ratelimited("%s: recv short packet (len=%d)\n",
> > > -				    tunnel->name, skb->len);
> > > +		pr_debug_ratelimited("%s: recv short packet (len=%d)\n",
> > > +				     tunnel->name, skb->len);
> > >   		goto error;
> > >   	}
> > > @@ -824,8 +824,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> > >   	/* Check protocol version */
> > >   	version = hdrflags & L2TP_HDR_VER_MASK;
> > >   	if (version != tunnel->version) {
> > > -		pr_warn_ratelimited("%s: recv protocol version mismatch: got %d expected %d\n",
> > > -				    tunnel->name, version, tunnel->version);
> > > +		pr_debug_ratelimited("%s: recv protocol version mismatch: got %d expected %d\n",
> > > +				     tunnel->name, version, tunnel->version);
> > >   		goto error;
> > >   	}
> > > @@ -863,8 +863,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> > >   			l2tp_session_dec_refcount(session);
> > >   		/* Not found? Pass to userspace to deal with */
> > > -		pr_warn_ratelimited("%s: no session found (%u/%u). Passing up.\n",
> > > -				    tunnel->name, tunnel_id, session_id);
> > > +		pr_debug_ratelimited("%s: no session found (%u/%u). Passing up.\n",
> > > +				     tunnel->name, tunnel_id, session_id);
> > >   		goto error;
> > >   	}

Attachment: signature.asc
Description: PGP signature