[PATCH net v2 0/4] tun: fix re-attaching the socket filter

[PATCH net v2 0/4] tun: fix re-attaching the socket filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v2 of the series that fixes attaching a queue to a TAP device which
re-installs the socket filter of a persistent device.

Patch 1 fixes an inverted error check in tun_attach(): a successful
re-attach returned early, so the queue was never published in
tun->tfiles[] while TUNSETIFF still reported success.  A failed re-attach
now aborts the attach, and the filter is detached again if a later step of
the attach fails.

Patch 2 adds sk_attach_filter_kern(), the kernel memory counterpart of
sk_attach_filter().

Patch 3 uses it to install a copy of the program that the kernel holds,
instead of reading tun->fprog from user space again on every later attach.
That is the second issue reported in the review of v1, fixed here together
with patch 1 as the review asked; IFF_NOFILTER keeps working and is no
longer needed as a workaround.

Patch 4 adds the selftest that a queue re-attached after the buffer the
program was copied from was made unreadable, a rejected TUNATTACHFILTER
that has to keep the saved descriptor, and attaching a queue without a
filter.

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

---
v2:
  - roll back the filter attach when a later step of tun_attach() fails
  - keep a copy of the program in the kernel, so that a later attach does
    not depend on the address space of the process that set the filter
  - add selftests for the re-attach and for a rejected TUNATTACHFILTER,
    without patch 3, two of them fail.
v1:
  - https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
---
 drivers/net/tun.c                 |  50 +++++++++-
 include/linux/filter.h            |   1 +
 net/core/filter.c                 |  22 +++++
 tools/testing/selftests/net/tun.c | 159 ++++++++++++++++++++++++++++++
 4 files changed, 227 insertions(+), 5 deletions(-)

-- 
2.43.0


No virus found
		Checked by Hillstone Network AntiVirus

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

tun_attach() returns early when sk_attach_filter() succeeds instead of when
it fails, so a queue attached to a persistent TAP is never published in
tun->tfiles[] while TUNSETIFF still reports success.

Fix the check, so that a failed re-attach aborts the attach, and detach the
filter again if a later step of tun_attach() fails.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 v2: roll back the filter attach when a later step of tun_attach() fails
 v1: https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
---
 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 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;
 	}
 
 	if (!tfile->detached &&
@@ -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;
 }
 
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH v2 2/4] net: filter: add sk_attach_filter_kern() function

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

sk_attach_filter() copies the program from user space and sk_attach_bpf()
takes it from a user file descriptor, so a program that the kernel keeps in
memory cannot be installed again later.

sk_attach_filter_kern() builds the program from a sock_fprog_kern, so no
user buffer is read, and attaches it like sk_attach_filter(). The caller
must hold the socket lock. Failing the attach releases it; so does the
socket when the filter is replaced, detached or the socket goes away.

Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 include/linux/filter.h |  1 +
 net/core/filter.c      | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 39decde7fc73..0de5a738fb26 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1218,6 +1218,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 void bpf_prog_destroy(struct bpf_prog *fp);
 
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk);
 int sk_attach_bpf(u32 ufd, struct sock *sk);
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
 int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..64d6505a4ef2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sk_attach_filter);
 
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
+{
+	struct bpf_prog *prog;
+	int err;
+
+	if (sock_flag(sk, SOCK_FILTER_LOCKED))
+		return -EPERM;
+
+	err = bpf_prog_create(&prog, fprog);
+	if (err)
+		return err;
+
+	err = __sk_attach_prog(prog, sk);
+	if (err < 0) {
+		__bpf_prog_release(prog);
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sk_attach_filter_kern);
+
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 {
 	struct bpf_prog *prog = __get_filter(fprog, sk);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH v2 3/4] tun: keep a kernel copy of the socket filter program

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

TUNATTACHFILTER copies only the sock_fprog header, so tun->fprog.filter
stays a pointer into the address space of the process that issued the
ioctl. tun_attach() reads it again whenever a queue is attached to the
persistent device later on: unmapped there, the attach fails with -EFAULT;
mapped, whatever bytes it holds become the filter of the new queue.

Keep the program in the kernel instead. tun->fprog_kern holds the
instructions and each queue gets its own program, built with
sk_attach_filter_kern().

The copy is freed when the filter is detached or replaced and with the
device, and tun->fprog is left untouched so TUNGETFILTER keeps its uapi
behaviour.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 drivers/net/tun.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a2fffef3735f..c796048742f9 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -197,6 +197,7 @@ struct tun_struct {
 	int			sndbuf;
 	struct tap_filter	txflt;
 	struct sock_fprog	fprog;
+	struct sock_fprog_kern	fprog_kern;
 	/* protected by rtnl lock */
 	bool			filter_attached;
 	u32			msg_enable;
@@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
 	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
 }
 
+/* Copy the filter that @argp points at into the kernel, so that it can be
+ * installed again later, from any context. tun->fprog and tun->fprog_kern
+ * are updated only once the copy succeeded.
+ */
+static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
+{
+	struct sock_fprog fprog;
+	struct sock_filter *insns;
+
+	if (copy_from_user(&fprog, argp, sizeof(fprog)))
+		return -EFAULT;
+
+	if (!fprog.len || fprog.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = memdup_array_user(fprog.filter, fprog.len,
+				  sizeof(struct sock_filter));
+	if (IS_ERR(insns))
+		return PTR_ERR(insns);
+
+	kfree(tun->fprog_kern.filter);
+	tun->fprog_kern.len = fprog.len;
+	tun->fprog_kern.filter = insns;
+	tun->fprog = fprog;
+
+	return 0;
+}
+
 static int tun_attach(struct tun_struct *tun, struct file *file,
 		      bool skip_filter, bool napi, bool napi_frags,
 		      bool publish_tun)
@@ -753,7 +782,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	/* Re-attach the filter to persist device */
 	if (!skip_filter && (tun->filter_attached == true)) {
 		lock_sock(tfile->socket.sk);
-		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
+		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
 		if (err)
 			goto out;
@@ -2404,6 +2433,7 @@ static void tun_free_netdev(struct net_device *dev)
 	security_tun_dev_free_security(tun->security);
 	__tun_set_ebpf(tun, &tun->steering_prog, NULL);
 	__tun_set_ebpf(tun, &tun->filter_prog, NULL);
+	kfree(tun->fprog_kern.filter);
 }
 
 static void tun_setup(struct net_device *dev)
@@ -3066,6 +3096,9 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
 		release_sock(tfile->socket.sk);
 	}
 
+	kfree(tun->fprog_kern.filter);
+	tun->fprog_kern.filter = NULL;
+	tun->fprog_kern.len = 0;
 	tun->filter_attached = false;
 }
 
