Re: [PATCH net v3 2/2] veth: fix skb length accounting after XDP frag adjustment

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling __skb_pull() before updating skb->len and skb->data_len triggers a BUG_ON() panic if the XDP program expands the linear area and shrinks the head by a large amount.
--

--- Patch [2]: [PATCH net v3 2/2] veth: fix skb length accounting after XDP frag adjustment ---
commit 0dd43470e1427eba807058df83fcecbe72fdc551
Author: Sun Jian <[email protected]>

veth: fix skb length accounting after XDP frag adjustment

This commit synchronizes skb->len and skb->data_len after an XDP program
adjusts the fragment area in veth_xdp_rcv_skb(). It also uses
skb_set_tail_pointer() instead of __skb_put() to handle non-linear skbs
safely.

Link: https://lore.kernel.org/bpf/[email protected]/

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 00e34afd858e7..f80f6584a6ff9 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -865,18 +865,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>  
>  	skb_reset_mac_header(skb);

[Severity: High]
This is a pre-existing issue, but does calling __skb_pull() just above this
line risk triggering a BUG() panic before the skb length synchronization logic
executes?

If an XDP program calls bpf_xdp_pull_data() to pull bytes into the linear
area (increasing xdp->data_end), and then calls bpf_xdp_adjust_head() to
advance the head by a large amount within the newly expanded linear area,
veth_xdp_rcv_skb() will call __skb_pull(skb, -off).

Because this executes before the new skb->len and skb->data_len
synchronization logic below, __skb_pull() subtracts the large adjustment
from the original skb->len. 

If this causes skb->len to drop below the original skb->data_len, wouldn't
this trigger the BUG() assertion in __skb_pull()?

> -	/* check if bpf_xdp_adjust_tail was used */
> -	off = xdp->data_end - orig_data_end;
> -	if (off != 0)
> -		__skb_put(skb, off); /* positive on grow, negative on shrink */
> -
>  	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
> -	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
> +	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
> +	 * from skb->len before updating data_len, then add the new one back.
>  	 */
> -	if (xdp_buff_has_frags(xdp))
> +	skb->len -= skb->data_len;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2