[PATCH net v7 0/2] tun: fix re-attaching the socket filter

[PATCH net v7 0/2] tun: fix re-attaching the socket filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

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

Patch 1 keeps a copy of the program in the kernel when it is configured,
instead of reading tun->fprog from user space again on every later attach,
and fixes the inverted error check in tun_attach(). The two changes are
one patch on purpose: with only the kernel copy every re-attach returns 0
without publishing the queue, and with only the check fixed a re-attach
that used to succeed without a filter starts to fail. IFF_NOFILTER keeps
working and is no longer needed to work around the re-attach bug. The
program is verified with bpf_prog_validate_classic() when it is copied in,
so a program that no verifier accepts is rejected even when no queue is
attached.

Patch 2 adds selftests: it re-attaches a queue after the buffer the program
was copied from was made unreadable, attaches a second queue from a fresh
socket to check that the filter is rebuilt from the kernel copy, and checks
that a TUNATTACHFILTER with a bad length or bad instructions keeps the
saved program, plus the ways to attach a queue without a filter.

Thanks to Willem de Bruijn for the reviews, and to the CI reviews for
the comments they raised.

Rongguang Wei (2):
  tun: keep a kernel copy of the socket filter program
  selftests: net: add TAP socket filter attach tests

---
v7:
  - validate the program with bpf_prog_validate_classic() in
    tun_copy_filter(), so an invalid program is rejected even when no
    queue is attached
  - fix the kernel-doc of sk_attach_filter_kern(): drop a reference to a
    function that does not exist and complete the Return section
  - make bpf_prog_validate_classic() reject a program whose length is out
    of range, like bpf_prog_create() does, and say so in its kernel-doc
  - document why the filter is dropped when a later step of tun_attach()
    fails, and why filter_accept[] has three instructions
  - selftests: add a case for a rejected program with valid length and
    bad instructions
v6:
  - add description for return value of sk_attach_filter_kern(); no code
    change is involved
  - https://lore.kernel.org/netdev/aa6f8ef9-56d8-4edb-9ae2-d3f3b7dc1721@163.com/
v5:
  - add kernel-doc for sk_attach_filter_kern() describing what the missing
    original program means for SO_GET_FILTER and sock_diag
  - mention in the message that a rejected header now leaves the installed
    filter alone
  - fix the wording of the mprotect() comment and note what the IFF_NOFILTER
    and TUNGETFILTER checks in the re-attach test
  - https://lore.kernel.org/netdev/20261012021113.45939-1-clementwei90@163.com/
v4:
  - merge the kernel copy and the error check into one patch, so that no
    step of the series turns a re-attach into a silent no-op or into a new
    failure
  - charge the kernel copy to the caller's memory cgroup
  - selftests: check that the queue is attached again, and attach a new
    queue to see that the filter was installed from the kernel copy
  - https://lore.kernel.org/netdev/20261003063859.136895-1-clementwei90@163.com/
v3:
  - reorder: the kernel copy of the program comes before the corrected
    error check
  - add sk_attach_filter_kern() in the patch that first uses it
  - address the review of the selftests
  - https://lore.kernel.org/netdev/20260930080746.135017-1-clementwei90@163.com/
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
  - https://lore.kernel.org/netdev/20260929093712.131096-1-clementwei90@163.com/
v1:
  - https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
---
 drivers/net/tun.c                 |  76 +++++++++-
 include/linux/filter.h            |   2 +
 net/core/filter.c                 |  62 +++++++++
 tools/testing/selftests/net/tun.c | 223 ++++++++++++++++++++++++++++++
 4 files changed, 358 insertions(+), 5 deletions(-)


base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
-- 
2.43.0


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v7 1/2] 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.

Rebuilding the filter from that pointer is not reliable. With the inverted
check below, a failure of the attach was not fatal: -EFAULT for an unmapped
address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
so the queue was attached without a filter. And when the read succeeded,
the early return meant the queue was never published at all.