@@ -3077,7 +3110,7 @@ static int tun_attach_filter(struct tun_struct *tun)
 	for (i = 0; i < tun->numqueues; i++) {
 		tfile = rtnl_dereference(tun->tfiles[i]);
 		lock_sock(tfile->socket.sk);
-		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
+		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
 		if (ret) {
 			tun_detach_filter(tun, i);
@@ -3425,8 +3458,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		ret = -EINVAL;
 		if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
 			break;
-		ret = -EFAULT;
-		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
+		ret = tun_copy_filter(tun, argp);
+		if (ret)
 			break;
 
 		ret = tun_attach_filter(tun);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

reattach_filter_without_user_buffer() attaches a filter, makes the buffer
the program was copied from unreadable and then attaches the queue again,
which used to fail with -EFAULT.

attach_filter_bad_len_keeps_descriptor() checks that a rejected
TUNATTACHFILTER leaves both the saved descriptor and the installed filter
alone.

attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
ways to attach a queue without a filter.

Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 tools/testing/selftests/net/tun.c | 159 ++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index abe488bac50b..bf8d4d6f21f7 100644
--- a/tools/testing/selftests/net/tun.c
+++ b/tools/testing/selftests/net/tun.c
@@ -8,13 +8,19 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <linux/filter.h>
 #include <linux/if_tun.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/socket.h>
 
 #include "kselftest_harness.h"
 #include "tuntap_helpers.h"
 
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
 static const char param_dev_geneve_name[] = "geneve1";
 static unsigned char param_hwaddr_outer_dst[] = { 0x00, 0xfe, 0x98,
 						  0x14, 0x22, 0x42 };
@@ -542,6 +548,159 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len */
+static const struct sock_filter filter_accept[] = {
+	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
+	BPF_STMT(BPF_RET | BPF_A, 0),
+};
+
+/* drop: return 0 */
+static const struct sock_filter filter_drop[] = {
+	BPF_STMT(BPF_RET | BPF_K, 0),
+};
+
+/* Put the instructions in an anonymous mapping, so that the test can make the
+ * address unreadable afterwards.
+ */
+static void *filter_alloc(const struct sock_filter *insns, unsigned int len)
+{
+	void *p;
+
+	p = mmap(NULL, getpagesize(), PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (p == MAP_FAILED)
+		return NULL;
+
+	memcpy(p, insns, len * sizeof(*insns));
+
+	return p;
+}
+
+static int filter_attach(int fd, const struct sock_filter *insns, unsigned int len)
+{
+	struct sock_fprog fp = {
+		.len = len,
+		.filter = (struct sock_filter *)insns,
+	};
+
+	return ioctl(fd, TUNATTACHFILTER, (void *)&fp);
+}
+
+static int filter_get(int fd, struct sock_fprog *fp)
+{
+	return ioctl(fd, TUNGETFILTER, (void *)fp);
+}
+
+static int tap_get_iff(int fd, short *flags)
+{
+	struct ifreq ifr = { 0 };
+
+	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
+		return -1;
+
+	*flags = ifr.ifr_flags;
+
+	return 0;
+}
+
+/*
+ * A queue can be attached long after the filter was configured, from a process
+ * that does not necessarily map the buffer the program was copied from, so
+ * the kernel has to keep its own copy of the program.  Here the mapping is
+ * made unreadable before the queue is attached again: re-reading the user
+ * address used to fail with -EFAULT.
+ */
+TEST_F(tun, reattach_filter_without_user_buffer)
+{
+	struct sock_fprog gf = { 0 };
+	short flags = 0;
+	void *prog;
+	int ret;
+
+	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
+	ASSERT_NE(prog, NULL);
+	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+
+	/* The process that called TUNATTACHFILTER no longer maps this memory */
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
+
+	ret = tun_attach(self->fd, self->ifname);
+	EXPECT_EQ(ret, 0);
+
+	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
+	ASSERT_EQ(munmap(prog, getpagesize()), 0);
+}
+
+/* A new queue attached with IFF_NOFILTER must not get the filter that is
+ * configured on the device.
+ */
+TEST_F(tun, attach_filter_nofilter_flag)
+{
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	int fd;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
+
+	fd = open("/dev/net/tun", O_RDWR);
+	ASSERT_GE(fd, 0);
+
+	strcpy(ifr.ifr_name, self->ifname);
+	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	EXPECT_EQ(tap_get_iff(fd, &flags), 0);
+	EXPECT_NE(flags & IFF_NOFILTER, 0);
+
+	close(fd);
+}
+
+/* After TUNDETACHFILTER a later attach must not install the filter again */
+TEST_F(tun, detach_filter_clears_reattach)
+{
+	short flags = 0;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
+	EXPECT_EQ(ioctl(self->fd, TUNDETACHFILTER, 0), 0);
+
+	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
+	EXPECT_NE(flags & IFF_NOFILTER, 0);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_attach(self->fd, self->ifname), 0);
+
+	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
+	EXPECT_NE(flags & IFF_NOFILTER, 0);
+}
+
+/* A TUNATTACHFILTER with a bad length must not clobber the saved descriptor */
+TEST_F(tun, attach_filter_bad_len_keeps_descriptor)
+{
+	struct sock_fprog gf = { 0 };
+	short flags = 0;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
+
+	errno = 0;
+	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+}
+
 FIXTURE(tun_vnet_udptnl)
 {
 	char ifname[IFNAMSIZ];
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3840 bytes --]

> 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?

> @@ -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

Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50b..bf8d4d6f21f7 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -8,13 +8,19 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <linux/filter.h>
>  #include <linux/if_tun.h>
>  #include <sys/ioctl.h>
> +#include <sys/mman.h>
>  #include <sys/socket.h>
>
>  #include "kselftest_harness.h"
>  #include "tuntap_helpers.h"
>
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif

This isn't a bug, but kselftest.h already provides ARRAY_SIZE through
kselftest_harness.h. Is this #ifndef block needed, or can it be dropped?

Looking at the include chain: kselftest_harness.h includes kselftest.h,
and kselftest.h defines ARRAY_SIZE at line 63-65. kselftest_harness.h also
defines it again at line 749. Other harness-based tests in the same
directory (tls.c, ip_local_port_range.c) use ARRAY_SIZE without defining
it themselves.

[ ... ]


---
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

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

[weirongguang]

on 2026/9/29 18:34, bot+bpf-ci@kernel.org wrote:
>> 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.
>> @@ -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

Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[weirongguang]

on 2026/9/29 18:34, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
>> index abe488bac50b..bf8d4d6f21f7 100644
>> --- a/tools/testing/selftests/net/tun.c
>> +++ b/tools/testing/selftests/net/tun.c
>> @@ -8,13 +8,19 @@
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <unistd.h>
>> +#include <linux/filter.h>
>>  #include <linux/if_tun.h>
>>  #include <sys/ioctl.h>
>> +#include <sys/mman.h>
>>  #include <sys/socket.h>
>>
>>  #include "kselftest_harness.h"
>>  #include "tuntap_helpers.h"
>>
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +#endif
> 
> This isn't a bug, but kselftest.h already provides ARRAY_SIZE through
> kselftest_harness.h. Is this #ifndef block needed, or can it be dropped?
> 
> Looking at the include chain: kselftest_harness.h includes kselftest.h,
> and kselftest.h defines ARRAY_SIZE at line 63-65. kselftest_harness.h also
> defines it again at line 749. Other harness-based tests in the same
> directory (tls.c, ip_local_port_range.c) use ARRAY_SIZE without defining
> it themselves.
> 
Yes, thank you.

I will drop it in v3.
> [ ... ]
> 
> 
> ---
> 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

Re: [PATCH v2 2/4] net: filter: add sk_attach_filter_kern() function

[Willem de Bruijn]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> sk_attach_filter() copies the program from user space and sk_attach_bpf()
> takes it from a user file descriptor, so a program that the kernel keeps in
> memory cannot be installed again later.
> 
> sk_attach_filter_kern() builds the program from a sock_fprog_kern, so no
> user buffer is read, and attaches it like sk_attach_filter(). The caller
> must hold the socket lock. Failing the attach releases it; so does the
> socket when the filter is replaced, detached or the socket goes away.
> 
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

This should probably be squashed into the next commit, that first uses it.

> ---
>  include/linux/filter.h |  1 +
>  net/core/filter.c      | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 39decde7fc73..0de5a738fb26 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1218,6 +1218,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
>  void bpf_prog_destroy(struct bpf_prog *fp);
>  
>  int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk);
>  int sk_attach_bpf(u32 ufd, struct sock *sk);
>  int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
>  int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2..64d6505a4ef2 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>  }
>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>  
> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
> +{
> +	struct bpf_prog *prog;
> +	int err;
> +
> +	if (sock_flag(sk, SOCK_FILTER_LOCKED))
> +		return -EPERM;
> +
> +	err = bpf_prog_create(&prog, fprog);
> +	if (err)
> +		return err;
> +
> +	err = __sk_attach_prog(prog, sk);
> +	if (err < 0) {
> +		__bpf_prog_release(prog);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(sk_attach_filter_kern);
> +
>  int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>  {
>  	struct bpf_prog *prog = __get_filter(fprog, sk);
> -- 
> 2.25.1
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus
> 

Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[Willem de Bruijn]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> reattach_filter_without_user_buffer() attaches a filter, makes the buffer
> the program was copied from unreadable and then attaches the queue again,
> which used to fail with -EFAULT.
> 
> attach_filter_bad_len_keeps_descriptor() checks that a rejected
> TUNATTACHFILTER leaves both the saved descriptor and the installed filter
> alone.
> 
> attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
> ways to attach a queue without a filter.
> 
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
  
> +/* accept: return skb->len */
> +static const struct sock_filter filter_accept[] = {
> +	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
> +	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
> +	BPF_STMT(BPF_RET | BPF_A, 0),
> +};
> +
> +/* drop: return 0 */
> +static const struct sock_filter filter_drop[] = {
> +	BPF_STMT(BPF_RET | BPF_K, 0),
> +};

Are both programs needed? One will do, right?

> +static int tap_get_iff(int fd, short *flags)

nit: tun_get_iff? Or even tun_get_iff_flags.

Also, could just return flags if positive, no call-by-reference needed.

> +{
> +	struct ifreq ifr = { 0 };
> +
> +	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
> +		return -1;
> +
> +	*flags = ifr.ifr_flags;
> +
> +	return 0;
> +}
> +

> +/* A new queue attached with IFF_NOFILTER must not get the filter that is
> + * configured on the device.
> + */
> +TEST_F(tun, attach_filter_nofilter_flag)
> +{
> +	struct ifreq ifr = { 0 };
> +	short flags = 0;
> +	int fd;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
> +
> +	fd = open("/dev/net/tun", O_RDWR);
> +	ASSERT_GE(fd, 0);
> +
> +	strcpy(ifr.ifr_name, self->ifname);
> +	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
> +	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
> +
> +	EXPECT_EQ(tap_get_iff(fd, &flags), 0);
> +	EXPECT_NE(flags & IFF_NOFILTER, 0);

nit: here and elsewhere: avoid the double negative?

	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER)

> +/* A TUNATTACHFILTER with a bad length must not clobber the saved descriptor */

What is the descriptor in this context? Do you mean installed program?

> +TEST_F(tun, attach_filter_bad_len_keeps_descriptor)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
> +	EXPECT_EQ(errno, EINVAL);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
> +
> +	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
> +}
> +
>  FIXTURE(tun_vnet_udptnl)
>  {
>  	char ifname[IFNAMSIZ];
> -- 
> 2.25.1
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus
> 

Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[weirongguang]

on 2026/9/30 10:48, Willem de Bruijn wrote:
> Rongguang Wei wrote:
>> From: Rongguang Wei <weirongguang@kylinos.cn>
>>
>> reattach_filter_without_user_buffer() attaches a filter, makes the buffer
>> the program was copied from unreadable and then attaches the queue again,
>> which used to fail with -EFAULT.
>>
>> attach_filter_bad_len_keeps_descriptor() checks that a rejected
>> TUNATTACHFILTER leaves both the saved descriptor and the installed filter
>> alone.
>>
>> attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
>> ways to attach a queue without a filter.
>>
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
>   
>> +/* accept: return skb->len */
>> +static const struct sock_filter filter_accept[] = {
>> +	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
>> +	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
>> +	BPF_STMT(BPF_RET | BPF_A, 0),
>> +};
>> +
>> +/* drop: return 0 */
>> +static const struct sock_filter filter_drop[] = {
>> +	BPF_STMT(BPF_RET | BPF_K, 0),
>> +};
> 
> Are both programs needed? One will do, right?
> 
Yes, I dropped filter_drop and use filter_accept.
>> +static int tap_get_iff(int fd, short *flags)
> 
> nit: tun_get_iff? Or even tun_get_iff_flags.
> 
> Also, could just return flags if positive, no call-by-reference needed.
> 
Rename to tun_get_iff_flags() now and returns the flags, the call sites like:

	flags = tun_get_iff_flags(fd);
	EXPECT_GE(flags, 0);
>> +{
>> +	struct ifreq ifr = { 0 };
>> +
>> +	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
>> +		return -1;
>> +
>> +	*flags = ifr.ifr_flags;
>> +
>> +	return 0;
>> +}
>> +
> 
>> +/* A new queue attached with IFF_NOFILTER must not get the filter that is
>> + * configured on the device.
>> + */
>> +TEST_F(tun, attach_filter_nofilter_flag)
>> +{
>> +	struct ifreq ifr = { 0 };
>> +	short flags = 0;
>> +	int fd;
>> +
>> +	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
>> +
>> +	fd = open("/dev/net/tun", O_RDWR);
>> +	ASSERT_GE(fd, 0);
>> +
>> +	strcpy(ifr.ifr_name, self->ifname);
>> +	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
>> +	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
>> +
>> +	EXPECT_EQ(tap_get_iff(fd, &flags), 0);
>> +	EXPECT_NE(flags & IFF_NOFILTER, 0);
> 
> nit: here and elsewhere: avoid the double negative?
>> 	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER)
Done, all three of them.
>
>> +/* A TUNATTACHFILTER with a bad length must not clobber the saved descriptor */
> 
> What is the descriptor in this context? Do you mean installed program?
> 
Yes, the installed program.

Thank you, I will send the series as v3, which also puts the kernel copy of the program
before the corrected error check, as suggested in the CI review of patch 1.
>> +TEST_F(tun, attach_filter_bad_len_keeps_descriptor)
>> +{
>> +	struct sock_fprog gf = { 0 };
>> +	short flags = 0;
>> +
>> +	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
>> +
>> +	errno = 0;
>> +	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
>> +	EXPECT_EQ(errno, EINVAL);
>> +
>> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
>> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
>> +
>> +	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
>> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
>> +}
>> +
>>  FIXTURE(tun_vnet_udptnl)
>>  {
>>  	char ifname[IFNAMSIZ];
>> -- 
>> 2.25.1
>>
>>
>> No virus found
>> 		Checked by Hillstone Network AntiVirus
>>

Re: [PATCH v2 2/4] net: filter: add sk_attach_filter_kern() function

[Rongguang Wei]

on 2026/9/30 10:43, Willem de Bruijn wrote:
> Rongguang Wei wrote:
>> From: Rongguang Wei <weirongguang@kylinos.cn>
>>
>> sk_attach_filter() copies the program from user space and sk_attach_bpf()
>> takes it from a user file descriptor, so a program that the kernel keeps in
>> memory cannot be installed again later.
>>
>> sk_attach_filter_kern() builds the program from a sock_fprog_kern, so no
>> user buffer is read, and attaches it like sk_attach_filter(). The caller
>> must hold the socket lock. Failing the attach releases it; so does the
>> socket when the filter is replaced, detached or the socket goes away.
>>
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> 
> This should probably be squashed into the next commit, that first uses it.
> 
Agreed, it is now part of that patch. I will send the series as v3.
>> ---
>>  include/linux/filter.h |  1 +
>>  net/core/filter.c      | 22 ++++++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 39decde7fc73..0de5a738fb26 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -1218,6 +1218,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
>>  void bpf_prog_destroy(struct bpf_prog *fp);
>>  
>>  int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
>> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk);
>>  int sk_attach_bpf(u32 ufd, struct sock *sk);
>>  int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
>>  int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 70dc621672f2..64d6505a4ef2 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>  }
>>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>>  
>> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
>> +{
>> +	struct bpf_prog *prog;
>> +	int err;
>> +
>> +	if (sock_flag(sk, SOCK_FILTER_LOCKED))
>> +		return -EPERM;
>> +
>> +	err = bpf_prog_create(&prog, fprog);
>> +	if (err)
>> +		return err;
>> +
>> +	err = __sk_attach_prog(prog, sk);
>> +	if (err < 0) {
>> +		__bpf_prog_release(prog);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(sk_attach_filter_kern);
>> +
>>  int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>  {
>>  	struct bpf_prog *prog = __get_filter(fprog, sk);
>> -- 
>> 2.25.1
>>
>>
>> No virus found
>> 		Checked by Hillstone Network AntiVirus
>>
> 

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch un-inverts the check in tun_attach()
  (drivers/net/tun.c), so a failed filter re-attach now aborts TUNSETIFF…

--

tun: fix inverted error check when re-attaching the filter

This patch fixes the inverted sk_attach_filter() error check in
tun_attach(). Before, a successful filter re-attach returned early and the
queue was never published in tun->tfiles[]. Now a failed re-attach aborts
the attach, and the filter is detached again if a later step of
tun_attach() fails.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..a2fffef3735fe 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -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;
>  	}

[Severity: Medium]
A failed re-attach is now fatal. Can that make TUNSETIFF, or TUNSETQUEUE
with IFF_ATTACH_QUEUE, fail on a persistent TAP that has a filter
attached?

tun->fprog only holds the {len, filter} header saved at TUNATTACHFILTER
time, and filter is still a __user pointer:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
		ret = -EFAULT;
		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
			break;
    ...
}

sk_attach_filter() then reads the instructions again, from the address
space of whichever task is doing the attach:

net/core/filter.c:__get_filter() {
    ...
	if (copy_from_user(prog->insns, fprog->filter, fsize))
    ...
}

Consider this sequence:

1. Process A issues TUNATTACHFILTER.
2. A exits or frees the buffer.
3. Process B issues TUNSETIFF (without IFF_NOFILTER) or IFF_ATTACH_QUEUE.

The copy in step 3 fails with -EFAULT, or the program fails validation
with -EINVAL, and the attach is aborted. Before this patch the attach went
ahead without a filter. If the stale address happens to be mapped in B,
the filter is built from whatever bytes are at that address.

tun_set_queue() always passes skip_filter=false, so IFF_ATTACH_QUEUE has
no way to opt out:

		ret = tun_attach(tun, file, false, tun->flags & IFF_NAPI,
				 tun->flags & IFF_NAPI_FRAGS, true);

A later patch in this series, "tun: keep a kernel copy of the socket
filter program", seems to fix this:

- At TUNATTACHFILTER time, tun_copy_filter() copies the instructions into
  tun->fprog_kern.
- tun_attach() and tun_attach_filter() switch to
  sk_attach_filter_kern(&tun->fprog_kern, ...).
- The selftest reattach_filter_without_user_buffer() covers this case.

Would it make sense to put that patch before this one? That way the
stale user pointer read is never fatal at any point in the series.

A smaller gap is still there at the end of the series. It predates this
patch. If TUNATTACHFILTER is issued from a detached queue fd while
numqueues is 0, tun_attach_filter() loops zero times. It then sets
filter_attached = true without running bpf_check_classic(). Later
attaches fail with -EINVAL until TUNDETACHFILTER is issued.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929093712.131096-1-clementwei90%40163.com

Re: [PATCH v2 2/4] net: filter: add sk_attach_filter_kern() function

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] sk_attach_filter_kern() compiles stored classic BPF
  instructions in whatever task happens to call it.
- [Low] sk_attach_filter_kern() (net/core/filter.c:1570-1590) builds its
  program with bpf_prog_create() and publishes it in sk->sk_filter…

--

net: filter: add sk_attach_filter_kern() function

This adds sk_attach_filter_kern(). It builds a classic BPF socket filter
from a kernel-resident sock_fprog_kern using bpf_prog_create(), then
attaches it with __sk_attach_prog(). The kernel can then reinstall a
stored filter without reading a user buffer or file descriptor again.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..64d6505a4ef23 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>  }
>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>  
> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
> +{
> +	struct bpf_prog *prog;
> +	int err;
> +
> +	if (sock_flag(sk, SOCK_FILTER_LOCKED))
> +		return -EPERM;
> +
> +	err = bpf_prog_create(&prog, fprog);

[Severity: Low]

bpf_prog_create() is documented as creating an unattached filter. It
drops the original program on purpose:

net/core/filter.c:bpf_prog_create() {
    ...
	/* Since unattached filters are not copied back to user
	 * space through sk_get_filter(), we do not need to hold
	 * a copy here, and can spare us the work.
	 */
	fp->orig_prog = NULL;
    ...
}

With this new export, that program ends up in sk->sk_filter. Can a
filter attached this way still be read back?

When orig_prog is NULL, sk_get_filter() returns -EACCES. Its comment
says this only happens for eBPF programs:

net/core/filter.c:sk_get_filter() {
    ...
	/* We're copying the filter that has been originally attached,
	 * so no conversion/decode needed anymore. eBPF programs that
	 * have no original program cannot be dumped through this.
	 */
	ret = -EACCES;
	fprog = filter->prog->orig_prog;
	if (!fprog)
		goto out;
    ...
}

In the same case, sock_diag_put_filterinfo() leaves out the filter
attribute without any error.

sk_attach_filter() goes through __get_filter()->bpf_prog_store_orig_filter()
and keeps the original program. So for classic filters, the two exported
attach APIs now behave differently for SO_GET_FILTER and sock_diag.

The tun consumer later in this series does not seem to be affected in a
way userspace can see. SO_GET_FILTER and sock_diag cannot reach its
sockets, and TUNGETFILTER returns tun->fprog.

Could this store orig_prog, the way bpf_prog_create_from_user() does
with save_orig? If not, could the limitation be documented and the
comments in bpf_prog_create() and sk_get_filter() updated?

[Severity: Medium]

Does this change whose credentials decide JIT hardening?

bpf_prog_create() calls bpf_prog_alloc(), and bpf_prog_alloc_no_stats()
sets:

	fp->blinding_requested = bpf_jit_blinding_enabled(fp);

When bpf_jit_harden is 1, bpf_jit_blinding_enabled() decides based on
current:

	if (bpf_jit_harden == 1 && bpf_token_capable(prog->aux->token, CAP_BPF))
		return false;

The instructions come from whoever stored them in the sock_fprog_kern.
Blinding, however, depends on whichever task calls
sk_attach_filter_kern() later.

With the tun consumer later in this series, this sequence looks possible:

1. An unprivileged owner of a persistent multi-queue TAP passes
   tun_not_capable() and issues TUNATTACHFILTER. The instructions are
   stored in tun->fprog_kern. The programs built for the queues attached
   at that point are blinded.

2. Later, a process with CAP_BPF or CAP_SYS_ADMIN attaches another queue
   using TUNSETIFF or TUNSETQUEUE.

3. tun_attach() calls sk_attach_filter_kern(&tun->fprog_kern, ...) with
   the privileged credentials. The unprivileged user's filter goes
   through bpf_prepare_filter()->bpf_migrate_filter()->
   bpf_prog_select_runtime() and is JIT-compiled without constant
   blinding.

Before this change, sk_attach_filter(&tun->fprog, ...) read the
instructions again from the attaching process's own address space. The
unprivileged user did not control the bytes compiled in the privileged
context.

With bpf_jit_harden=1, does this let constants chosen by the user into
JIT memory that is not blinded? Could the blinding decision be recorded
when the instructions are supplied, or be tied to where the program came
from rather than to current at attach time?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929093712.131096-1-clementwei90%40163.com

Re: [PATCH v2 3/4] tun: keep a kernel copy of the socket filter program

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This commit carries `Fixes: 54f968d6efdb`, so it will be picked
  for stable.
- [Low] For TUNATTACHFILTER, tun_copy_filter() replaces the core
  bpf_check_basics_ok() validation with its own checks, and the two…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a failed
  filter replacement leaves the queues in mixed states.
- [Medium] This problem wasn't introduced by this patch, but
  TUNDETACHFILTER only detaches the filter from the tun->numqueues…
- [Low] This problem wasn't introduced by this patch, but when every
  queue of a multiqueue TAP is disabled with IFF_DETACH_QUEUE,…

--

tun: keep a kernel copy of the socket filter program

With this commit, TUNATTACHFILTER copies the classic BPF instructions
into tun->fprog_kern instead of keeping only the user pointer in
tun->fprog. tun_attach() and tun_attach_filter() then build each queue's
filter from that kernel copy through sk_attach_filter_kern(). The copy
is freed on detach, when it is replaced, and in tun_free_netdev().

Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a2fffef3735fe..c796048742f9b 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>  }
>  
> +/* Copy the filter that @argp points at into the kernel, so that it can be
> + * installed again later, from any context. tun->fprog and tun->fprog_kern
> + * are updated only once the copy succeeded.
> + */
> +static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
> +{
> +	struct sock_fprog fprog;
> +	struct sock_filter *insns;
> +
> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
> +		return -EFAULT;
> +
> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
> +		return -EINVAL;
> +
> +	insns = memdup_array_user(fprog.filter, fprog.len,
> +				  sizeof(struct sock_filter));

[Severity: Low]
Does this change the errno TUNATTACHFILTER returns when fprog.filter is
NULL and fprog.len is nonzero?

Before this patch, sk_attach_filter()->__get_filter()->bpf_check_basics_ok()
rejected a NULL filter with -EINVAL, the same errno SO_ATTACH_FILTER
returns. Here only the length is checked. memdup_array_user(NULL, ...)
then fails in copy_from_user(), and the ioctl returns -EFAULT.

The bpf_check_basics_ok() call inside bpf_prog_create() only sees the
kernel copy, which is never NULL, so it can't catch this case.

Would adding a !fprog.filter test to the length check keep the old
-EINVAL?

> +	if (IS_ERR(insns))
> +		return PTR_ERR(insns);
> +
> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.len = fprog.len;
> +	tun->fprog_kern.filter = insns;
> +	tun->fprog = fprog;
> +
> +	return 0;
> +}
> +
>  static int tun_attach(struct tun_struct *tun, struct file *file,
>  		      bool skip_filter, bool napi, bool napi_frags,
>  		      bool publish_tun)
> @@ -753,7 +782,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	/* Re-attach the filter to persist device */
>  	if (!skip_filter && (tun->filter_attached == true)) {
>  		lock_sock(tfile->socket.sk);
> -		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);

[Severity: Low]
Should this commit record that it depends on the helper commit? It
carries Fixes: 54f968d6efdb, so it will be picked for stable.

sk_attach_filter_kern() is only added by the previous commit in the
series, "net: filter: add sk_attach_filter_kern() function". That commit
has no Fixes: tag, no Cc: stable and no stable dependency annotation.

This commit's message doesn't say it depends on that commit, or on "tun:
fix inverted error check when re-attaching the filter".

If it is backported by itself, tun_attach() and tun_attach_filter() won't
build. sk_attach_filter_kern is implicitly declared, and the symbol is
undefined when tun is built as a module.

Would a Cc: stable tag with a dependency note on the helper commit
prevent that?

>  		release_sock(tfile->socket.sk);
>  		if (err)
>  			goto out;

[ ... ]

> @@ -3066,6 +3096,9 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
>  		release_sock(tfile->socket.sk);
>  	}
>  
> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.filter = NULL;
> +	tun->fprog_kern.len = 0;
>  	tun->filter_attached = false;

