Message-ID: <aRycMp7hlhY3ZC5U@krikkit>
Date: Tue, 18 Nov 2025 17:17:54 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Ralf Lici <ralf@...delbit.com>
Cc: netdev@...r.kernel.org, Antonio Quartulli <antonio@...nvpn.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [RFC net-next] ovpn: allocate smaller skb when TCP headroom
 exceeds u16

2025-11-13, 13:21:43 +0100, Ralf Lici wrote:
> Hi all,
> 
> While testing openvpn over TCP under high traffic conditions,
> specifically on the same machine using net namespaces (with veth pairs
> interconnecting them), we consistently hit a warning in
> skb_reset_network_header. The culprit is an attempt to store an offset
> (skb->data - skb->head) larger than U16_MAX in skb->network_header,
> which is a u16. This leads to packet drops.
> 
> In ovpn_tcp_recv, we're handed an skb from __strp_rcv and need to
> linearize it and pull up to the beginning of the openvpn packet. If it's

We don't currently linearize (= move all the data into ->head), right?

> a data-channel packet, we then pull an additional 24 bytes of openvpn
> encapsulation header so that skb->data points to the inner IP packet.
> This is necessary for authentication, decryption, and reinjection into
> the networking stack of the decapsulated packet, but when the skb is too
> large, the network header offset overflows the field.
> 
> AFAWCT, these oversized skbs can result from:
> - GRO,
> - TCP skb coalescing (tcp_try_coalesce, skb_try_coalesce),
> - streamparser (__strp_rcv appends more skbs when an openvpn packet
>   spans multiple skbs).
>
> Note that this issue is likely affecting espintcp as well, since its
> logic similarly involves extracting discrete packets from a coalesced
> TCP stream handed off by streamparser, and reinjecting them into the
> stack.

Most likely yes. I'll see if I can reproduce the problem on espintcp.

> We've brainstormed a few possible directions, though we haven't yet
> assessed their feasibility:
> - introduce a u32 field in struct tcp_sock to limit skb->len during TCP
>   coalescing (each socket user can set the limit if needed);

I doubt the TCP maintainers would accept a patch to TCP for a problem
that affects only (some of) the users of strp.

> - modify strp to build an skb containing only the relevant frags for the
>   current openvpn packet in frag_list.

This would penalize the other users of strp. It may make sense to
introduce such a mechanism in strp, but only on request (eg via a bool
in strp_init, a flag in the cb struct).

> In this patch, we implement a solution entirely contained within ovpn:
> we allocate a new skb and copy the content of the current openvpn packet
> into it. This avoids the large headroom issue, but it’s not ideal
> because the kernel keeps coalescing skbs while we effectively undo that
> work, which isn’t very efficient.

Well, that coalescing is useful, and the un-coalescing is necessary
(because even without this offset problem, we have to get back the
individual packets from the stream).


Copying the full contents (full_len) of the openvpn packet seems a bit
heavy when what we want is "pull and get rid of that extra space at
the head". It seems pskb_extract would do the job without manual
handling in ovpn and without copying the entire payload? (but it will
clone the skb and realloc the head every time, so we'd only want to
call it in the "offset too big" case)

> We're sending this RFC to gather ideas and suggestions on how best to
> address this issue. Any thoughts or guidance would be appreciated.

One thing I'm a bit concerned about is if those reduced skbs need to
be re-sent somewhere else. Then we don't have any headroom to push a
new header and we'll have to realloc again to create some space. OTOH
it doesn't really make sense to carry 65kB of extra data through the
stack.



A few comments on the implementation:

[...]
> diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> index b7348da9b040..301fcb1c0495 100644
> --- a/drivers/net/ovpn/tcp.c
> +++ b/drivers/net/ovpn/tcp.c
> @@ -70,39 +70,87 @@ static void ovpn_tcp_to_userspace(struct ovpn_peer *peer, struct sock *sk,
>  	peer->tcp.sk_cb.sk_data_ready(sk);
>  }
>  
> -static void ovpn_tcp_rcv(struct strparser *strp, struct sk_buff *skb)
> +/* takes ownership of orig_skb */
> +static struct sk_buff *ovpn_tcp_skb_packet(const struct ovpn_peer *peer,
> +					   struct sk_buff *orig_skb,
> +					   const int full_len, const int offset)
>  {
> -	struct ovpn_peer *peer = container_of(strp, struct ovpn_peer, tcp.strp);
> -	struct strp_msg *msg = strp_msg(skb);
> -	size_t pkt_len = msg->full_len - 2;
> -	size_t off = msg->offset + 2;
> -	u8 opcode;
> +	struct sk_buff *ovpn_skb = orig_skb;
> +	const int pkt_len = full_len - 2;
> +	int pkt_offset = offset + 2;
> +	int err;
> +
> +	/* If the final headroom will overflow a u16 we will not be able to
> +	 * reset the network header to it so we need to create a new smaller
> +	 * skb with the content of this packet.
> +	 */
> +	if (unlikely(skb_headroom(orig_skb) + pkt_offset + OVPN_HEADER_SIZE >
> +		     U16_MAX)) {
> +		ovpn_skb = netdev_alloc_skb(peer->ovpn->dev, full_len);

>From my reading of __strp_recv, strp already gave us a fresh clone, do
we need to reallocate a full skb?

> +		if (!ovpn_skb) {
> +			ovpn_skb = orig_skb;
> +			goto err;
> +		}
> +
> +		skb_copy_header(ovpn_skb, orig_skb);
> +		pkt_offset = 2;
> +
> +		/* copy the entire openvpn packet + 2 bytes length */
> +		err = skb_copy_bits(orig_skb, offset,
> +				    skb_put(ovpn_skb, full_len), full_len);
> +		kfree(orig_skb);
> +		if (err) {
> +			net_warn_ratelimited("%s: skb_copy_bits failed for peer %u\n",
> +					     netdev_name(peer->ovpn->dev),
> +					     peer->id);
> +			goto err;
> +		}
> +	}
>  
>  	/* ensure skb->data points to the beginning of the openvpn packet */
> -	if (!pskb_pull(skb, off)) {
> +	if (!pskb_pull(ovpn_skb, pkt_offset)) {
>  		net_warn_ratelimited("%s: packet too small for peer %u\n",
> -				     netdev_name(peer->ovpn->dev), peer->id);
> +				     netdev_name(peer->ovpn->dev),
> +				     peer->id);
>  		goto err;
>  	}
>  
>  	/* strparser does not trim the skb for us, therefore we do it now */
> -	if (pskb_trim(skb, pkt_len) != 0) {
> +	if (pskb_trim(ovpn_skb, pkt_len) != 0) {
>  		net_warn_ratelimited("%s: trimming skb failed for peer %u\n",
> -				     netdev_name(peer->ovpn->dev), peer->id);
> +				     netdev_name(peer->ovpn->dev),
> +				     peer->id);
>  		goto err;
>  	}
>  
> +	return ovpn_skb;
> +err:
> +	kfree(ovpn_skb);

This needs to be kfree_skb/consume_skb in all cases where you're
freeing an skb.

> +	return NULL;
> +}

-- 
Sabrina

Powered by blists - more mailing lists