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

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v4 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 as a workaround.

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

---
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
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
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
v1:
  - https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
---
 drivers/net/tun.c                 |  56 ++++++++-
 include/linux/filter.h            |   1 +
 net/core/filter.c                 |  22 +++++
 tools/testing/selftests/net/tun.c | 193 ++++++++++++++++++++++++++++++
 4 files changed, 267 insertions(+), 5 deletions(-)


base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0


No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v4 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 failed read (-EFAULT for an unmapped address, -EINVAL for
bytes that are not a valid classic BPF program) was not fatal: for a new
tfile err is overwritten by "err = 0", 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 at the same time when tun_attach() returns
early when sk_attach_filter_kern() succeeds instead of when it fails.
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 inverted check was discovered by manual code inspection first [1], and
the review of v1 reported the other things.

[1] https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/

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      | 56 ++++++++++++++++++++++++++++++++++++++----
 include/linux/filter.h |  1 +
 net/core/filter.c      | 22 +++++++++++++++++
 3 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68a..b58ad67b77ad 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,47 @@ 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;
+
+	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;
+	}
+
+	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 +788,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 +854,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
 	tun_set_real_num_queues(tun);
 out:
+	if (err && rollback_filter) {
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
@@ -2397,6 +2439,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)
@@ -3059,6 +3102,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;
 }
 
@@ -3070,7 +3116,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);
@@ -3418,8 +3464,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 39decde7fc73..0de5a738fb26 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1218,6 +1218,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 void bpf_prog_destroy(struct bpf_prog *fp);
 
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk);
 int sk_attach_bpf(u32 ufd, struct sock *sk);
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
 int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..64d6505a4ef2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(sk_attach_filter);
 
