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 > > > > > > --- > > > > > > 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; > > > }