[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 2/2] l2tp: unify headroom calculation

[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: unify headroom calculation

This introduces three inline helpers in net/l2tp/l2tp_core.h that compute
the UDP header length, the per-session encapsulation overhead and the
required skb headroom, and switches l2tp_xmit_core() and
l2tp_eth_adjust_mtu() over to them.  A few questions below on the new
helpers and their users.

> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index d6fa0f743662..f52bf3b53290 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;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but since the patch re-touches
exactly these reads, would it be worth snapshotting session->hdr_len once
into a local here?

l2tp_xmit_core() reads session->hdr_len three times for the same packet
with no lock held: once inside l2tp_session_skb_headroom() for the
skb_cow_head() reservation, once for the header push:

	if (tunnel->version == L2TP_HDR_VER_2)
		l2tp_build_l2tpv2_header(session, __skb_push(skb, session->hdr_len));
	else
		l2tp_build_l2tpv3_header(session, __skb_push(skb, session->hdr_len));

and once more for udp_len further down.

session->hdr_len is written at run time by l2tp_session_set_header_len()
from contexts that share no lock with the transmit path: netlink session
modify on L2TP_ATTR_SEND_SEQ, pppol2tp setsockopt PPPOL2TP_SO_SENDSEQ, and
the receive softirq LNS-mode auto-enable in l2tp_recv_common():

	if (!session->lns_mode && !session->send_seq) {
		trace_session_seqnum_lns_enable(session);
		session->send_seq = 1;
		l2tp_session_set_header_len(session, tunnel->version,
					    tunnel->encap);

The last one is driven by whether the remote peer sets the S bit in
received data packets, so the window can be opened by remote input rather
than only by privileged configuration.

If hdr_len goes from 6 to 10 (L2TPv2, send_seq turned on) between the
__skb_push() and the udp_len computation, does the UDP length written by
udp_set_len_short() and the pseudo-header used for the checksum end up
describing four more bytes than were actually pushed?  sk->sk_lock.slock is
taken only after the push, and none of the writers hold it.

> @@ -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 aab574376d95..2c41877b6ce5 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);
> +}

[Severity: Low]
The hardcoded formulas in the PPP pseudowire are pre-existing and not
introduced by this patch, but the helpers land in the shared header while
the equivalent open-coded computations in net/l2tp/l2tp_ppp.c stay as they
are.  Was the PPP pseudowire meant to be part of this?

pppol2tp_xmit() still computes its own value:

	uhlen = (tunnel->encap == L2TP_ENCAPTYPE_UDP) ? sizeof(struct udphdr) : 0;
	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))

pppol2tp_sendmsg() allocates with the same hardcoded IPv4 header size:

	skb = sock_wmalloc(sk, NET_SKB_PAD + sizeof(struct iphdr) +
			   uhlen + session->hdr_len +
			   2 + total_len, /* 2 bytes for PPP_ALLSTATIONS & PPP_UI */
			   0, GFP_KERNEL);

pppol2tp_tunnel_mtu() still uses the fixed PPPOL2TP_HEADER_OVERHEAD instead
of l2tp_session_overhead(), and pppol2tp_connect() still sets
po->chan.hdrlen = PPPOL2TP_L2TP_HDR_SIZE_SEQ, which is the PPP-layer
analogue of the dev->needed_headroom this patch fixes for l2tpeth.

Since sizeof(struct iphdr) substitutes for tunnel->l3_overhead, for an IPv6
tunnel socket the PPP paths ask for at least 20 bytes less than the
subsequent skb_cow_head(l2tp_session_skb_headroom()) in l2tp_xmit_core()
requests.  In practice __skb_cow() rounds the expansion up with
ALIGN(delta, NET_SKB_PAD), so this normally only costs a redundant
pskb_expand_head(), but the tree now carries two disagreeing definitions of
the same quantity.

Also, the changelog says "Unify the calculation to determine the required
headroom for each skb" - could it be narrowed to say which pseudowires were
converted, since the PPP paths keep their own calculation?

> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 66d1fc3ad409..687ce1c6db94 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);
> -	}
> -

[ ... ]

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

[Severity: Low]
Should NET_SKB_PAD be part of dev->needed_headroom?

Before the patch the field accumulated only bytes this device actually
prepends (sizeof(struct udphdr) for UDP encap plus session->hdr_len).  With
l2tp_session_skb_headroom() the value also contains NET_SKB_PAD, which is
the generic skb allocation pad rather than a header the device prepends.

include/linux/netdevice.h describes needed_headroom as "Extra headroom the
hardware may need", and it is added to hard_header_len for every skb the
stack allocates for transmission on the device:

	#define LL_RESERVED_SPACE_EX(dev, hlen) \
		((((hlen) + READ_ONCE((dev)->needed_headroom)) \
		  & ~(HH_DATA_MOD - 1)) + HH_DATA_MOD)

It is also propagated to stacked devices via netdev_get_fwd_headroom():

	return dev->priv_flags & IFF_PHONY_HEADROOM ? 0 : dev->needed_headroom;

so when the l2tpeth device is enslaved to a bridge, br_if.c's
update_headroom() pushes the maximum port value into every other port with
netdev_set_rx_headroom(), and the extra ~64 bytes get charged to unrelated
devices.  A grep over net/ and drivers/net/ did not turn up another device
that folds NET_SKB_PAD into needed_headroom.

Would dropping NET_SKB_PAD from the value stored in needed_headroom (or
keeping it only in the skb_cow_head() request in l2tp_xmit_core()) be the
intended reconciliation?  Either way, the changelog does not mention this
change of meaning for needed_headroom.
-- 
This is an AI-generated review.

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.