Keep the program in the kernel instead: tun->fprog_kern holds the
instructions, and each queue gets its own program built from it with the
new sk_attach_filter_kern(), the kernel memory counterpart of
sk_attach_filter(). 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.

Also fix the inverted check, which made tun_attach() return early when
sk_attach_filter_kern() succeeded instead of when it failed. Neither change
works on its own: with only the kernel copy, every re-attach returns 0
without publishing the queue; with only the check fixed, a re-attach that
used to succeed without installing a filter would start to fail.

sk_attach_filter_kern() builds the program with bpf_prog_create(), which
does not keep an original program, so SO_GET_FILTER returns -EACCES and
sock_diag omits the filter for sockets that use it. tun sockets are not
exposed as file descriptors, so this is not user visible.

The header and the instructions are checked in tun_copy_filter() before
anything is replaced: a TUNATTACHFILTER that no verifier would accept now
fails early and leaves the previously attached filter in place.  The
instructions are verified with the new bpf_prog_validate_classic(), which
runs the same checks as bpf_prog_create() on the copied program.  Before
this series the check only happened per queue in tun_attach_filter(),
which never ran when no queue was attached, so an invalid program could be
stored; with the corrected error check, every later attach of such a
program would then fail with -EINVAL.

The inverted check was discovered by manual code inspection first, and the
review of v1 reported the other things.

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      | 76 +++++++++++++++++++++++++++++++++++++++---
 include/linux/filter.h |  2 ++
 net/core/filter.c      | 62 ++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 242899f7fd071..e7d9f4480a89f 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,12 +723,61 @@ 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, independent of the ioctl caller's address space.
+ * 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;
+	int err;
+
+	if (copy_from_user(&fprog, argp, sizeof(fprog)))
+		return -EFAULT;
+
+	if (!fprog.len || fprog.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = kmalloc_array(fprog.len, sizeof(struct sock_filter),
+			      GFP_KERNEL_ACCOUNT);
+	if (!insns)
+		return -ENOMEM;
+
+	if (copy_from_user(insns, fprog.filter,
+			   fprog.len * sizeof(struct sock_filter))) {
+		kfree(insns);
+		return -EFAULT;
+	}
+
+	/* The instructions are only verified when a program is built from
+	 * them, which tun_attach_filter() does once per attached queue.
+	 * Check them here as well: with no queue attached that would not
+	 * happen at all, and, now that tun_attach() treats that error as
+	 * fatal, every later attach would fail with -EINVAL instead of
+	 * rejecting the program now.
+	 */
+	err = bpf_prog_validate_classic(insns, fprog.len);
+	if (err) {
+		kfree(insns);
+		return err;
+	}
+
+	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)
 {
 	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);
@@ -752,10 +802,11 @@ 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)
+		if (err)
 			goto out;
+		rollback_filter = true;
 	}
 
 	if (!tfile->detached &&
@@ -817,6 +868,17 @@ 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) {
+		/* The filter was installed before the rest of the attach
+		 * ran, so drop it again if a later step failed.  The
+		 * program it replaced is already gone: this leaves the
+		 * queue without a filter rather than restoring the
+		 * previous one.  Best effort, the filter may be locked.
+		 */
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2400,6 +2462,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)
@@ -3064,6 +3127,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;
 }
 
@@ -3075,7 +3141,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);
@@ -3423,8 +3489,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);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 39decde7fc730..e6f8506de3e2f 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1216,8 +1216,10 @@ int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog);
 int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 			      bpf_aux_classic_check_t trans, bool save_orig);
 void bpf_prog_destroy(struct bpf_prog *fp);
+int bpf_prog_validate_classic(const struct sock_filter *insns, unsigned int len);
 
 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 70dc621672f2e..bcc7906353fef 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1476,6 +1476,27 @@ void bpf_prog_destroy(struct bpf_prog *fp)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_destroy);
 
