On 05/02/2026 11:42, Sabrina Dubroca wrote:
2026-02-04, 23:42:31 +0100, Antonio Quartulli wrote:
@@ -113,7 +132,7 @@ static void ovpn_tcp_rcv(struct strparser *strp, struct
sk_buff *skb)
/* The packet size header must be there when sending the packet
* to userspace, therefore we put it back
*/
- skb_push(skb, 2);
+ *((__force __be16 *)__skb_push(skb, 2)) = cpu_to_be16(pkt_len);
Why switch to __skb_push here? The skb_under_panic() would be nice in
the (unexpected) case we have an invalid skb.
We just created the skb, so it should be safe to assume that the skb is
valid, no?
I meant invalid in the sense of "not having the expected room to push
the header".
Here we rely on the extra room that netdev_alloc_skb reserves
(NET_SKB_PAD) in case we have to re-push the packet length, right?
The comment at
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/skbuff.h#L3282
about NET_SKB_PAD seems to suggest that this is actual headroom that
networking drivers can assume to have, no?
I guess so. But I'd feel a bit safer if we had something enforcing
that at least this push can't fail, more as a guarantee against an
accidental code change in ovpn. But maybe we'd have bigger problems
through the rest of the stack if ovpn skbs get bridged/forwarded and
we don't have all that headroom.
Exactly my thought. If this assumption doesn't hold anymore, more things
are going to break.
(I expect also other mechanisms outside ovpn to be affected)
On top of that, we ovpn_tcp_send_skb() basically relying on the same
assumption already.
Therefore, I'd just stick with the current version.
However, I have a little nitpick: Ralf, please use the same style as the
assignment in ovpn_tcp_send_skb(), for consistency:
584 *(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
Regards,
And it should never be less than 32 bytes.
Regards,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
