Re: [PATCH v2 1/4] tun: fix inverted error check when re-attaching the filter

on 2026/9/29 18:34, bot+bpf-ci@kernel.org wrote:

quoted

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68a..a2fffef3735f 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c

@@ -728,6 +728,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 {
 	struct tun_file *tfile = file->private_data;
 	struct net_device *dev = tun->dev;
+	bool rollback_filter = false;
 	int err;

 	err = security_tun_dev_attach(tfile->socket.sk, tun->security);

@@ -754,8 +755,9 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 		lock_sock(tfile->socket.sk);
 		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (!err)
+		if (err)
 			goto out;
+		rollback_filter = true;
 	}

Does this change the behavior for cases where tun->fprog.filter holds a
__user pointer from a different process?

When TUNATTACHFILTER runs, it copies only the sock_fprog header:

    case TUNATTACHFILTER:
        ...
        if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))

This means tun->fprog.filter still points into the address space of the
process that issued that ioctl. When sk_attach_filter() runs here, it
calls __get_filter() which does:

    copy_from_user(prog->insns, fprog->filter, fsize)

in the context of the current process.

For a persistent TAP, if process A creates it and runs TUNATTACHFILTER
with a filter array on its stack or heap, then closes the fd, and process
B later opens /dev/net/tun and runs TUNSETIFF on the same name without
IFF_NOFILTER, the fprog.filter pointer may not be valid in process B's
address space.

Before this patch, when sk_attach_filter() failed (which was actually the
success case due to the inverted check), the err value was overwritten by
later operations (xdp_rxq_info_reg() followed by 'err = 0'), so the queue
was published and TUNSETIFF succeeded, just without a filter installed.

After this patch, if the pointer is unmapped, __get_filter() returns
-EFAULT and TUNSETIFF now fails. If it happens to be mapped but holds
other data, bpf_prepare_filter() usually returns -EINVAL and TUNSETIFF
fails. When the bytes happen to pass validation, they become the filter of
a queue that actually gets published.

This affects both the persistent TAP case described above and multi-queue
setups where another queue is opened after the buffer holding the original
filter has been freed or reused.

The next patch in the series, 508f529200c5 ("tun: keep a kernel copy of
the socket filter program"), addresses this by storing a kernel copy of
the filter program. That commit's changelog describes exactly this
behavior: "unmapped there, the attach fails with -EFAULT; mapped,
whatever bytes it holds become the filter of the new queue". It relies on
8c6a2cf8bd0a ("net: filter: add sk_attach_filter_kern() function").

Until that commit is applied, this patch changes the re-attach behavior
from succeeding (without filter) to failing (with -EFAULT or -EINVAL).
Both commits carry the same Fixes: tag, so a stable backport or bisect
that includes only this commit would encounter the problem.

Would it make sense to reorder these commits, putting the kernel-copy
change before the inverted-check fix, so every step of the series works?

Yes, thank you.  

I have reordered the series as you suggest, it now is:

  1/4 net: filter: add sk_attach_filter_kern() function
  2/4 tun: keep a kernel copy of the socket filter program
  3/4 tun: fix inverted error check when re-attaching the filter
  4/4 selftests: net: add TAP socket filter attach tests

I will send it as v3.

quoted

@@ -817,6 +819,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
 	tun_set_real_num_queues(tun);
 out:
+	if (err && rollback_filter) {
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36552651982