+/**
+ * bpf_prog_validate_classic - verify a classic BPF program in kernel memory
+ * @insns: instruction array
+ * @len: number of instructions in @insns
+ *
+ * Run the same checks that bpf_prog_create() performs on a classic BPF
+ * program, without allocating a struct bpf_prog.  Callers that already hold
+ * the instructions in kernel memory can use this to reject a program when it
+ * is configured instead of when it is attached to a socket.
+ *
+ * Return: 0 if the program is valid, otherwise -EINVAL.
+ */
+int bpf_prog_validate_classic(const struct sock_filter *insns, unsigned int len)
+{
+	if (!bpf_check_basics_ok(insns, len))
+		return -EINVAL;
+
+	return bpf_check_classic(insns, len);
+}
+EXPORT_SYMBOL_GPL(bpf_prog_validate_classic);
+
 static int __sk_attach_prog(struct bpf_prog *prog, struct sock *sk)
 {
 	struct sk_filter *fp, *old_fp;
@@ -1567,6 +1588,47 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sk_attach_filter);
 
+/**
+ * sk_attach_filter_kern - attach a classic BPF filter held in kernel memory
+ * @fprog: program to attach, with @fprog->filter pointing at kernel memory
+ * @sk: socket to attach the filter to
+ *
+ * Attach a copy of a classic BPF program that the kernel already holds, the
+ * way sk_attach_filter() does for one that still lives in user space. The
+ * caller must hold the socket lock; SOCK_FILTER_LOCKED is checked here.
+ *
+ * The program is built by bpf_prog_create(), which does not keep an original
+ * copy of the instructions, so sk_get_filter() returns -EACCES and sock_diag
+ * omits the filter for a socket that uses this helper.
+ *
+ * Return: 0 on success, or a negative errno: -EPERM if @sk has
+ * SOCK_FILTER_LOCKED set, otherwise whatever bpf_prog_create() or
+ * __sk_attach_prog() returned (for example -EINVAL for an invalid classic
+ * BPF program, -ENOMEM, or -ENOTSUPP when a JIT is required but is not
+ * available).
+ */
+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.43.0


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v7 2/2] 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, detaches the queue and attaches it
again. It then attaches a second queue from a fresh socket, which starts
without a filter of its own, so a clear IFF_NOFILTER shows that the filter
was installed again from the kernel copy.

attach_filter_bad_len_keeps_program() checks that a TUNATTACHFILTER with a
bad length leaves the installed program and the filter state that
TUNGETIFF reports alone, and that a queue attached afterwards still gets
the filter.  attach_filter_bad_prog_keeps_program() refuses a program with
a valid length and instructions that bpf_check_classic() rejects, and
checks that the program installed before is left in place.

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 | 223 ++++++++++++++++++++++++++++++
 1 file changed, 223 insertions(+)

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index afaa81c9bac53..1efff537dbae9 100644
--- a/tools/testing/selftests/net/tun.c
+++ b/tools/testing/selftests/net/tun.c
@@ -8,9 +8,11 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <linux/filter.h>
 #include <linux/if_tun.h>
 #include <netinet/tcp.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/socket.h>
 
 #include "kselftest_harness.h"
