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

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v10 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,
and the copy is committed only after every queue accepted it, so a program
that is refused later, when it is built, does not replace the saved one.
The header is zeroed before it is filled in, so TUNGETFILTER never copies
uninitialized padding to user space.

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

---
v10:
  - selftest: fix the comment message. no code change is involved
  - add to to CC bpf@
v9:
  - zero the whole sock_fprog header in tun_copy_filter(), so that the
    padding TUNGETFILTER copies is not uninitialized stack
  - https://lore.kernel.org/netdev/20261030060043.94719-1-clementwei90@163.com/
v8:
  - narrow the bpf_prog_validate_classic() contract to the classic
    verifier and note that it can also return -ENOMEM; a program can still
    be refused when it is built, and the commit message now says so
  - commit the new program only after every queue took it, so a late
    failure does not replace the saved one, and filter_attached is left
    alone
  - selftests: check the filter state and a newly attached queue in
    attach_filter_bad_prog_keeps_program() too
  - https://lore.kernel.org/netdev/20261028093235.148649-1-clementwei90@163.com/
v7:
  - verify the instructions in tun_copy_filter() before anything is
    replaced, 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
  - reject a program whose length is out of range before allocating
    memory for it
  - 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
  - https://lore.kernel.org/netdev/20261023092239.186665-1-clementwei90@163.com/
v6:
  - add description for return value of sk_attach_filter_kern(); no code
    change is involved
  - https://lore.kernel.org/netdev/20261016070654.170932-1-clementwei90@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                 | 127 +++++++++++++--
 include/linux/filter.h            |   2 +
 net/core/filter.c                 |  67 ++++++++
 tools/testing/selftests/net/tun.c | 249 ++++++++++++++++++++++++++++++
 4 files changed, 433 insertions(+), 12 deletions(-)

-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v10 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 the classic verifier rejects
now fails early and leaves the previously attached filter in place.  The
instructions are verified with the new bpf_prog_validate_classic(), which
runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
That does not cover the s16 check in bpf_convert_filter() or the runtime
selection in bpf_prog_select_runtime(), so a program can still be refused
when it is built for a queue. Such a late failure does not replace the
saved program: the copy is committed to tun->fprog and tun->fprog_kern
only after tun_attach_filter() installed it on every queue. A queue that
already took the copy is rolled back, which leaves it without a filter,
because the program it ran before was already released; restoring it
would need an extra reference per queue.

The inverted check was discovered by manual code inspection first.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/tun.c      | 127 +++++++++++++++++++++++++++++++++++++----
 include/linux/filter.h |   2 +
 net/core/filter.c      |  67 ++++++++++++++++++++++
 3 files changed, 184 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index dcec7311e601..67c8b74d59bd 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;
@@ -730,12 +731,69 @@ 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.
+ * @fprog_kern receives the copy, which belongs to the caller until it is
+ * installed or freed, and @fprog receives the header, with the user pointer
+ * kept for TUNGETFILTER.
+ */
+static int tun_copy_filter(struct sock_fprog __user *argp,
+			   struct sock_fprog *fprog,
+			   struct sock_fprog_kern *fprog_kern)
+{
+	struct sock_fprog fp;
+	struct sock_filter *insns;
+	int err;
+
+	if (copy_from_user(&fp, argp, sizeof(fp)))
+		return -EFAULT;
+
+	if (!fp.len || fp.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
+			      GFP_KERNEL_ACCOUNT);
+	if (!insns)
+		return -ENOMEM;
+
+	if (copy_from_user(insns, fp.filter,
+			   fp.len * sizeof(struct sock_filter))) {
+		kfree(insns);
+		return -EFAULT;
+	}
+
+	/* The instructions are only verified when a program is built from
+	 * them, which happens when the filter is attached to a socket. Run
+	 * the classic verifier here as well, so that a bad program is
+	 * refused before it is saved and before any queue is touched.
+	 * bpf_prog_create() can still refuse the program later, when it is
+	 * converted to eBPF or when no runtime can be selected for it; the
+	 * attach path returns that error, and the saved program is not
+	 * replaced.
+	 */
+	err = bpf_prog_validate_classic(insns, fp.len);
+	if (err) {
+		kfree(insns);
+		return err;
+	}
+
+	/* Zero the whole header before filling it in. */
+	memset(fprog, 0, sizeof(*fprog));
+	fprog->len = fp.len;
+	fprog->filter = fp.filter;
+	fprog_kern->len = fp.len;
+	fprog_kern->filter = insns;
+
+	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);
@@ -760,10 +818,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 &&
@@ -825,6 +884,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. Any
+		 * program it replaced is already gone: this leaves the
+		 * queue without a filter rather than restoring the
+		 * previous one.
+		 */
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2410,6 +2480,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)
@@ -3080,10 +3151,14 @@ 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;
 }
 
