[PATCH net-next v4 1/2] l2tp: account for IP version in SKB headroom

[PATCH net-next v4 1/2] l2tp: account for IP version in SKB headroom

[David Bauer]

Account for the IP version of the tunnel when accounting skb headroom on
xmit. This avoids having to potentially copy the skb a second time down
the stack due to allocating not enough space for IPv6 headers in case
the tunnel uses IPv6.

Signed-off-by: David Bauer <mail@david-bauer.net>
---
 v4:
  - Update comment to reflect new code behavior

 net/l2tp/l2tp_core.c |  3 ++-
 net/l2tp/l2tp_core.h |  1 +
 net/l2tp/l2tp_eth.c  | 14 ++++----------
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 4712cc41881a3..d6fa0f7436629 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1237,7 +1237,7 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
 	 * make room. Adjust truesize.
 	 */
 	uhlen = (tunnel->encap == L2TP_ENCAPTYPE_UDP) ? sizeof(*uh) : 0;
-	headroom = NET_SKB_PAD + sizeof(struct iphdr) + uhlen + session->hdr_len;
+	headroom = NET_SKB_PAD + tunnel->l3_overhead + uhlen + session->hdr_len;
 	if (skb_cow_head(skb, headroom)) {
 		kfree_skb(skb);
 		return NET_XMIT_DROP;
@@ -1688,6 +1688,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	}
 
 	sk->sk_allocation = GFP_ATOMIC;
+	tunnel->l3_overhead = kernel_sock_ip_overhead(sk);
 	release_sock(sk);
 
 	sock_hold(sk);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index ffd8ced3a51ff..aab574376d95f 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -167,6 +167,7 @@ struct l2tp_tunnel {
 	u32			tunnel_id;
 	u32			peer_tunnel_id;
 	int			version;	/* 2=>L2TPv2, 3=>L2TPv3 */
+	int			l3_overhead;	/* IP header overhead */
 
 	char			name[L2TP_TUNNEL_NAME_MAX]; /* for logging */
 	enum l2tp_encap_type	encap;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index a4956ef9574cc..66d1fc3ad409a 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -188,7 +188,6 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 				struct net_device *dev)
 {
 	unsigned int overhead = 0;
-	u32 l3_overhead = 0;
 	u32 mtu;
 
 	/* if the encap is UDP, account for UDP header size */
@@ -197,22 +196,17 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 		dev->needed_headroom += sizeof(struct udphdr);
 	}
 