@@ -543,6 +545,227 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len.  Three instructions on purpose, so that the
+ * length read back through TUNGETFILTER tells it apart from a rejected
+ * program that is shorter.
+ */
+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),
+};
+
+/* 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 tun_get_iff_flags(int fd)
+{
+	struct ifreq ifr = { 0 };
+
+	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
+		return -1;
+
+	return ifr.ifr_flags;
+}
+
+/*
+ * 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, and a
+ * second queue is then attached from a fresh socket, which starts without a
+ * filter of its own.
+ */
+TEST_F(tun, reattach_filter_without_user_buffer)
+{
+	struct sock_fprog gf = { 0 };
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	void *prog;
+	int fd, 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);
+
+	/* This memory is no longer readable */
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
+
+	ret = tun_attach(self->fd, self->ifname);
+	EXPECT_EQ(ret, 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	/* The queue is attached again, not left disabled */
+	EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	/* The two checks below document the expected state, they cannot
+	 * tell a reinstalled filter from no filter: tun_detach() leaves
+	 * sk_filter alone, and TUNGETFILTER only returns the header.
+	 */
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	/* A new queue starts without a filter, so a clear IFF_NOFILTER shows
+	 * that the filter was installed again from the kernel copy.
+	 */
+	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;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	close(fd);
+
+	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_accept, ARRAY_SIZE(filter_accept)), 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);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+
+	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_accept, ARRAY_SIZE(filter_accept)), 0);
+	EXPECT_EQ(ioctl(self->fd, TUNDETACHFILTER, 0), 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_attach(self->fd, self->ifname), 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER);
+}
+
+/* A TUNATTACHFILTER with a bad length must leave the installed program and the
+ * filter state that TUNGETIFF reports alone.
+ */
+TEST_F(tun, attach_filter_bad_len_keeps_program)
+{
+	struct sock_fprog gf = { 0 };
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	int fd;
+
+	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));
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	/* A queue attached afterwards still gets the filter, so the rejected
+	 * ioctl left the saved program and filter_attached alone.
+	 */
+	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;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	flags = tun_get_iff_flags(fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	close(fd);
+}
+
+/* A TUNATTACHFILTER with instructions that bpf_check_classic() rejects must
+ * be refused, and must leave the program installed already in place.
+ */
+TEST_F(tun, attach_filter_bad_prog_keeps_program)
+{
+	struct sock_fprog gf = { 0 };
+	struct sock_filter bad[] = {
+		BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
+	};
+
+	ASSERT_EQ(filter_attach(self->fd, filter_accept,
+				ARRAY_SIZE(filter_accept)), 0);
+
+	errno = 0;
+	EXPECT_EQ(filter_attach(self->fd, bad, ARRAY_SIZE(bad)), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	/* the program attached before is untouched */
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+}
+
 FIXTURE(tun_vnet_gso)
 {
 	char ifname[IFNAMSIZ];
-- 
2.43.0


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v7 1/2] tun: keep a kernel copy of the socket filter program

[Willem de Bruijn]

Rongguang Wei wrote:
> 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.
> 
> Rebuilding the filter from that pointer is not reliable. With the inverted
> check below, a failure of the attach was not fatal: -EFAULT for an unmapped
> address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
> for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
> so the queue was attached without a filter. And when the read succeeded,
> the early return meant the queue was never published at all.
> 
> Keep the program in the kernel instead: tun->fprog_kern holds the
> instructions, and each queue gets its own program built from it with the
> new sk_attach_filter_kern(), the kernel memory counterpart of
> sk_attach_filter(). 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.
> 
> Also fix the inverted check, which made tun_attach() return early when
> sk_attach_filter_kern() succeeded instead of when it failed. Neither change
> works on its own: with only the kernel copy, every re-attach returns 0
> without publishing the queue; with only the check fixed, a re-attach that
> used to succeed without installing a filter would start to fail.
> 
> sk_attach_filter_kern() builds the program with bpf_prog_create(), which
> does not keep an original program, so SO_GET_FILTER returns -EACCES and
> sock_diag omits the filter for sockets that use it. tun sockets are not
> exposed as file descriptors, so this is not user visible.
> 
> The header and the instructions are checked in tun_copy_filter() before
> anything is replaced: a TUNATTACHFILTER that no verifier would accept now
> fails early and leaves the previously attached filter in place.  The
> instructions are verified with the new bpf_prog_validate_classic(), which
> runs the same checks as bpf_prog_create() on the copied program.  Before
> this series the check only happened per queue in tun_attach_filter(),
> which never ran when no queue was attached, so an invalid program could be
> stored; with the corrected error check, every later attach of such a
> program would then fail with -EINVAL.
> 
> The inverted check was discovered by manual code inspection first, and the
> review of v1 reported the other things.
> 
> 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>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net v7 2/2] 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, detaches the queue and attaches it
> again. It then attaches a second queue from a fresh socket, which starts
> without a filter of its own, so a clear IFF_NOFILTER shows that the filter
> was installed again from the kernel copy.
> 
> attach_filter_bad_len_keeps_program() checks that a TUNATTACHFILTER with a
> bad length leaves the installed program and the filter state that
> TUNGETIFF reports alone, and that a queue attached afterwards still gets
> the filter.  attach_filter_bad_prog_keeps_program() refuses a program with
> a valid length and instructions that bpf_check_classic() rejects, and
> checks that the program installed before is left in place.
> 
> 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>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net v7 1/2] tun: keep a kernel copy of the socket filter program

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (2) · 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).

