Re: [PATCH net-next 2/3] tun: don't disable BHs for the XDP program check of every packet

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] tun: ubuf_info reference leak in tun_get_user() [1]
--

commit 1a1e58a937e3b55d0632ff554e804970bf806fab
Author: Antonio Quartulli <antonio@mandelbit.com>

tun: don't disable BHs for the XDP program check of every packet

This commit checks tun->xdp_prog with rcu_access_pointer() in
tun_build_skb() and tun_get_user() so packets on devices without an XDP
program avoid entering an RCU read-side critical section or disabling BHs.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9fc60a73fb963..395923651f312 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1702,13 +1702,12 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	int pad = TUN_RX_PAD;
>  	int metasize = 0;
>  	int err = 0;
> +	bool xdp;
>  
> -	rcu_read_lock();
> -	xdp_prog = rcu_dereference(tun->xdp_prog);
> -	if (xdp_prog)
> +	xdp = rcu_access_pointer(tun->xdp_prog);
> +	if (xdp)
>  		pad += XDP_PACKET_HEADROOM;
>  	buflen += SKB_DATA_ALIGN(len + pad);
> -	rcu_read_unlock();
>  
>  	alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
>  	if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
> @@ -1725,7 +1724,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	 * of xdp_prog above, this should be rare and for simplicity
>  	 * we do XDP on skb in case the headroom is not enough.
>  	 */
> -	if (hdr->gso_type || !xdp_prog) {
> +	if (hdr->gso_type || !xdp) {
>  		*skb_xdp = 1;
>  		return __tun_build_skb(tfile, alloc_frag, buf, buflen, len,
>  				       pad, metasize);
> @@ -1962,7 +1961,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	skb_reset_network_header(skb);
>  	skb_record_rx_queue(skb, tfile->queue_index);
>  
> -	if (skb_xdp) {
> +	/* Only TAP devices can have an XDP program: don't disable BHs and
> +	 * enter an RCU section for every packet just to find out it's unset.
> +	 */
> +	if (skb_xdp && rcu_access_pointer(tun->xdp_prog)) {
>  		struct bpf_prog *xdp_prog;
>  		int ret;
>  

[Severity: High]
This problem wasn't introduced by this patch, but does tun_get_user() leak
the msg_control ubuf_info reference when tun_build_skb() consumes the
packet via XDP [1]?

When msg_control is non-NULL but iov_iter_npages() exceeds MAX_SKB_FRAGS,
zerocopy stays false and tun_can_build_skb() can succeed:

drivers/net/tun.c:tun_get_user() {
    ...
	if (msg_control) {
		...
		if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
			zerocopy = true;
	}

	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
		skb = tun_build_skb(tun, tfile, from, gso, len, &skb_xdp);
		err = PTR_ERR_OR_ZERO(skb);
		if (err)
			goto drop;
		if (!skb)
			return total_len;
	}
    ...
	if (zerocopy)
		skb_zcopy_init(skb, msg_control);
	else if (msg_control)
		net_zcopy_complete(NULL, msg_control, false);
}

If the XDP program in tun_build_skb() returns XDP_DROP, XDP_TX, or
XDP_REDIRECT, tun_build_skb() returns NULL and tun_get_user() returns
total_len without calling net_zcopy_complete().  That permanently leaks the
ubuf_info reference, which exhausts vhost_net virtqueue descriptors and
hangs vhost_net_ubuf_put_wait_and_free() on teardown.

[1] https://sashiko.dev/#/bug/linux-f9997fb6-303f-41d6-93e2-60c392b1856f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112150612.3866546-1-antonio@mandelbit.com?part=2