-static int tun_attach_filter(struct tun_struct *tun)
+static int tun_attach_filter(struct tun_struct *tun,
+			     struct sock_fprog_kern *fprog_kern)
 {
 	int i, ret = 0;
 	struct tun_file *tfile;
@@ -3091,15 +3166,29 @@ 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(fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (ret) {
-			tun_detach_filter(tun, i);
-			return ret;
-		}
+		if (ret)
+			goto out_detach;
 	}
 
 	tun->filter_attached = true;
+	return 0;
+
+out_detach:
+	/* Drop the copies already installed on queues 0..i-1, which leaves
+	 * them without a filter: their previous sk_filter, if any, was
+	 * already replaced and released. The caller does not replace the
+	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
+	 * filter_attached keep their previous values.
+	 */
+	while (i--) {
+		tfile = rtnl_dereference(tun->tfiles[i]);
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
+
 	return ret;
 }
 
@@ -3434,17 +3523,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		tun_set_sndbuf(tun);
 		break;
 
-	case TUNATTACHFILTER:
+	case TUNATTACHFILTER: {
+		struct sock_fprog_kern fprog_kern;
+		struct sock_fprog fprog;
+
 		/* Can be set only for TAPs */
 		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(argp, &fprog, &fprog_kern);
+		if (ret)
+			break;
+
+		ret = tun_attach_filter(tun, &fprog_kern);
+		if (ret) {
+			kfree(fprog_kern.filter);
 			break;
+		}
 
-		ret = tun_attach_filter(tun);
+		/* Install the copy only after every queue accepted it. */
+		kfree(tun->fprog_kern.filter);
+		tun->fprog_kern = fprog_kern;
+		tun->fprog = fprog;
 		break;
+	}
 
 	case TUNDETACHFILTER:
 		/* Can be set only for TAPs */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 4f0cd67caccb..13f1e8864807 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1263,8 +1263,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 3613a7d605d7..a0b9448738a9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1476,6 +1476,32 @@ void bpf_prog_destroy(struct bpf_prog *fp)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_destroy);
 
+/**
+ * bpf_prog_validate_classic - run the classic BPF verifier on kernel memory
+ * @insns: instruction array
+ * @len: number of instructions in @insns
+ *
+ * Run bpf_check_basics_ok() and bpf_check_classic() on a program that is
+ * already in kernel memory, without building a struct bpf_prog.  These are
+ * the checks bpf_prog_create() runs before it converts the program, so
+ * callers can reject a bad program when it is configured instead of when it
+ * is first attached to a socket.
+ *
+ * A program that passes here can still be refused by bpf_prog_create(), when
+ * it is converted to eBPF or when no runtime can be selected for it.
+ *
+ * Return: 0 if the program passes the classic verifier, -EINVAL if it does
+ * not, or -ENOMEM if the verifier cannot allocate its state.
+ */
+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 +1593,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.25.1