+int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
+{
+	struct bpf_prog *prog;
+	int err;
+
+	if (sock_flag(sk, SOCK_FILTER_LOCKED))
+		return -EPERM;
+
+	err = bpf_prog_create(&prog, fprog);
+	if (err)
+		return err;
+
+	err = __sk_attach_prog(prog, sk);
+	if (err < 0) {
+		__bpf_prog_release(prog);
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sk_attach_filter_kern);
+
 int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk)
 {
 	struct bpf_prog *prog = __get_filter(fprog, sk);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v4 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 failed read (-EFAULT for an unmapped address, -EINVAL for
> bytes that are not a valid classic BPF program) was not fatal: for a new
> tfile err is overwritten by "err = 0", 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 at the same time when tun_attach() returns
> early when sk_attach_filter_kern() succeeds instead of when it fails.
> 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 inverted check was discovered by manual code inspection first [1], and
> the review of v1 reported the other things.
> 
> [1] https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
> 
> 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 v4 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: 1 (3) · Low: 1

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

- [Medium] TUNATTACHFILTER can store an invalid classic BPF program and
  still return 0.
- [Low] The new exported helper sk_attach_filter_kern()
  (net/core/filter.c:1570-1590) attaches a program built by…

Pre-existing issues:
- [Medium] TUNDETACHFILTER does not remove the filter from detached
  (disabled) queues, so a re-enabled queue keeps filtering with an old…
- [Medium] TUNATTACHFILTER and TUNGETFILTER do not work for 32-bit user
  space on a 64-bit CONFIG_COMPAT kernel.
- [Medium] Replacing an attached filter leaves the device in an
  inconsistent state if attaching the new one fails partway.

--

tun: keep a kernel copy of the socket filter program

This patch makes tun keep the classic BPF filter for TUNATTACHFILTER in
kernel memory (tun->fprog_kern), so it no longer re-reads it from the user
pointer every time a queue is attached. Each queue gets its own program
from a new helper, sk_attach_filter_kern(). The patch also fixes the
inverted error check in tun_attach().

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

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

[ ... ]

> @@ -722,12 +723,47 @@ 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;
> +
> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
> +		return -EFAULT;

[Severity: Medium]
This isn't a bug introduced by this patch, but does TUNATTACHFILTER work
for 32-bit user space on a 64-bit CONFIG_COMPAT kernel?

TUNATTACHFILTER and TUNGETFILTER are defined with the size of struct
sock_fprog in include/uapi/linux/if_tun.h:

#define TUNATTACHFILTER _IOW('T', 213, struct sock_fprog)

sock_fprog contains a pointer, so it is 8 bytes on 32-bit and 16 bytes on
64-bit, and the encoded command numbers differ. tun_chr_compat_ioctl()
passes cmd through unchanged:

	return __tun_chr_ioctl(file, cmd, arg, sizeof(struct compat_ifreq));

So the 32-bit numbers match no case in __tun_chr_ioctl() and fall through
to the default tun_vnet_ioctl() branch.

Even if the numbers were normalised, this copy_from_user() reads the
native sock_fprog layout, and TUNGETFILTER writes the native layout back.
Nothing on this path converts to or from compat_sock_fprog.

> +
> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
> +		return -EINVAL;

[Severity: Medium]
Can TUNATTACHFILTER now store an invalid classic BPF program and still
return 0?

This check covers only the length. The instructions are checked only by
bpf_check_classic(), via
sk_attach_filter_kern()->bpf_prog_create()->bpf_prepare_filter(). That
runs once per attached queue in tun_attach_filter():

	for (i = 0; i < tun->numqueues; i++) {
		...
	}

	tun->filter_attached = true;
	return ret;

If tun->numqueues is 0, the loop never runs and the program is never
checked. filter_attached is still set to true and the ioctl returns
success.

This state can be reached on a multiqueue TAP. TUNSETQUEUE with
IFF_DETACH_QUEUE goes through __tun_detach(tfile, false), which leaves
tfile->tun set. The same fd can then issue TUNATTACHFILTER while no
queues are attached.

With the corrected check in tun_attach():

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

every later TUNSETQUEUE(IFF_ATTACH_QUEUE) through tun_set_queue(), and
every TUNSETIFF without IFF_NOFILTER through tun_set_iff(), would then
fail with -EINVAL from bpf_prepare_filter(). This lasts until someone
issues TUNDETACHFILTER.

Could the instructions be checked once in the TUNATTACHFILTER path,
whatever numqueues is? One option is to build and destroy a throwaway
prog with bpf_prog_create(). Another is to run bpf_check_classic() on
the copy.

There is also a smaller behaviour change. A bad header now makes the
ioctl fail early and leaves the previous filter in place. Before, it
failed on queue 0 and cleared filter_attached. Should the commit message
mention this?

> +
> +	insns = kmalloc_array(fprog.len, sizeof(struct sock_filter),
> +			      GFP_KERNEL_ACCOUNT);

[ ... ]

> @@ -3059,6 +3102,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 is a pre-existing issue, but does TUNDETACHFILTER leave the old
filter on queues that are currently detached?

tun_detach_filter(tun, tun->numqueues) walks only tun->tfiles[0..n). A
queue detached with IFF_DETACH_QUEUE sits on tun->disabled instead, and
__tun_detach(tfile, false) does not touch its sk->sk_filter.

When that queue is attached again while filter_attached is false,
tun_attach() skips the filter block:

	if (!skip_filter && (tun->filter_attached == true)) {

Take this sequence: attach a filter, IFF_DETACH_QUEUE on queue Q,
TUNDETACHFILTER, IFF_ATTACH_QUEUE on Q. Q keeps filtering packets with
the removed program, while the device reports no filter.

>  
> @@ -3070,7 +3116,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 isn't a bug introduced by this patch, but what happens to queues
i..numqueues-1 when replacing an attached filter fails on queue i?

tun_detach_filter(tun, i) detaches only queues 0..i-1. It then frees
fprog_kern and clears filter_attached (see tun_detach_filter() above).
Queues i..numqueues-1 keep the old sk_filter.

Failure at i > 0 can happen through -ENOMEM from __sk_filter_charge() or
the allocations. Failure at i == 0 can happen with an invalid program.

By this point tun_copy_filter() has already freed the old fprog_kern and
set tun->fprog to the new program. After the failure:

- the device reports no filter
- new queues attach without a filter
- some existing queues still run the old program
- TUNGETFILTER returns a program that is not attached anywhere

Memory safety is not affected, because bpf_prog_create() copies the
instructions. The unwind logic is the same as before the patch.

[ ... ]

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

[Severity: Low]
This isn't a bug, but bpf_prog_create() assumes its result is never
attached to a socket:

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

sk_attach_filter_kern() now attaches exactly such a program to
sk->sk_filter. When orig_prog is NULL, sk_get_filter() returns -EACCES
and sock_diag_put_filterinfo() silently leaves the filter out.

The commit message notes that tun sockets do not expose this. Even so,
the comment in bpf_prog_create() is now inaccurate. Unlike
sk_attach_filter(), the new exported helper also has no kernel-doc to
warn future callers.

Could sk_attach_filter_kern() save the original program, the way
bpf_prog_create_from_user() does with save_orig set? If not, could it
get kernel-doc that describes the limitation, along with an updated
comment in bpf_prog_create()?

[ ... ]

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

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

[Rongguang Wei]

on 2026/10/4 14:39, 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: 1 (3) · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] TUNATTACHFILTER can store an invalid classic BPF program and
>   still return 0.
> - [Low] The new exported helper sk_attach_filter_kern()
>   (net/core/filter.c:1570-1590) attaches a program built by…
> 
> Pre-existing issues:
> - [Medium] TUNDETACHFILTER does not remove the filter from detached
>   (disabled) queues, so a re-enabled queue keeps filtering with an old…
> - [Medium] TUNATTACHFILTER and TUNGETFILTER do not work for 32-bit user
>   space on a 64-bit CONFIG_COMPAT kernel.
> - [Medium] Replacing an attached filter leaves the device in an
>   inconsistent state if attaching the new one fails partway.
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This patch makes tun keep the classic BPF filter for TUNATTACHFILTER in
> kernel memory (tun->fprog_kern), so it no longer re-reads it from the user
> pointer every time a queue is attached. Each queue gets its own program
> from a new helper, sk_attach_filter_kern(). The patch also fixes the
> inverted error check in tun_attach().
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 5a302709a68aa..b58ad67b77ad1 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -722,12 +723,47 @@ 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;
>> +
>> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
>> +		return -EFAULT;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does TUNATTACHFILTER work
> for 32-bit user space on a 64-bit CONFIG_COMPAT kernel?
> 
> TUNATTACHFILTER and TUNGETFILTER are defined with the size of struct
> sock_fprog in include/uapi/linux/if_tun.h:
> 
> #define TUNATTACHFILTER _IOW('T', 213, struct sock_fprog)
> 
> sock_fprog contains a pointer, so it is 8 bytes on 32-bit and 16 bytes on
> 64-bit, and the encoded command numbers differ. tun_chr_compat_ioctl()
> passes cmd through unchanged:
> 
> 	return __tun_chr_ioctl(file, cmd, arg, sizeof(struct compat_ifreq));
> 
> So the 32-bit numbers match no case in __tun_chr_ioctl() and fall through
> to the default tun_vnet_ioctl() branch.
> 
> Even if the numbers were normalised, this copy_from_user() reads the
> native sock_fprog layout, and TUNGETFILTER writes the native layout back.
> Nothing on this path converts to or from compat_sock_fprog.
> 
>> +
>> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
>> +		return -EINVAL;
> 
> [Severity: Medium]
> Can TUNATTACHFILTER now store an invalid classic BPF program and still
> return 0?
> 
The issue itself is pre-existing.  Before this series tun_attach_filter()
walked the same numqueues and set filter_attached = true either way, so an
invalid program was stored without ever being checked. And fix it where the
program is stored, but bpf_check_classic() is static in net/core/filter.c, 
so it needs either a new exported wrapper or a throwaway bpf_prog_create().  
Either way it changes what TUNATTACHFILTER accepts, so I would rather send 
that as its own patch than fold it into a series that is otherwise about 
re-attaching.
> This check covers only the length. The instructions are checked only by
> bpf_check_classic(), via
> sk_attach_filter_kern()->bpf_prog_create()->bpf_prepare_filter(). That
> runs once per attached queue in tun_attach_filter():
> 
> 	for (i = 0; i < tun->numqueues; i++) {
> 		...
> 	}
> 
> 	tun->filter_attached = true;
> 	return ret;
> 
> If tun->numqueues is 0, the loop never runs and the program is never
> checked. filter_attached is still set to true and the ioctl returns
> success.
> 
> This state can be reached on a multiqueue TAP. TUNSETQUEUE with
> IFF_DETACH_QUEUE goes through __tun_detach(tfile, false), which leaves
> tfile->tun set. The same fd can then issue TUNATTACHFILTER while no
> queues are attached.
> 
> With the corrected check in tun_attach():
> 
> 		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
> 		release_sock(tfile->socket.sk);
> 		if (err)
> 			goto out;
> 
> every later TUNSETQUEUE(IFF_ATTACH_QUEUE) through tun_set_queue(), and
> every TUNSETIFF without IFF_NOFILTER through tun_set_iff(), would then
> fail with -EINVAL from bpf_prepare_filter(). This lasts until someone
> issues TUNDETACHFILTER.
> 
> Could the instructions be checked once in the TUNATTACHFILTER path,
> whatever numqueues is? One option is to build and destroy a throwaway
> prog with bpf_prog_create(). Another is to run bpf_check_classic() on
> the copy.
> 
> There is also a smaller behaviour change. A bad header now makes the
> ioctl fail early and leaves the previous filter in place. Before, it
> failed on queue 0 and cleared filter_attached. Should the commit message
> mention this?
> 
I will add in the commit message: the length check in tun_copy_filter()
runs before anything is replaced, so a bad header now leaves the previously
attached filter in place, while before the failed attach on queue 0 cleared
filter_attached.
>> +
>> +	insns = kmalloc_array(fprog.len, sizeof(struct sock_filter),
>> +			      GFP_KERNEL_ACCOUNT);
> 
> [ ... ]
> 
>> @@ -3059,6 +3102,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 is a pre-existing issue, but does TUNDETACHFILTER leave the old
> filter on queues that are currently detached?
> 
> tun_detach_filter(tun, tun->numqueues) walks only tun->tfiles[0..n). A
> queue detached with IFF_DETACH_QUEUE sits on tun->disabled instead, and
> __tun_detach(tfile, false) does not touch its sk->sk_filter.
> 
> When that queue is attached again while filter_attached is false,
> tun_attach() skips the filter block:
> 
> 	if (!skip_filter && (tun->filter_attached == true)) {
> 
> Take this sequence: attach a filter, IFF_DETACH_QUEUE on queue Q,
> TUNDETACHFILTER, IFF_ATTACH_QUEUE on Q. Q keeps filtering packets with
> the removed program, while the device reports no filter.
> 
>>  
>> @@ -3070,7 +3116,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 isn't a bug introduced by this patch, but what happens to queues
> i..numqueues-1 when replacing an attached filter fails on queue i?
> 
> tun_detach_filter(tun, i) detaches only queues 0..i-1. It then frees
> fprog_kern and clears filter_attached (see tun_detach_filter() above).
> Queues i..numqueues-1 keep the old sk_filter.
> 
> Failure at i > 0 can happen through -ENOMEM from __sk_filter_charge() or
> the allocations. Failure at i == 0 can happen with an invalid program.
> 
> By this point tun_copy_filter() has already freed the old fprog_kern and
> set tun->fprog to the new program. After the failure:
> 
> - the device reports no filter
> - new queues attach without a filter
> - some existing queues still run the old program
> - TUNGETFILTER returns a program that is not attached anywhere
> 
> Memory safety is not affected, because bpf_prog_create() copies the
> instructions. The unwind logic is the same as before the patch.
> 
> [ ... ]
> 
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 70dc621672f2e..64d6505a4ef23 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>  }
>>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>>  
>> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
>> +{
>> +	struct bpf_prog *prog;
>> +	int err;
>> +
>> +	if (sock_flag(sk, SOCK_FILTER_LOCKED))
>> +		return -EPERM;
>> +
>> +	err = bpf_prog_create(&prog, fprog);
>> +	if (err)
>> +		return err;
> 
> [Severity: Low]
> This isn't a bug, but bpf_prog_create() assumes its result is never
> attached to a socket:
> 
> 	/* Since unattached filters are not copied back to user
> 	 * space through sk_get_filter(), we do not need to hold
> 	 * a copy here, and can spare us the work.
> 	 */
> 	fp->orig_prog = NULL;
> 
> sk_attach_filter_kern() now attaches exactly such a program to
> sk->sk_filter. When orig_prog is NULL, sk_get_filter() returns -EACCES
> and sock_diag_put_filterinfo() silently leaves the filter out.
> 
> The commit message notes that tun sockets do not expose this. Even so,
> the comment in bpf_prog_create() is now inaccurate. Unlike
> sk_attach_filter(), the new exported helper also has no kernel-doc to
> warn future callers.
> 
> Could sk_attach_filter_kern() save the original program, the way
> bpf_prog_create_from_user() does with save_orig set? If not, could it
> get kernel-doc that describes the limitation, along with an updated
> comment in bpf_prog_create()?
> 
No tun socket is ever exposed as a file descriptor, so SO_GET_FILTER and
sock_diag cannot observe the difference, while it would add an allocation
and a copy to every queue attach.
I will add kernel-doc for it in the respin.
> [ ... ]
> 
pw-bot: cr

[PATCH net v4 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 rejected
TUNATTACHFILTER leaves the installed program and the filter state that
TUNGETIFF reports alone, and that a queue attached afterwards still gets
the filter.

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

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index abe488bac50b..cfaba5727402 100644
--- a/tools/testing/selftests/net/tun.c
+++ b/tools/testing/selftests/net/tun.c
@@ -8,8 +8,10 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <linux/filter.h>
 #include <linux/if_tun.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/socket.h>
 
 #include "kselftest_harness.h"
@@ -542,6 +544,197 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len */
+static const struct sock_filter filter_accept[] = {
+	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
+	BPF_STMT(BPF_RET | BPF_A, 0),
+};
+
+/* 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);
+
+	/* The process that called TUNATTACHFILTER no longer maps this memory */
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
+
+	ret = tun_attach(self->fd, self->ifname);
+	EXPECT_EQ(ret, 0);
+
+	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);
+
+	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);
+}
+
 FIXTURE(tun_vnet_udptnl)
 {
 	char ifname[IFNAMSIZ];
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v4 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 rejected
> TUNATTACHFILTER leaves the installed program and the filter state that
> TUNGETIFF reports alone, and that a queue attached afterwards still gets
> the filter.
> 
> 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 v4 2/2] selftests: net: add TAP socket filter attach tests

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c), the inline comment "The process…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests in tools/testing/selftests/net/tun.c for
socket filter attach. They check that a filter is reattached from the
kernel copy after the user buffer becomes unreadable, that a rejected
TUNATTACHFILTER leaves the existing program in place, and that
IFF_NOFILTER and TUNDETACHFILTER both result in a queue without a filter.

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