- [High] bpf_prog_validate_classic() does not run "the same checks that
  bpf_prog_create() performs", although its kernel-doc and the commit…
- [Low] The kernel-doc for the new exported helper
  bpf_prog_validate_classic() (net/core/filter.c) says it returns "0 if…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a filter
  replacement that passes tun_copy_filter() and then fails on some queue…
- [Medium] This problem wasn't introduced by this patch, but
  TUNDETACHFILTER leaves disabled queues with their old socket filter,…

--

tun: keep a kernel copy of the socket filter program

This commit keeps a kernel copy of the classic BPF program passed to
TUNATTACHFILTER in tun->fprog_kern. Each queue's filter is now built from
that copy with a new sk_attach_filter_kern() helper. It also fixes the
inverted error check in tun_attach(), and it validates the program up
front in tun_copy_filter() with a new bpf_prog_validate_classic() helper.

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

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd071..e7d9f4480a89f 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> +	/* The instructions are only verified when a program is built from
> +	 * them, which tun_attach_filter() does once per attached queue.
> +	 * Check them here as well: with no queue attached that would not
> +	 * happen at all, and, now that tun_attach() treats that error as
> +	 * fatal, every later attach would fail with -EINVAL instead of
> +	 * rejecting the program now.
> +	 */
> +	err = bpf_prog_validate_classic(insns, fprog.len);
> +	if (err) {
> +		kfree(insns);
> +		return err;
> +	}

[Severity: High]
Does bpf_prog_validate_classic() really reject everything that
bpf_prog_create() rejects? Its kernel-doc and the commit message both say
it does:

  "The instructions are verified with the new bpf_prog_validate_classic(),
  which runs the same checks as bpf_prog_create() on the copied program."

The helper only runs bpf_check_basics_ok() and bpf_check_classic().
bpf_prog_create() also goes through this path:

bpf_prog_create()
  bpf_prepare_filter()
    bpf_migrate_filter()
      bpf_convert_filter()
    bpf_prog_select_runtime()
      __bpf_prog_select_runtime()

bpf_convert_filter() rejects any jump whose converted offset does not fit
in s16:

	if (off < off_min || off > off_max)
		goto err;

__bpf_prog_select_runtime() can also return -ENOTSUPP when a JIT is
required but did not run.

Take a 4096 instruction program: BPF_JMP|BPF_JA with k=4094 at pc 0, then
4094 BPF_LD|BPF_B|BPF_ABS loads of offset 0, then BPF_RET|BPF_K.
bpf_check_classic() accepts it. convert_bpf_ld_abs() expands each load
into 12 eBPF instructions, so the JA offset grows to about 49128, and
bpf_convert_filter() fails with -EINVAL.

Suppose every queue on a multiqueue TAP has been disabled with
IFF_DETACH_QUEUE. tun_copy_filter() stores this program, and
tun_attach_filter() loops over zero queues and then does:

	tun->filter_attached = true;

>From then on, every IFF_ATTACH_QUEUE, and every TUNSETIFF without
IFF_NOFILTER, calls sk_attach_filter_kern() in tun_attach() and gets
-EINVAL. With the corrected check, tun_attach() then takes the goto out
path and the queue is never published.

