[PATCH net] tun: complete the zerocopy buffer of packets consumed by XDP

[PATCH net] tun: complete the zerocopy buffer of packets consumed by XDP

[Antonio Quartulli]

When vhost-net passes a zerocopy buffer that tun_get_user() does not use,
because the packet spans too many pages, the packet is copied and the
buffer completed. If the copy goes through build_skb() and native XDP
consumes the packet (DROP, TX or REDIRECT), tun_get_user() returns
without completing it: vhost-net never gets the descriptor back, and
closing the vhost-net device then blocks forever.

Complete it in that case too, as copied.

Reproducer: vhost-net with experimental_zcopytx=1 on a tap device with a
native XDP_DROP program; send one 806-byte packet as 40 descriptors, each
in its own page. The descriptor is never returned and close() of the
vhost-net fd does not return. With the patch it is returned.

Tested in an x86-64 VM with the reproducer and the tun kselftest.

Found by the Sashiko AI review bot, as a pre-existing issue while
reviewing an unrelated tun patch, and confirmed with the reproducer.

Link: https://sashiko.dev/#/bug/linux-f9997fb6-303f-41d6-93e2-60c392b1856f
Fixes: 761876c857cb ("tap: XDP support")
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
Notes:
- Completed as copied (success false), as the copy path does: the packet
  was copied into a page fragment.
- net-next replaced ubuf_info->ops with net_zcopy_complete() in
  ecca9bb2c855; there the call becomes
  net_zcopy_complete(NULL, msg_control, false).

 drivers/net/tun.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 86fd44e10755..927196840725 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1871,8 +1871,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		err = PTR_ERR_OR_ZERO(skb);
 		if (err)
 			goto drop;
-		if (!skb)
+		if (!skb) {
+			/* consumed by XDP, after copying */
+			if (msg_control) {
+				struct ubuf_info *uarg = msg_control;
+
+				uarg->ops->complete(NULL, uarg, false);
+			}
 			return total_len;
+		}
 	} else {
 		if (!zerocopy) {
 			copylen = len;

base-commit: ead6fa9a0ab194f02a2f36d634b193e9c5f9a1c6
-- 
2.56.0

Re: [PATCH net] tun: complete the zerocopy buffer of packets consumed by XDP

[sashiko-bot]

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:
- [Critical] tun: type confusion in tun_napi_alloc_frags() [1]
--

commit 432dd92fb61360561c1a6f28a7f9ecf8e823ad4c
Author: Antonio Quartulli <antonio@mandelbit.com>

tun: complete the zerocopy buffer of packets consumed by XDP

This commit completes the caller's zerocopy ubuf_info as copied when
tun_build_skb() copies the packet and native XDP consumes it.

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

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 86fd44e107550..9271968407253 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1871,8 +1871,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		err = PTR_ERR_OR_ZERO(skb);
>  		if (err)
>  			goto drop;
> -		if (!skb)
> +		if (!skb) {
> +			/* consumed by XDP, after copying */
> +			if (msg_control) {
> +				struct ubuf_info *uarg = msg_control;
> +
> +				uarg->ops->complete(NULL, uarg, false);
> +			}
>  			return total_len;
> +		}
>  	} else {
>  		if (!zerocopy) {
>  			copylen = len;

[Severity: Critical]
This problem wasn't introduced by this patch, but can
tun_napi_alloc_frags() in the adjacent else branch corrupt memory when
tun_get_user() is called with a non-iovec iterator [1]?

drivers/net/tun.c:tun_get_user() {
    ...
		if (frags) {
			mutex_lock(&tfile->napi_mutex);
			skb = tun_napi_alloc_frags(tfile, copylen, from);
    ...
}

drivers/net/tun.c:tun_napi_alloc_frags() {
    ...
	for (i = 1; i < it->nr_segs; i++) {
		const struct iovec *iov = iter_iov(it) + i;
		size_t fragsz = iov->iov_len;
    ...
		frag = netdev_alloc_frag(fragsz);
    ...
}

When IFF_NAPI_FRAGS is enabled and a caller writes to the tun device using
an ITER_BVEC iterator (for example via splice() or io_uring),
tun_napi_alloc_frags() treats the bio_vec array as an iovec array via
iter_iov(it). Reading iov->iov_len from a struct bio_vec produces a bogus
fragment size from bv_len and bv_offset, causing skb_copy_datagram_from_iter()
to overflow the allocated skb fragments.

[1] https://sashiko.dev/#/bug/linux-ddceb3c7-6f22-48cf-b390-d7b534490c8f

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