[ ... ]

> +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);
> +
> +	/* The process that called TUNATTACHFILTER no longer maps this memory */
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
> +
> +	ret = tun_attach(self->fd, self->ifname);
> +	EXPECT_EQ(ret, 0);
> +
> +	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);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Low]
Is the comment above the mprotect() call accurate? mprotect(PROT_NONE)
leaves the page mapped and only makes it inaccessible. Later in the same
test the protection is put back and the page is unmapped:

	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
	ASSERT_EQ(munmap(prog, getpagesize()), 0);

Would something like "this memory is no longer readable" describe it
better?

Also, do the IFF_NOFILTER and gf.len checks on self->fd test the reattach
at all?

__tun_detach() with clean=false does not remove the socket filter from the
detached queue. So IFF_NOFILTER would still be clear on self->fd even if
tun_attach() installed nothing.

TUNGETFILTER returns the tun->fprog header, and the reattach path does not
change it. So gf.len matches no matter what happened during reattach.

The checks that do test something are ret == 0, IFF_DETACH_QUEUE == 0, and
the IFF_NOFILTER check on the fresh socket below. Could the two redundant
checks be dropped, or replaced with ones that can tell a reinstalled filter
from no filter?

On the patched kernel, sk_attach_filter_kern() succeeds from the kernel
copy. So this test does run the detached-queue success path, and the
IFF_DETACH_QUEUE check would catch the earlier "success, goto out without
publishing" bug if it came back.

> +
> +	/* A new queue starts without a filter, so a clear IFF_NOFILTER shows
> +	 * that the filter was installed again from the kernel copy.
> +	 */

[ ... ]

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