Isn't this the "every later attach would fail with -EINVAL" case that the
comment above says is avoided?

When queues are active, the same program fails at queue 0, and
tun_attach_filter() calls tun_detach_filter(tun, 0). That frees fprog_kern
and clears filter_attached, which doesn't seem to match this part of the
commit message:

  "a TUNATTACHFILTER that no verifier would accept now fails early and
  leaves the previously attached filter in place."

The sk_attach_filter_kern() kernel-doc in this same patch lists -EINVAL
from bpf_prog_create() and -ENOTSUPP as possible results. That also
suggests the two checks are not equivalent.

Would it be more reliable for tun_copy_filter() to build the program once
with bpf_prog_create() and then destroy it, instead of calling
bpf_prog_validate_classic()?

[ ... ]

> @@ -3064,6 +3127,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 old filter in place on disabled queues?

tun_detach_filter() only walks the active queues:

	for (i = 0; i < n; i++) {
		tfile = rtnl_dereference(tun->tfiles[i]);

TUNDETACHFILTER passes tun->numqueues as n. A queue disabled with
IFF_DETACH_QUEUE was already removed from tun->tfiles by
__tun_detach(tfile, false) via tun_disable_queue(). That path never calls
sk_detach_filter(), so the disabled socket keeps its sk_filter.

When the queue is re-enabled with IFF_ATTACH_QUEUE, tun_attach() skips its
filter block because filter_attached is now false. Nothing clears the
stale filter, and tun_net_xmit() checks the socket filter directly:

	if (tfile->socket.sk->sk_filter) {
		drop_reason = sk_filter_reason(tfile->socket.sk, skb);

So could packets on that queue still be filtered after a successful
TUNDETACHFILTER? The baseline code has the same omission.

> @@ -3075,7 +3141,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 what happens to the device-wide filter
state when a replacement filter passes tun_copy_filter() and then fails on
one of the queues here?

sk_attach_filter_kern() can still fail for a single queue. Examples are
-ENOMEM from bpf_prog_create(), the optmem charge in __sk_attach_prog()
(the new filter is charged before the old one is uncharged), or the s16
conversion failure described above.

If the failure is at index i, tun_detach_filter(tun, i) removes the filter
only from queues 0..i-1. It then frees fprog_kern and sets filter_attached
to false. Queues i..numqueues-1, and any disabled queues, keep their old
socket filter.

tun->fprog already holds the new header, so TUNGETFILTER reports a program
that is not attached. Because filter_attached is false, later tun_attach()
calls skip sk_attach_filter_kern(), and those queues come up without a
filter.

Doesn't this leave the device with some queues using the old filter and
some with no filter at all? The baseline tun_attach_filter() and
tun_detach_filter() pair is structured the same way; this patch only adds
the kfree() of fprog_kern.

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..bcc7906353fef 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1476,6 +1476,27 @@ void bpf_prog_destroy(struct bpf_prog *fp)
>  }
>  EXPORT_SYMBOL_GPL(bpf_prog_destroy);
>  
> +/**
> + * bpf_prog_validate_classic - verify a classic BPF program in kernel memory
> + * @insns: instruction array
> + * @len: number of instructions in @insns
> + *
> + * Run the same checks that bpf_prog_create() performs on a classic BPF
> + * program, without allocating a struct bpf_prog.  Callers that already hold
> + * the instructions in kernel memory can use this to reject a program when it
> + * is configured instead of when it is attached to a socket.
> + *
> + * Return: 0 if the program is valid, otherwise -EINVAL.
> + */
> +int bpf_prog_validate_classic(const struct sock_filter *insns, unsigned int len)
> +{
> +	if (!bpf_check_basics_ok(insns, len))
> +		return -EINVAL;
> +
> +	return bpf_check_classic(insns, len);
> +}

[Severity: Low]
Is the Return: description accurate? bpf_prog_validate_classic() passes
the result of bpf_check_classic() through unchanged. For a program that
ends in BPF_RET, that result comes from check_load_and_stores(), which
allocates:

	masks = kmalloc_array(flen, sizeof(*masks), GFP_KERNEL);
	if (!masks)
		return -ENOMEM;

So the helper can also return -ENOMEM. Along the same lines, "without
allocating a struct bpf_prog" is literally true, but the helper does still
allocate memory.

tun_copy_filter() passes the error through correctly, so this only affects
the documented contract of the exported helper.

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

Re: [PATCH net v7 2/2] selftests: net: add TAP socket filter attach tests

[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] attach_filter_bad_prog_keeps_program() in
  tools/testing/selftests/net/tun.c says a refused program "must leave…

--

selftests: net: add TAP socket filter attach tests

This commit adds five TAP socket filter tests to
tools/testing/selftests/net/tun.c. They cover reinstalling the filter
from the kernel copy after the user buffer is made unreadable, attaching
queues with IFF_NOFILTER or after TUNDETACHFILTER, and TUNATTACHFILTER
calls rejected for a bad length or for bad instructions.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index afaa81c9bac53..1efff537dbae9 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> +/* A TUNATTACHFILTER with instructions that bpf_check_classic() rejects must
> + * be refused, and must leave the program installed already in place.
> + */
> +TEST_F(tun, attach_filter_bad_prog_keeps_program)
> +{
> +	struct sock_fprog gf = { 0 };
> +	struct sock_filter bad[] = {
> +		BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
> +	};
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept,
> +				ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, bad, ARRAY_SIZE(bad)), -1);
> +	EXPECT_EQ(errno, EINVAL);
> +
> +	/* the program attached before is untouched */
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Medium]
Can this check tell whether the earlier program is still installed?

