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

[Lorenzo Bianconi]

> veth exposes non-linear skb fragments through an xdp_buff. If an XDP
> program adjusts the fragment area, veth_xdp_rcv_skb() copies
> xdp_frags_size back to skb->data_len but leaves skb->len containing the
> old fragment contribution.
> 
> After a fragment shrink, this makes skb_headlen() larger than the actual
> linear area. In the reproduced UDP receive path, __skb_datagram_iter()
> copied 1024 bytes past the actual linear tail to userspace, starting at
> struct skb_shared_info. The copied bytes included the affected skb's
> nr_frags, xdp_frags_size and a kernel pointer from
> skb_shinfo(skb)->frags[0]. Real packet data was displaced by the same
> amount and truncated at the end.
> 
> Subtract the old data_len before replacing it and add the new data_len
> afterwards, keeping skb->len and skb->data_len synchronized.
> 
> The fragment accounting must run before the linear tail adjustment:
> when bpf_xdp_adjust_tail() shrinks the packet into the linear area it
> releases all fragments, and __skb_put() requires skb->data_len == 0
> by that point.
> 
> A 60000-byte UDP datagram on a veth pair with MTU 64000 was shortened by
> 1024 bytes from its fragment area. Before the fix, all 10 runs produced
> corrupted payloads. After the fix, all 10 runs matched the expected
> payload exactly.
> 
> Fixes: 718a18a0c8a6 ("veth: Rework veth_xdp_rcv_skb in order to accept non-linear skb")
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/
> Signed-off-by: Sun Jian <sun.jian.kdev@xxxxxxxxx>

Acked-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>

> ---
>  drivers/net/veth.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 00e34afd858e..348391e87e14 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -865,18 +865,25 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>  
>  	skb_reset_mac_header(skb);
>  
> -	/* 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.
> +	 * This must precede the linear tail adjustment below: a changed
> +	 * data_end implies that no fragments remain, and __skb_put() requires
> +	 * a linear skb.
>  	 */
> -	if (xdp_buff_has_frags(xdp))
> +	skb->len -= skb->data_len;
> +	if (xdp_buff_has_frags(xdp)) {
>  		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> -	else
> +		skb->len += skb->data_len;
> +	} else {
>  		skb->data_len = 0;
> +	}
> +
> +	/* 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 */
>  
>  	skb->protocol = eth_type_trans(skb, rq->dev);
>  
> -- 
> 2.43.0
>

Attachment: signature.asc
Description: PGP signature