Re: [RFC net-next] ovpn: allocate smaller skb when TCP headroom exceeds u16

[Ralf Lici <ralf@mandelbit.com>]

On Tue, 2025-11-18 at 17:17 +0100, Sabrina Dubroca wrote:
> 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?

Actually, my understanding is that we effectively do, though it happens
implicitly in the crypto layer rather than in the RX callback.

While ovpn_tcp_rcv pulls up to the beginning of the openvpn packet in
the skb, the subsequent call to ovpn_aead_decrypt invokes skb_cow_data
to ensure the *whole* skb is writable for decryption. Since strparser
hands us cloned skbs, skb_cow_data calls __pskb_pull_tail(skb,
__skb_pagelen(skb)). As I understand it, this forces a full
linearization for every single packet in the huge skb.

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

Thanks!

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

Perfectly understandable. I will drop this idea.

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

I agree that we should keep strparser generic. Since this issue is
specific to how ovpn/espintcp handle the resulting skbs (decapsulation
and header resetting), it is probably better to handle the output within
the users rather than complicating the strparser API.

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

Thanks, I was not aware of this function and I believe it would indeed
solve the overflow issue. However, I'm not sure it would be more
efficient. Since the problem arises from huge offsets on non-linear
skbs, AFAICT it's almost certain that we'd take the
pskb_carve_inside_nonlinear path in pskb_carve. That function will
produce a fully non-linear cloned skb by removing the pages that are not
needed. But, following the skb_cow_data explanation above, we would end
up linearizing the content of this skb in the end anyway. So I fear
pskb_extract might not be beneficial in this scenario.

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

That's a valid concern and I have the feeling that there must be a
"standard" way of creating an skb with some content and a reasonable
headroom. I thought netdev_alloc_skb could be a good choice because it
builds an skb with NET_SKB_PAD headeroom, but I'm not sure if that's the
proper size. 

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

As explained above, the reason is the call to skb_cow_data in
ovpn_aead_decrypt, which, in the end, would linearize the whole cloned
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.

Yep, sorry. Something got lost during the refactoring.

> 
> > +	return NULL;
> > +}

Thanks a lot for your input.

-- 
Ralf Lici
Mandelbit Srl