TUNGETFILTER in __tun_chr_ioctl() only copies out the saved header:

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

tun_detach_filter() is also the failure path of tun_attach_filter(). It
drops every queue's sk_filter, frees fprog_kern and clears
filter_attached, but it never touches tun->fprog:

drivers/net/tun.c:tun_detach_filter() {
	for (i = 0; i < n; i++) {
		...
		sk_detach_filter(tfile->socket.sk);
		...
	}

	kfree(tun->fprog_kern.filter);
	tun->fprog_kern.filter = NULL;
	tun->fprog_kern.len = 0;
	tun->filter_attached = false;
}

Suppose a later change made the bad program path go through
tun_detach_filter(), for example by validating after the attach and
rolling back with tun_detach_filter(tun, tun->numqueues). gf.len would
still be 3 and this test would pass. Meanwhile the queues would have lost
their filter, and queues attached later would come up without one.

The current kernel passes for the right reason. tun_copy_filter() calls
bpf_prog_validate_classic() before it changes any tun state. The test
just can't catch a regression in the behaviour it says it covers.

attach_filter_bad_len_keeps_program() handles this by checking
IFF_NOFILTER through TUNGETIFF on self->fd. It also attaches a fresh
TUNSETIFF queue and checks that the new queue gets the filter. Could the
same checks be added here, so that the test verifies what the commit
message describes?

  "checks that the program installed before is left in place"

The comment in reattach_filter_without_user_buffer() makes the same point
about this ioctl: "TUNGETFILTER only returns the header".

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

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