[Severity: Medium]
This isn't a bug introduced by this patch, but does TUNDETACHFILTER leave
the filter on queues that are disabled at that point?

tun_detach_filter() only walks the first n == tun->numqueues entries:

	for (i = 0; i < n; i++) {
		tfile = rtnl_dereference(tun->tfiles[i]);
		lock_sock(tfile->socket.sk);
		sk_detach_filter(tfile->socket.sk);
		...

A queue disabled through TUNSETQUEUE with IFF_DETACH_QUEUE
(tun_set_queue()->__tun_detach(tfile, false)) keeps its sk_filter.

When that queue is re-enabled with IFF_ATTACH_QUEUE, tun_attach() skips
the filter block because filter_attached is false. It doesn't clear the
leftover filter either.

tun_net_xmit() checks tfile->socket.sk->sk_filter directly. So after
TUNDETACHFILTER succeeds, the re-enabled queue still applies the old
filter.

>  }
>  
> @@ -3077,7 +3110,7 @@ static int tun_attach_filter(struct tun_struct *tun)
>  	for (i = 0; i < tun->numqueues; i++) {
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		lock_sock(tfile->socket.sk);
> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
>  		if (ret) {
>  			tun_detach_filter(tun, i);

[Severity: Medium]
This is a pre-existing issue, but can a failed filter replacement leave
the queues in mixed states?

Say filter A is attached and TUNATTACHFILTER is issued with program B.
tun_copy_filter() frees A's kernel copy and stores B. tun_attach_filter()
then fails on queue i. That happens if bpf_check_classic() rejects B
(i == 0), or if bpf_prog_alloc() or __sk_filter_charge() fails at any i.

tun_detach_filter(tun, i) detaches only queues [0, i). It also frees B's
copy and sets filter_attached = false.

Queues [i, numqueues) keep running filter A, but the device-level state
says no filter is attached. Queues attached later through tun_attach()
get no filter at all.

The code behaved the same way before this patch.

> @@ -3425,8 +3458,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  		ret = -EINVAL;
>  		if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
>  			break;
> -		ret = -EFAULT;
> -		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
> +		ret = tun_copy_filter(tun, argp);
> +		if (ret)
>  			break;
>  
>  		ret = tun_attach_filter(tun);

[Severity: Low]
This isn't a bug introduced by this patch, but what happens when every
queue of a multiqueue TAP has been disabled with IFF_DETACH_QUEUE?

tun->numqueues is 0 then, but tfile->tun is still set, so tun_get() still
reaches TUNATTACHFILTER.

tun_copy_filter() checks only the length and copies the instructions.
The loop in tun_attach_filter() runs zero times, so filter_attached is
set to true and 0 is returned. bpf_check_classic() never runs.

A later IFF_ATTACH_QUEUE calls sk_attach_filter_kern() from tun_attach().
bpf_prepare_filter() then rejects the saved program with -EINVAL. The
queue can't be re-enabled until the owner issues TUNDETACHFILTER or a
valid TUNATTACHFILTER.

This patch narrows the problem a little: before its length check, this
path accepted even len == 0.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929093712.131096-1-clementwei90%40163.com

Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:614-641), only the tun_attach()…
- [Low] attach_filter_bad_len_keeps_descriptor() only checks "the
  installed filter" through IFF_NOFILTER on self->fd…
- [Low] attach_filter_nofilter_flag()
  (tools/testing/selftests/net/tun.c:646-665) only checks that a queue…
- [Low] In reattach_filter_without_user_buffer() the inline comment at
  tools/testing/selftests/net/tun.c:627 says "The process that called…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests to tools/testing/selftests/net/tun.c for
TUNATTACHFILTER handling. They cover four cases:

- re-attaching a queue after the user buffer that held the filter is
  made unreadable
- attaching a queue with IFF_NOFILTER
- attaching a queue after TUNDETACHFILTER
- rejecting a zero-length filter

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50bb..bf8d4d6f21f77 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> @@ -542,6 +548,159 @@ TEST_F(tun, reattach_close_delete)

[ ... ]

> +TEST_F(tun, reattach_filter_without_user_buffer)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +	void *prog;
> +	int ret;
> +
> +	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
> +	ASSERT_NE(prog, NULL);
> +	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
> +
> +	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
> +
> +	/* The process that called TUNATTACHFILTER no longer maps this memory */
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);