[PATCH net v10 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 the classic verifier rejects
now fails early and leaves the previously attached filter in place.  The
instructions are verified with the new bpf_prog_validate_classic(), which
runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
That does not cover the s16 check in bpf_convert_filter() or the runtime
selection in bpf_prog_select_runtime(), so a program can still be refused
when it is built for a queue. Such a late failure does not replace the
saved program: the copy is committed to tun->fprog and tun->fprog_kern
only after tun_attach_filter() installed it on every queue. A queue that
already took the copy is rolled back, which leaves it without a filter,
because the program it ran before was already released; restoring it
would need an extra reference per queue.

The inverted check was discovered by manual code inspection first.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
Reviewed-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/tun.c      | 127 +++++++++++++++++++++++++++++++++++++----
 include/linux/filter.h |   2 +
 net/core/filter.c      |  67 ++++++++++++++++++++++
 3 files changed, 184 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index dcec7311e601..67c8b74d59bd 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;
@@ -730,12 +731,69 @@ 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.
+ * @fprog_kern receives the copy, which belongs to the caller until it is
+ * installed or freed, and @fprog receives the header, with the user pointer
+ * kept for TUNGETFILTER.
+ */
+static int tun_copy_filter(struct sock_fprog __user *argp,
+			   struct sock_fprog *fprog,
+			   struct sock_fprog_kern *fprog_kern)
+{
+	struct sock_fprog fp;
+	struct sock_filter *insns;
+	int err;
+
+	if (copy_from_user(&fp, argp, sizeof(fp)))
+		return -EFAULT;
+
+	if (!fp.len || fp.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
+			      GFP_KERNEL_ACCOUNT);
+	if (!insns)
+		return -ENOMEM;
+
+	if (copy_from_user(insns, fp.filter,
+			   fp.len * sizeof(struct sock_filter))) {
+		kfree(insns);
+		return -EFAULT;
+	}
+
+	/* The instructions are only verified when a program is built from
+	 * them, which happens when the filter is attached to a socket. Run
+	 * the classic verifier here as well, so that a bad program is
+	 * refused before it is saved and before any queue is touched.
+	 * bpf_prog_create() can still refuse the program later, when it is
+	 * converted to eBPF or when no runtime can be selected for it; the
+	 * attach path returns that error, and the saved program is not
+	 * replaced.
+	 */
+	err = bpf_prog_validate_classic(insns, fp.len);
+	if (err) {
+		kfree(insns);
+		return err;
+	}
+
+	/* Zero the whole header before filling it in. */
+	memset(fprog, 0, sizeof(*fprog));
+	fprog->len = fp.len;
+	fprog->filter = fp.filter;
+	fprog_kern->len = fp.len;
+	fprog_kern->filter = insns;
+
+	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);
@@ -760,10 +818,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 &&
@@ -825,6 +884,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. Any
+		 * program it replaced is already gone: this leaves the
+		 * queue without a filter rather than restoring the
+		 * previous one.
+		 */
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2410,6 +2480,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)
@@ -3080,10 +3151,14 @@ 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;
 }
 
-static int tun_attach_filter(struct tun_struct *tun)
+static int tun_attach_filter(struct tun_struct *tun,
+			     struct sock_fprog_kern *fprog_kern)
 {
 	int i, ret = 0;
 	struct tun_file *tfile;
@@ -3091,15 +3166,29 @@ 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(fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (ret) {
-			tun_detach_filter(tun, i);
-			return ret;
-		}
+		if (ret)
+			goto out_detach;
 	}
 
 	tun->filter_attached = true;
+	return 0;
+
+out_detach:
+	/* Drop the copies already installed on queues 0..i-1, which leaves
+	 * them without a filter: their previous sk_filter, if any, was
+	 * already replaced and released. The caller does not replace the
+	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
+	 * filter_attached keep their previous values.
+	 */
+	while (i--) {
+		tfile = rtnl_dereference(tun->tfiles[i]);
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
+
 	return ret;
 }
 
@@ -3434,17 +3523,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		tun_set_sndbuf(tun);
 		break;
 
-	case TUNATTACHFILTER:
+	case TUNATTACHFILTER: {
+		struct sock_fprog_kern fprog_kern;
+		struct sock_fprog fprog;
+
 		/* Can be set only for TAPs */
 		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(argp, &fprog, &fprog_kern);
+		if (ret)
+			break;
+
+		ret = tun_attach_filter(tun, &fprog_kern);
+		if (ret) {
+			kfree(fprog_kern.filter);
 			break;
+		}
 
-		ret = tun_attach_filter(tun);
+		/* Install the copy only after every queue accepted it. */
+		kfree(tun->fprog_kern.filter);
+		tun->fprog_kern = fprog_kern;
+		tun->fprog = fprog;
 		break;
+	}
 
 	case TUNDETACHFILTER:
 		/* Can be set only for TAPs */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 4f0cd67caccb..13f1e8864807 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1263,8 +1263,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 3613a7d605d7..a0b9448738a9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1476,6 +1476,32 @@ void bpf_prog_destroy(struct bpf_prog *fp)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_destroy);
 