-	lock_sock(tunnel->sock);
-	l3_overhead = kernel_sock_ip_overhead(tunnel->sock);
-	release_sock(tunnel->sock);
-
-	if (l3_overhead == 0) {
+	if (tunnel->l3_overhead == 0) {
 		/* L3 Overhead couldn't be identified, this could be
-		 * because tunnel->sock was NULL or the socket's
-		 * address family was not IPv4 or IPv6,
-		 * dev mtu stays at 1500.
+		 * because the socket's address family was not IPv4
+		 * or IPv6 - dev mtu stays at 1500.
 		 */
 		return;
 	}
 	/* Adjust MTU, factor overhead - underlay L3, overlay L2 hdr
 	 * UDP overhead, if any, was already factored in above.
 	 */
-	overhead += session->hdr_len + ETH_HLEN + l3_overhead;
+	overhead += session->hdr_len + ETH_HLEN + tunnel->l3_overhead;
 
 	mtu = l2tp_tunnel_dst_mtu(tunnel) - overhead;
 	if (mtu < dev->min_mtu || mtu > dev->max_mtu)
-- 
2.53.0

[PATCH net-next v4 2/2] l2tp: unify headroom calculation

[David Bauer]

Unify the calculation to determine the required headroom for each skb.
This was previously done inconsistently, resulting requesting more space
in the skb headroom when crafting the L2TP header than indicated with
needed_headroom.

Signed-off-by: David Bauer <mail@david-bauer.net>
---
 net/l2tp/l2tp_core.c | 10 +++++-----
 net/l2tp/l2tp_core.h | 19 +++++++++++++++++++
 net/l2tp/l2tp_eth.c  | 14 +++-----------
 3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index d6fa0f7436629..f52bf3b532907 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1227,18 +1227,16 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
 	struct l2tp_tunnel *tunnel = session->tunnel;
 	unsigned int data_len = skb->len;
 	struct sock *sk = tunnel->sock;
-	int headroom, uhlen, udp_len;
 	int ret = NET_XMIT_SUCCESS;
 	struct inet_sock *inet;
 	struct udphdr *uh;
+	int udp_len;
 
 	/* Check that there's enough headroom in the skb to insert IP,
 	 * UDP and L2TP headers. If not enough, expand it to
 	 * make room. Adjust truesize.
 	 */
-	uhlen = (tunnel->encap == L2TP_ENCAPTYPE_UDP) ? sizeof(*uh) : 0;
-	headroom = NET_SKB_PAD + tunnel->l3_overhead + uhlen + session->hdr_len;
-	if (skb_cow_head(skb, headroom)) {
+	if (skb_cow_head(skb, l2tp_session_skb_headroom(session, tunnel))) {
 		kfree_skb(skb);
 		return NET_XMIT_DROP;
 	}
@@ -1290,7 +1288,9 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
 		uh = udp_hdr(skb);
 		uh->source = inet->inet_sport;
 		uh->dest = inet->inet_dport;
-		udp_len = uhlen + session->hdr_len + data_len;
+
+		udp_len = l2tp_tunnel_udp_hdrlen(tunnel);
+		udp_len += session->hdr_len + data_len;
 		if (udp_len > U16_MAX) {
 			kfree_skb(skb);
 			ret = NET_XMIT_DROP;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index aab574376d95f..2c41877b6ce57 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -335,6 +335,25 @@ static inline int l2tp_v3_ensure_opt_in_linear(struct l2tp_session *session, str
 	return 0;
 }
 
+static inline int l2tp_tunnel_udp_hdrlen(struct l2tp_tunnel *tunnel)
+{
+	return tunnel->encap == L2TP_ENCAPTYPE_UDP ?
+		sizeof(struct udphdr) : 0;
+}
+
+static inline int l2tp_session_overhead(struct l2tp_session *session,
+					struct l2tp_tunnel *tunnel)
+{
+	return l2tp_tunnel_udp_hdrlen(tunnel) + session->hdr_len +
+		tunnel->l3_overhead;
+}
+
+static inline int l2tp_session_skb_headroom(struct l2tp_session *session,
+					    struct l2tp_tunnel *tunnel)
+{
+	return NET_SKB_PAD + l2tp_session_overhead(session, tunnel);
+}
+
 #define MODULE_ALIAS_L2TP_PWTYPE(type) \
 	MODULE_ALIAS("net-l2tp-type-" __stringify(type))
 
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 66d1fc3ad409a..687ce1c6db946 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -190,12 +190,6 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 	unsigned int overhead = 0;
 	u32 mtu;
 
-	/* if the encap is UDP, account for UDP header size */
-	if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
-		overhead += sizeof(struct udphdr);
-		dev->needed_headroom += sizeof(struct udphdr);
-	}
-
 	if (tunnel->l3_overhead == 0) {
 		/* L3 Overhead couldn't be identified, this could be
 		 * because the socket's address family was not IPv4
@@ -203,10 +197,8 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 		 */
 		return;
 	}
-	/* Adjust MTU, factor overhead - underlay L3, overlay L2 hdr
-	 * UDP overhead, if any, was already factored in above.
-	 */
-	overhead += session->hdr_len + ETH_HLEN + tunnel->l3_overhead;
+	/* Calculate required overhead */
+	overhead = ETH_HLEN + l2tp_session_overhead(session, tunnel);
 
 	mtu = l2tp_tunnel_dst_mtu(tunnel) - overhead;
 	if (mtu < dev->min_mtu || mtu > dev->max_mtu)
@@ -214,7 +206,7 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 	else
 		dev->mtu = mtu;
 
-	dev->needed_headroom += session->hdr_len;
+	dev->needed_headroom = l2tp_session_skb_headroom(session, tunnel);
 }
 
 static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
-- 
2.53.0

Re: [PATCH net-next v4 1/2] l2tp: account for IP version in SKB headroom

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
l2tp: account for IP version in SKB headroom

The patch caches kernel_sock_ip_overhead() in the new
tunnel->l3_overhead field at tunnel registration time, and uses that
value instead of sizeof(struct iphdr) when computing the transmit
headroom in l2tp_xmit_core() and the device MTU in
l2tp_eth_adjust_mtu().

A few questions on the new field and its users below.

> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 4712cc41881a..d6fa0f743662 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1237,7 +1237,7 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
>  	 * make room. Adjust truesize.
>  	 */
>  	uhlen = (tunnel->encap == L2TP_ENCAPTYPE_UDP) ? sizeof(*uh) : 0;
> -	headroom = NET_SKB_PAD + sizeof(struct iphdr) + uhlen + session->hdr_len;
> +	headroom = NET_SKB_PAD + tunnel->l3_overhead + uhlen + session->hdr_len;
>  	if (skb_cow_head(skb, headroom)) {
>  		kfree_skb(skb);
>  		return NET_XMIT_DROP;

[Severity: Medium]
Does the PPP pseudowire still hit the extra copy this patch aims to
remove?  The producers in net/l2tp/l2tp_ppp.c were not converted and
still reserve an IPv4-sized L3 header.

pppol2tp_xmit():

	headroom = NET_SKB_PAD +
		   sizeof(struct iphdr) + /* IP header */
		   uhlen +		/* UDP header (if L2TP_ENCAPTYPE_UDP) */
		   session->hdr_len +	/* L2TP header */
		   2;			/* 2 bytes for PPP_ALLSTATIONS & PPP_UI */
	if (skb_cow_head(skb, headroom))
		goto abort_put_sess;

pppol2tp_sendmsg():

	skb = sock_wmalloc(sk, NET_SKB_PAD + sizeof(struct iphdr) +
			   uhlen + session->hdr_len +
			   2 + total_len,
			   0, GFP_KERNEL);
	...
	skb_reserve(skb, sizeof(struct iphdr));

For an AF_INET6 tunnel socket tunnel->l3_overhead is at least
sizeof(struct ipv6hdr) (40), so the requirement in l2tp_xmit_core()
exceeds what pppol2tp_xmit()/pppol2tp_sendmsg() just reserved by around
20 bytes and __skb_cow() calls pskb_expand_head() again, this time for
every transmitted PPP-over-L2TP packet.

PPPOL2TP_HEADER_OVERHEAD, which drives po->chan.mtu, is likewise still
a fixed 40:

	/* Space for UDP, L2TP and PPP headers */
	#define PPPOL2TP_HEADER_OVERHEAD	40

Would it make sense to convert these sites to tunnel->l3_overhead as
well, so the benefit is not limited to the l2tp_eth pseudowire?
(l2tp_ip_sendmsg() in net/l2tp/l2tp_ip.c also uses sizeof(struct iphdr),
but it is AF_INET-only, so it is only stylistically inconsistent.)

> @@ -1688,6 +1688,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  	}
>  
>  	sk->sk_allocation = GFP_ATOMIC;
> +	tunnel->l3_overhead = kernel_sock_ip_overhead(sk);
>  	release_sock(sk);

[Severity: Medium]
Can this snapshot go stale?  kernel_sock_ip_overhead() intentionally
includes mutable socket state:

net/socket.c:kernel_sock_ip_overhead() {
	case AF_INET:
		overhead += sizeof(struct iphdr);
		opt = rcu_dereference_protected(inet->inet_opt, ...);
		if (opt)
			overhead += opt->opt.optlen;
	...
	case AF_INET6:
		overhead += sizeof(struct ipv6hdr);
		...
		if (optv6)
			overhead += (optv6->opt_flen + optv6->opt_nflen);
}

This is the only assignment of tunnel->l3_overhead in net/l2tp/, and
after release_sock(sk) the tunnel fd stays open for user-provided
sockets (tunnel->fd >= 0), so setsockopt(SOL_IP, IP_OPTIONS) or the
IPv6 sticky options (IPV6_HOPOPTS/IPV6_DSTOPTS/IPV6_RTHDR) can change
the real overhead with nothing refreshing the cached value.

Before the patch l2tp_eth_adjust_mtu() re-read the value live under
lock_sock() each time a session/netdev was created, so the sequence

    create tunnel -> setsockopt(fd, IPPROTO_IP, IP_OPTIONS, ...)
                  -> L2TP_CMD_SESSION_CREATE (pwtype ETH)

now computes dev->mtu and dev->needed_headroom from a too-small
overhead, and l2tp_xmit_core() under-reserves headroom for every packet.
Since l2tp_xmit_queue() sets skb->ignore_df = 1, does this end up
fragmenting every full-size frame?

Would reading the overhead where it is consumed (under lock_sock), or
refreshing it when the socket options change, be preferable?  If the
snapshot semantics are intentional, could the field comment and the
changelog say so?

> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> index ffd8ced3a51f..aab574376d95 100644
> --- a/net/l2tp/l2tp_core.h
> +++ b/net/l2tp/l2tp_core.h
> @@ -167,6 +167,7 @@ struct l2tp_tunnel {
>  	u32			tunnel_id;
>  	u32			peer_tunnel_id;
>  	int			version;	/* 2=>L2TPv2, 3=>L2TPv3 */
> +	int			l3_overhead;	/* IP header overhead */
>  
>  	char			name[L2TP_TUNNEL_NAME_MAX]; /* for logging */
>  	enum l2tp_encap_type	encap;

> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index a4956ef9574c..66d1fc3ad409 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c

[ ... ]

> @@ -197,22 +196,17 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
>  		dev->needed_headroom += sizeof(struct udphdr);
>  	}
>  
> -	lock_sock(tunnel->sock);
> -	l3_overhead = kernel_sock_ip_overhead(tunnel->sock);
> -	release_sock(tunnel->sock);
> -
> -	if (l3_overhead == 0) {
> +	if (tunnel->l3_overhead == 0) {
>  		/* L3 Overhead couldn't be identified, this could be
> -		 * because tunnel->sock was NULL or the socket's
> -		 * address family was not IPv4 or IPv6,
> -		 * dev mtu stays at 1500.
> +		 * because the socket's address family was not IPv4
> +		 * or IPv6 - dev mtu stays at 1500.
>  		 */
>  		return;
>  	}

[Severity: Low]
Can tunnel->l3_overhead still be 0 here for the reason the reworded
comment gives?  l2tp_validate_socket() rejects everything but
PF_INET/PF_INET6:

net/l2tp/l2tp_core.c:l2tp_validate_socket() {
	if (sk->sk_family != PF_INET && sk->sk_family != PF_INET6)
		return -EPROTONOSUPPORT;
}

and l2tp_tunnel_register() only assigns tunnel->l3_overhead on the
success path after that check with a non-NULL sk, so
kernel_sock_ip_overhead() returns at least sizeof(struct iphdr) or
sizeof(struct ipv6hdr).  The one previously reachable reason
(tunnel->sock == NULL) is the one that was dropped from the comment.

Should the guard just be removed, or the comment changed to describe the
actual invariant?

>  	/* Adjust MTU, factor overhead - underlay L3, overlay L2 hdr
>  	 * UDP overhead, if any, was already factored in above.
>  	 */
> -	overhead += session->hdr_len + ETH_HLEN + l3_overhead;
> +	overhead += session->hdr_len + ETH_HLEN + tunnel->l3_overhead;
>  
>  	mtu = l2tp_tunnel_dst_mtu(tunnel) - overhead;
>  	if (mtu < dev->min_mtu || mtu > dev->max_mtu)

[Severity: Medium]
This isn't a bug introduced by this patch, but the same expression is
worth a look while it is being touched.  Both subtractions here are
unsigned, and the fallback assignment escapes the min_mtu/max_mtu
check:

	mtu = l2tp_tunnel_dst_mtu(tunnel) - overhead;
	if (mtu < dev->min_mtu || mtu > dev->max_mtu)
		dev->mtu = ETH_DATA_LEN - overhead;
	else
		dev->mtu = mtu;

For an AF_INET6 tunnel socket the L3 overhead includes
opt_flen + opt_nflen from sticky extension-header options, which can be
a couple of kilobytes each, so overhead can exceed ETH_DATA_LEN and
ETH_DATA_LEN - overhead wraps into a near-2^32 value written straight
into dev->mtu.

The fallback branch is reachable because l2tp_eth_create() sets
dev->min_mtu = 0 and l2tp_tunnel_dst_mtu() returns 0 when no dst is
cached, so mtu = 0 - overhead exceeds dev->max_mtu.

Would an early bound such as "if (overhead >= ETH_DATA_LEN) return;"
be worth adding here?
-- 
This is an AI-generated review.