[Severity: Low]
This isn't a bug, but is this comment accurate? mprotect(PROT_NONE) makes
the page unreadable, but it stays mapped. It is only unmapped at the end of
the test by munmap().

The header comment above the test says the mapping is "made unreadable",
which matches the code.

copy_from_user() faults the same way in both cases, so the test still works.

> +
> +	ret = tun_attach(self->fd, self->ifname);
> +	EXPECT_EQ(ret, 0);
> +
> +	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Low]
Can these last two checks tell whether tun_attach() actually re-installed
the filter?

The tun_detach() helper uses TUNSETQUEUE with IFF_DETACH_QUEUE, which ends
up in __tun_detach(tfile, false). That path disables the queue but never
calls sk_detach_filter(). The socket therefore keeps the filter from the
first TUNATTACHFILTER:

drivers/net/tun.c:__tun_detach() {
    ...
		} else {
			tun_disable_queue(tun, tfile);
			tun_napi_disable(tfile);
		}
    ...
}

TUNGETIFF reports IFF_NOFILTER only when sk_filter is NULL:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
		if (!tfile->socket.sk->sk_filter)
			ifr.ifr_flags |= IFF_NOFILTER;
    ...
}

So the IFF_NOFILTER check passes whether or not this block in tun_attach()
ran:

