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

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

[Matthias Schiffer]

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.

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.

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@universe-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;
 	}
 
-- 
2.30.1

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 3867 bytes --]

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?

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 5499 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 8583 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 10351 bytes --]

On 2/22/21 12:49 PM, Tom Parkin wrote:
> 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!

That is indeed what I want to achieve.

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

Only unencrypted connections would be offloaded.

fastd's data protocol will be modified to use L2TP Ethernet pseudowire as 
specified by the RFC (I actually finished the userspace implementation of 
this yesterday, in branch method-l2tp for now). Two peers can negotiate the 
protocol to use (called "method" in fastd terms) in the session handshake. 
A session would only be offloaded to kernel-space L2TP when both sides 
agree that they indeed want to use the L2TP method; otherwise fastd will 
continue to use TUN/TAP.

The only part of the fastd protocol that I can't modify arbitrarily is the 
first packet of the handshake, as it must be compatible with older versions 
of fastd. There may be a point when I can set the T flag in handshakes 
unconditionally, but that would be 3~5 years in the future.


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

As explained above, this only concerns fastd's handshake format. As long as 
no new L2TP version accepts 0 as its "Version" field and such packets 
continue to passed to userspace even without the T flag, fastd would be fine.


> 
> 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 :-)

(e)BPF might actually be an option. I will look into this.


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

Hmm, is it common for protocol implementations in the kernel to warn about 
invalid packets they receive? While L2TP uses connected sockets and thus 
usually no unrelated packets end up in the socket, a simple UDP port scan 
originating from the configured remote address/port will trigger the "short 
packet" warning now (nmap uses a zero-length payload for UDP scans by 
default). Log spam caused by a malicous party might also be a concern.


> 
>>
>> [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@universe-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;
>>>>    	}



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Jakub Kicinski]

On Mon, 22 Feb 2021 17:40:16 +0100 Matthias Schiffer wrote:
> >> 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.  
> 
> Hmm, is it common for protocol implementations in the kernel to warn about 
> invalid packets they receive? While L2TP uses connected sockets and thus 
> usually no unrelated packets end up in the socket, a simple UDP port scan 
> originating from the configured remote address/port will trigger the "short 
> packet" warning now (nmap uses a zero-length payload for UDP scans by 
> default). Log spam caused by a malicous party might also be a concern.

Indeed, seems like appropriate counters would be a good fit here? 
The prints are both potentially problematic for security and lossy.

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 11719 bytes --]

On  Mon, Feb 22, 2021 at 17:40:16 +0100, Matthias Schiffer wrote:
> On 2/22/21 12:49 PM, Tom Parkin wrote:
> > 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!
> 
> That is indeed what I want to achieve.
> 
> > 
> > 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).
> 
> Only unencrypted connections would be offloaded.
> 
> fastd's data protocol will be modified to use L2TP Ethernet pseudowire as
> specified by the RFC (I actually finished the userspace implementation of
> this yesterday, in branch method-l2tp for now). Two peers can negotiate the
> protocol to use (called "method" in fastd terms) in the session handshake. A
> session would only be offloaded to kernel-space L2TP when both sides agree
> that they indeed want to use the L2TP method; otherwise fastd will continue
> to use TUN/TAP.
> 
> The only part of the fastd protocol that I can't modify arbitrarily is the
> first packet of the handshake, as it must be compatible with older versions
> of fastd. There may be a point when I can set the T flag in handshakes
> unconditionally, but that would be 3~5 years in the future.
>

I see.  So the session would end up using L2TP headers, which seems
like it should be fine.

> 
> > 
> > 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.
> 
> As explained above, this only concerns fastd's handshake format. As long as
> no new L2TP version accepts 0 as its "Version" field and such packets
> continue to passed to userspace even without the T flag, fastd would be
> fine.
> 
> 
> > 
> > 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 :-)
> 
> (e)BPF might actually be an option. I will look into this.
> 
> 
> > 
> > > > > 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.
> 
> Hmm, is it common for protocol implementations in the kernel to warn about
> invalid packets they receive? While L2TP uses connected sockets and thus
> usually no unrelated packets end up in the socket, a simple UDP port scan
> originating from the configured remote address/port will trigger the "short
> packet" warning now (nmap uses a zero-length payload for UDP scans by
> default). Log spam caused by a malicous party might also be a concern.

It's not unheard of, but it's not especially common either.  You can
see other pr_warn_ratelimit in the recv path in net/, but not very
many.  I note that pr_debug_ratelimit is much rarer.

I appreciate the argument about log spam -- I'm not really wedded to
these log messages per se, but it seemed worthwhile exploring why you
were tripping over them.

If fastd's data packet headers end up being L2TP headers when using
the L2TP data path, I don't think there's too much of an issue to be
honest.

> > 
> > > 
> > > [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@universe-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;
> > > > >    	}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On  Mon, Feb 22, 2021 at 14:31:38 -0800, Jakub Kicinski wrote:
> On Mon, 22 Feb 2021 17:40:16 +0100 Matthias Schiffer wrote:
> > >> 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.  
> > 
> > Hmm, is it common for protocol implementations in the kernel to warn about 
> > invalid packets they receive? While L2TP uses connected sockets and thus 
> > usually no unrelated packets end up in the socket, a simple UDP port scan 
> > originating from the configured remote address/port will trigger the "short 
> > packet" warning now (nmap uses a zero-length payload for UDP scans by 
> > default). Log spam caused by a malicous party might also be a concern.
> 
> Indeed, seems like appropriate counters would be a good fit here? 
> The prints are both potentially problematic for security and lossy.

Yes, I agree with this argument.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 1893 bytes --]

On 2/23/21 10:47 AM, Tom Parkin wrote:
> On  Mon, Feb 22, 2021 at 14:31:38 -0800, Jakub Kicinski wrote:
>> On Mon, 22 Feb 2021 17:40:16 +0100 Matthias Schiffer wrote:
>>>>> 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.
>>>
>>> Hmm, is it common for protocol implementations in the kernel to warn about
>>> invalid packets they receive? While L2TP uses connected sockets and thus
>>> usually no unrelated packets end up in the socket, a simple UDP port scan
>>> originating from the configured remote address/port will trigger the "short
>>> packet" warning now (nmap uses a zero-length payload for UDP scans by
>>> default). Log spam caused by a malicous party might also be a concern.
>>
>> Indeed, seems like appropriate counters would be a good fit here?
>> The prints are both potentially problematic for security and lossy.
> 
> Yes, I agree with this argument.
> 

Sounds good, I'll send an updated patch adding a counter for invalid packets.

By now I've found another project affected by the kernel warnings:
https://github.com/wlanslovenija/tunneldigger/issues/160


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]