Re: [PATCH net v7 1/2] tun: keep a kernel copy of the socket filter program

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (2) · 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).
> 
> - [High] bpf_prog_validate_classic() does not run "the same checks that
>   bpf_prog_create() performs", although its kernel-doc and the commit…
> - [Low] The kernel-doc for the new exported helper
>   bpf_prog_validate_classic() (net/core/filter.c) says it returns "0 if…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but a filter
>   replacement that passes tun_copy_filter() and then fails on some queue…
> - [Medium] This problem wasn't introduced by this patch, but
>   TUNDETACHFILTER leaves disabled queues with their old socket filter,…
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This commit keeps a kernel copy of the classic BPF program passed to
> TUNATTACHFILTER in tun->fprog_kern. Each queue's filter is now built from
> that copy with a new sk_attach_filter_kern() helper. It also fixes the
> inverted error check in tun_attach(), and it validates the program up
> front in tun_copy_filter() with a new bpf_prog_validate_classic() helper.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 242899f7fd071..e7d9f4480a89f 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
> > +	/* The instructions are only verified when a program is built from
> > +	 * them, which tun_attach_filter() does once per attached queue.
> > +	 * Check them here as well: with no queue attached that would not
> > +	 * happen at all, and, now that tun_attach() treats that error as
> > +	 * fatal, every later attach would fail with -EINVAL instead of
> > +	 * rejecting the program now.
> > +	 */
> > +	err = bpf_prog_validate_classic(insns, fprog.len);
> > +	if (err) {
> > +		kfree(insns);
> > +		return err;
> > +	}
> 
> [Severity: High]
> Does bpf_prog_validate_classic() really reject everything that
> bpf_prog_create() rejects? Its kernel-doc and the commit message both say
> it does:
> 
>   "The instructions are verified with the new bpf_prog_validate_classic(),
>   which runs the same checks as bpf_prog_create() on the copied program."
> 
> The helper only runs bpf_check_basics_ok() and bpf_check_classic().
> bpf_prog_create() also goes through this path:
> 
> bpf_prog_create()
>   bpf_prepare_filter()
>     bpf_migrate_filter()
>       bpf_convert_filter()
>     bpf_prog_select_runtime()
>       __bpf_prog_select_runtime()
> 
> bpf_convert_filter() rejects any jump whose converted offset does not fit
> in s16:
> 
> 	if (off < off_min || off > off_max)
> 		goto err;
> 
> __bpf_prog_select_runtime() can also return -ENOTSUPP when a JIT is
> required but did not run.
> 
> Take a 4096 instruction program: BPF_JMP|BPF_JA with k=4094 at pc 0, then
> 4094 BPF_LD|BPF_B|BPF_ABS loads of offset 0, then BPF_RET|BPF_K.
> bpf_check_classic() accepts it. convert_bpf_ld_abs() expands each load
> into 12 eBPF instructions, so the JA offset grows to about 49128, and
> bpf_convert_filter() fails with -EINVAL.
> 
> Suppose every queue on a multiqueue TAP has been disabled with
> IFF_DETACH_QUEUE. tun_copy_filter() stores this program, and
> tun_attach_filter() loops over zero queues and then does:
> 
> 	tun->filter_attached = true;
> 
> >From then on, every IFF_ATTACH_QUEUE, and every TUNSETIFF without
> IFF_NOFILTER, calls sk_attach_filter_kern() in tun_attach() and gets
> -EINVAL. With the corrected check, tun_attach() then takes the goto out
> path and the queue is never published.
> 
> Isn't this the "every later attach would fail with -EINVAL" case that the
> comment above says is avoided?
> 
> When queues are active, the same program fails at queue 0, and
> tun_attach_filter() calls tun_detach_filter(tun, 0). That frees fprog_kern
> and clears filter_attached, which doesn't seem to match this part of the
> commit message:
> 
>   "a TUNATTACHFILTER that no verifier would accept now fails early and
>   leaves the previously attached filter in place."
> 
> The sk_attach_filter_kern() kernel-doc in this same patch lists -EINVAL
> from bpf_prog_create() and -ENOTSUPP as possible results. That also
> suggests the two checks are not equivalent.
> 
> Would it be more reliable for tun_copy_filter() to build the program once
> with bpf_prog_create() and then destroy it, instead of calling
> bpf_prog_validate_classic()?

This sounds to me as the perfect being the enemy of the good.

Even the addition of calling bpf_prog_validate_classic is a nice to have.
This series fixes a real issue.

Yes, some issues are still only discovered later when the BPF program is
built. But as long that late discovery is also handled well, that is
acceptable.