drivers/net/tun.c:tun_attach() {
    ...
	if (!skip_filter && (tun->filter_attached == true)) {
		lock_sock(tfile->socket.sk);
		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
    ...
}

TUNGETFILTER returns tun->fprog, which is the saved user descriptor, not
fprog_kern:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
	case TUNGETFILTER:
    ...
		if (copy_to_user(argp, &tun->fprog, sizeof(tun->fprog)))
    ...
}

So gf.len == 3 doesn't show which instructions the queue is running.

Only the ret == 0 check can tell a fixed kernel from a broken one. If the
re-install block were skipped, or filter_attached were wrongly false, the
rest of the test would still pass.

The comment describes a queue "attached long after the filter was
configured". The test, however, reuses a queue that already has a filter.

Could the test also attach a new queue with TUNSETIFF and without
IFF_NOFILTER? On that queue, fprog_kern is the only source of the filter.

It could also overwrite the still-readable buffer, for example with
filter_drop. That would cover the other hazard described in "tun: keep a
kernel copy of the socket filter program".

> +
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
> +	ASSERT_EQ(munmap(prog, getpagesize()), 0);
> +}
> +
> +/* A new queue attached with IFF_NOFILTER must not get the filter that is
> + * configured on the device.
> + */
> +TEST_F(tun, attach_filter_nofilter_flag)
> +{
> +	struct ifreq ifr = { 0 };
> +	short flags = 0;
> +	int fd;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
> +
> +	fd = open("/dev/net/tun", O_RDWR);
> +	ASSERT_GE(fd, 0);
> +
> +	strcpy(ifr.ifr_name, self->ifname);
> +	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
> +	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
> +
> +	EXPECT_EQ(tap_get_iff(fd, &flags), 0);
> +	EXPECT_NE(flags & IFF_NOFILTER, 0);

[Severity: Low]
Is there a positive control for this test? tun_set_iff() passes the
IFF_NOFILTER bit to tun_attach() as skip_filter:

drivers/net/tun.c:tun_set_iff() {
    ...
		err = tun_attach(tun, file, ifr->ifr_flags & IFF_NOFILTER,
    ...
}

None of the new tests opens a new queue with TUNSETIFF and without
IFF_NOFILTER after TUNATTACHFILTER, then checks that the device filter is
installed on it. Both fixture queues are created in FIXTURE_SETUP, before
any filter exists.

Suppose filter_attached were never set, or the re-install branch in
tun_attach() never ran. The new socket here would have no filter, and this
check would still pass.

Would it help to also open a queue without IFF_NOFILTER and check that
IFF_NOFILTER is clear on it?

> +
> +	close(fd);
> +}

[ ... ]

> +/* A TUNATTACHFILTER with a bad length must not clobber the saved descriptor */
> +TEST_F(tun, attach_filter_bad_len_keeps_descriptor)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
> +	EXPECT_EQ(errno, EINVAL);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
> +
> +	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);