+/**
+ * bpf_prog_validate_classic - run the classic BPF verifier on kernel memory
+ * @insns: instruction array
+ * @len: number of instructions in @insns
+ *
+ * Run bpf_check_basics_ok() and bpf_check_classic() on a program that is
+ * already in kernel memory, without building a struct bpf_prog.  These are
+ * the checks bpf_prog_create() runs before it converts the program, so
+ * callers can reject a bad program when it is configured instead of when it
+ * is first attached to a socket.
+ *
+ * A program that passes here can still be refused by bpf_prog_create(), when
+ * it is converted to eBPF or when no runtime can be selected for it.
+ *
+ * Return: 0 if the program passes the classic verifier, -EINVAL if it does
+ * not, or -ENOMEM if the verifier cannot allocate its state.
+ */
+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 +1593,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.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v10 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: 0 · Medium: 2 · 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] TUNATTACHFILTER on a device with zero enabled queues saves a
  program that has never been built.
- [Medium] After a partial TUNATTACHFILTER failure, the device's recorded
  filter state no longer matches what the queues enforce, and the reason…

--

tun: keep a kernel copy of the socket filter program

This patch makes TUNATTACHFILTER keep a kernel copy of the classic BPF
instructions in tun->fprog_kern. Each queue gets its own program, built
from that copy with the new sk_attach_filter_kern() helper. The patch
also fixes the inverted error check in tun_attach(), and checks the
program with the new bpf_prog_validate_classic() before the saved filter
is replaced.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index dcec7311e6011..67c8b74d59bde 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -3091,15 +3166,29 @@ 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(fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (ret) {
> -			tun_detach_filter(tun, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out_detach;
>  	}
>  
>  	tun->filter_attached = true;
> +	return 0;

[Severity: Medium]
What happens here when tun->numqueues is 0?

The loop body never runs, so bpf_prog_create() is never called on the new
program. Even so, filter_attached is set, and the TUNATTACHFILTER case in
__tun_chr_ioctl() commits the program:

	kfree(tun->fprog_kern.filter);
	tun->fprog_kern = fprog_kern;
	tun->fprog = fprog;

tun_copy_filter() only runs bpf_prog_validate_classic(). A program can pass
that check and still be refused later by bpf_convert_filter(), when a
converted jump offset falls outside the s16 range. It can also be refused
by bpf_prog_select_runtime(), which returns -ENOTSUPP when a JIT is
required but cannot be used.

A usable fd with numqueues == 0 looks reachable on a multiqueue TAP.
tun_set_queue() lets IFF_DETACH_QUEUE detach every queue, and
__tun_detach(tfile, false) keeps tfile->tun. A detached fd can therefore
still issue TUNATTACHFILTER.

The inverted check in tun_attach() is now fixed:

	err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
	release_sock(tfile->socket.sk);
	if (err)
		goto out;

With that fix, wouldn't every later IFF_ATTACH_QUEUE on this persistent
device fail? The same goes for every TUNSETIFF without IFF_NOFILTER.
Before the patch, a new tfile in this situation still attached.

The commit message says "Such a late failure does not replace the saved
program". Here, though, the program is saved without ever being built, so
the late failure happens on every later queue attach instead.

Could the program be built once with bpf_prog_create() even when there
are no queues to attach it to?

> +
> +out_detach:
> +	/* Drop the copies already installed on queues 0..i-1, which leaves
> +	 * them without a filter: their previous sk_filter, if any, was
> +	 * already replaced and released. The caller does not replace the
> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
> +	 * filter_attached keep their previous values.
> +	 */
> +	while (i--) {
> +		tfile = rtnl_dereference(tun->tfiles[i]);
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}

[Severity: Medium]
Does this rollback leave the device's filter state out of sync with what
the queues actually enforce?

Suppose filter P is attached on every queue, and a new TUNATTACHFILTER
fails on queue i > 0. On queues 0..i-1, __sk_attach_prog() has already
replaced and released P. This loop then detaches those queues, so they
run no filter at all. Queues i..n-1 still run P.

tun->fprog, tun->fprog_kern and filter_attached keep their old values, so
TUNGETFILTER still reports P. tun_attach() also installs P on queues
attached later. Meanwhile, queues 0..i-1 pass every packet through
sk_filter() unfiltered.

Before the patch, tun_detach_filter(tun, i) cleared filter_attached on
this path. The device therefore did not claim a filter that some of its
queues lacked.

The commit message says:

    A queue that
    already took the copy is rolled back, which leaves it without a filter,
    because the program it ran before was already released; restoring it
    would need an extra reference per queue.

Is that accurate? P's instructions are still in tun->fprog_kern. It looks
like sk_attach_filter_kern(&tun->fprog_kern, sk) could rebuild P on queues
0..i-1, the same way tun_attach() does for new queues. Another option
would be to clear the saved state when the attach fails partway.

A failure partway through the loop looks reachable in several ways:

  - -ENOMEM from bpf_prog_alloc(), or from kmalloc_obj() in
    __sk_attach_prog()
  - the per-socket optmem check in __sk_filter_charge()
  - under CONFIG_BPF_JIT_ALWAYS_ON, bpf_jit_limit being reached partway
    through, because each queue JITs its own copy

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

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

[weirongguang]

netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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] TUNATTACHFILTER on a device with zero enabled queues saves a
>   program that has never been built.
> - [Medium] After a partial TUNATTACHFILTER failure, the device's recorded
>   filter state no longer matches what the queues enforce, and the reason…
> 
> --
> 
Thanks for the review.
> tun: keep a kernel copy of the socket filter program
> 
> This patch makes TUNATTACHFILTER keep a kernel copy of the classic BPF
> instructions in tun->fprog_kern. Each queue gets its own program, built
> from that copy with the new sk_attach_filter_kern() helper. The patch
> also fixes the inverted error check in tun_attach(), and checks the
> program with the new bpf_prog_validate_classic() before the saved filter
> is replaced.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index dcec7311e6011..67c8b74d59bde 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -3091,15 +3166,29 @@ 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(fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>> -		if (ret) {
>> -			tun_detach_filter(tun, i);
>> -			return ret;
>> -		}
>> +		if (ret)
>> +			goto out_detach;
>>  	}
>>  
>>  	tun->filter_attached = true;
>> +	return 0;
> 
> [Severity: Medium]
> What happens here when tun->numqueues is 0?
> 
> The loop body never runs, so bpf_prog_create() is never called on the new
> program. Even so, filter_attached is set, and the TUNATTACHFILTER case in
> __tun_chr_ioctl() commits the program:
> 
> 	kfree(tun->fprog_kern.filter);
> 	tun->fprog_kern = fprog_kern;
> 	tun->fprog = fprog;
> 
> tun_copy_filter() only runs bpf_prog_validate_classic(). A program can pass
> that check and still be refused later by bpf_convert_filter(), when a
> converted jump offset falls outside the s16 range. It can also be refused
> by bpf_prog_select_runtime(), which returns -ENOTSUPP when a JIT is
> required but cannot be used.
> 
> A usable fd with numqueues == 0 looks reachable on a multiqueue TAP.
> tun_set_queue() lets IFF_DETACH_QUEUE detach every queue, and
> __tun_detach(tfile, false) keeps tfile->tun. A detached fd can therefore
> still issue TUNATTACHFILTER.
> 
> The inverted check in tun_attach() is now fixed:
> 
> 	err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
> 	release_sock(tfile->socket.sk);
> 	if (err)
> 		goto out;
> 
> With that fix, wouldn't every later IFF_ATTACH_QUEUE on this persistent
> device fail? The same goes for every TUNSETIFF without IFF_NOFILTER.
> Before the patch, a new tfile in this situation still attached.
> 
> The commit message says "Such a late failure does not replace the saved
> program". Here, though, the program is saved without ever being built, so
> the late failure happens on every later queue attach instead.
> 
> Could the program be built once with bpf_prog_create() even when there
> are no queues to attach it to?
> 
The program is stored only after it passed the classic verifier, and a
program that is refused later, when it is built, only makes the attach fail:
nothing is left half-installed, and TUNATTACHFILTER can be reissued with a
valid program. Building it once in tun_attach_filter() when there are no
queues would just move the same failure earlier in the ioctl; it would not
make the program installable. The commit message sentence is vacuous in that
case, not wrong: the intent is that a failed build does not replace the saved
program, not that the stored copy has necessarily been built.
>> +
>> +out_detach:
>> +	/* Drop the copies already installed on queues 0..i-1, which leaves
>> +	 * them without a filter: their previous sk_filter, if any, was
>> +	 * already replaced and released. The caller does not replace the
>> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
>> +	 * filter_attached keep their previous values.
>> +	 */
>> +	while (i--) {
>> +		tfile = rtnl_dereference(tun->tfiles[i]);
>> +		lock_sock(tfile->socket.sk);
>> +		sk_detach_filter(tfile->socket.sk);
>> +		release_sock(tfile->socket.sk);
>> +	}
> 
> [Severity: Medium]
> Does this rollback leave the device's filter state out of sync with what
> the queues actually enforce?
> 
> Suppose filter P is attached on every queue, and a new TUNATTACHFILTER
> fails on queue i > 0. On queues 0..i-1, __sk_attach_prog() has already
> replaced and released P. This loop then detaches those queues, so they
> run no filter at all. Queues i..n-1 still run P.
> 
> tun->fprog, tun->fprog_kern and filter_attached keep their old values, so
> TUNGETFILTER still reports P. tun_attach() also installs P on queues
> attached later. Meanwhile, queues 0..i-1 pass every packet through
> sk_filter() unfiltered.
> 
> Before the patch, tun_detach_filter(tun, i) cleared filter_attached on
> this path. The device therefore did not claim a filter that some of its
> queues lacked.
> 
> The commit message says:
> 
>     A queue that
>     already took the copy is rolled back, which leaves it without a filter,
>     because the program it ran before was already released; restoring it
>     would need an extra reference per queue.
> 
> Is that accurate? P's instructions are still in tun->fprog_kern. It looks
> like sk_attach_filter_kern(&tun->fprog_kern, sk) could rebuild P on queues
> 0..i-1, the same way tun_attach() does for new queues. Another option
> would be to clear the saved state when the attach fails partway.
> 
This is not a state introduced here. Before this series the same failure ran
tun_detach_filter(tun, i), which also left queues 0..i-1 without a filter,
queues i..n-1 on the old program, and cleared filter_attached while
tun->fprog already held the new header. What this series changes is that
tun->fprog, tun->fprog_kern and filter_attached keep describing the program
queues i..n-1 still run, so a later attach retries it instead of silently
starting without a filter.

Rebuilding from tun->fprog_kern would not remove the failure:
__sk_attach_prog() uncharges the old sk_filter as soon as the replacement is
installed, so queues 0..i-1 no longer hold it, and the rebuild is a new
bpf_prog_create() on the path where that allocation, the optmem charge or
the JIT limit has just failed. It needs a fallback of its own. Building and
charging every program before swapping any of them in would require
splitting __sk_attach_prog(), which is a filter core change, not something to
mix into this fix.

Both are error paths of TUNATTACHFILTER with no corruption outside the
device, so I would leave them as they are.
> A failure partway through the loop looks reachable in several ways:
> 
>   - -ENOMEM from bpf_prog_alloc(), or from kmalloc_obj() in
>     __sk_attach_prog()
>   - the per-socket optmem check in __sk_filter_charge()
>   - under CONFIG_BPF_JIT_ALWAYS_ON, bpf_jit_limit being reached partway
>     through, because each queue JITs its own copy
>