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

[Paolo Abeni <pabeni@redhat.com>]

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.