[Severity: Low]
The commit message says this test checks that a rejected TUNATTACHFILTER
"leaves both the saved descriptor and the installed filter alone". Can this
IFF_NOFILTER check detect a lost filter?

The check reads sk_filter on self->fd. A failed attach on queue 0 never
touches sk_filter, even on the kernel before this series.

In the old code, len=0 was copied into tun->fprog. tun_attach_filter() then
failed at i=0. tun_detach_filter(tun, 0) detached no sockets, but it still
cleared the device state:

drivers/net/tun.c:tun_detach_filter() {
    ...
	tun->filter_attached = false;
}

The socket kept its filter, so this check passed even though the device had
silently lost its re-attach state. On the old kernel, only the gf.len check
fails.

tun_attach() reads tun->filter_attached and tun->fprog_kern on later
attaches. The test never attaches a queue again after the failed ioctl.

A regression could clear filter_attached or free fprog_kern before the
length check while keeping tun->fprog. That regression would pass this test.

Could the test detach and re-attach the queue (or add a new one) after the
failed TUNATTACHFILTER, then check IFF_NOFILTER? That would show the
installed filter is really kept.

> +}
> +
>  FIXTURE(tun_vnet_udptnl)
>  {
>  	char ifname[IFNAMSIZ];

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929093712.131096-1-clementwei90%40163.com