Message-ID: <2e75a78b-afa2-3776-2695-f9f6ac93eb67@universe-factory.net>
Date:   Sat, 20 Feb 2021 10:56:33 +0100
From:   Matthias Schiffer <mschiffer@...verse-factory.net>
To:     Tom Parkin <tparkin@...alix.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: l2tp: reduce log level when passing up invalid
 packets

Hi Tom,

On 2/19/21 9:12 PM, Tom Parkin wrote:
> Hi Matthias,
> 
> Thanks for your patch!
> 
> 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.


> 
>> 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.


[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@...verse-factory.net>



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




Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists