[PATCH net v3 0/3] tun: fix re-attaching the socket filter

[PATCH net v3 0/3] tun: fix re-attaching the socket filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

This is v3 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.
It also adds sk_attach_filter_kern(), the kernel memory counterpart of
sk_attach_filter(), which the patch uses. That is the second issue reported
in the review of v1; IFF_NOFILTER keeps working and is no longer needed as
a workaround.

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

It has to follow patch 1: with the check fixed but the program still read
from the caller's address space, an attach that used to succeed without a
filter would fail with -EFAULT or -EINVAL.

Patch 3 adds selftests for the re-attach path and for attaching a queue
without a filter. The two tests that cover the kernel copy fail without
patch 1; the rollback in patch 2 needs a failure inside tun_attach() after
the filter attach, which userspace cannot trigger reliably, so it has no
test.

Thanks to Willem de Bruijn for the review.

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

---
v3:
  - reorder: the kernel copy of the program comes before the corrected
    error check, so that no step of the series turns an attach that
    succeeded without a filter into a failure
  - 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                 |  50 +++++++++-
 include/linux/filter.h            |   1 +
 net/core/filter.c                 |  22 +++++
 tools/testing/selftests/net/tun.c | 155 ++++++++++++++++++++++++++++++
 4 files changed, 223 insertions(+), 5 deletions(-)


base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0

No virus found
		Checked by Hillstone Network AntiVirus

[PATCH net v3 1/3] tun: keep a kernel copy of the socket filter program

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

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

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

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

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

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

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68a..22fb34e745bd 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -197,6 +197,7 @@ struct tun_struct {
 	int			sndbuf;
 	struct tap_filter	txflt;
 	struct sock_fprog	fprog;
+	struct sock_fprog_kern	fprog_kern;
 	/* protected by rtnl lock */
 	bool			filter_attached;
 	u32			msg_enable;
@@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
 	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
 }
 
+/* Copy the filter that @argp points at into the kernel, so that it can be
+ * installed again later, from any context. tun->fprog and tun->fprog_kern
+ * are updated only once the copy succeeded.
+ */
+static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
+{
+	struct sock_fprog fprog;
+	struct sock_filter *insns;
+
+	if (copy_from_user(&fprog, argp, sizeof(fprog)))
+		return -EFAULT;
+
+	if (!fprog.len || fprog.len > BPF_MAXINSNS)
+		return -EINVAL;
+
+	insns = memdup_array_user(fprog.filter, fprog.len,
+				  sizeof(struct sock_filter));
+	if (IS_ERR(insns))
+		return PTR_ERR(insns);
+
+	kfree(tun->fprog_kern.filter);
+	tun->fprog_kern.len = fprog.len;
+	tun->fprog_kern.filter = insns;
+	tun->fprog = fprog;
+
+	return 0;
+}
+
 static int tun_attach(struct tun_struct *tun, struct file *file,
 		      bool skip_filter, bool napi, bool napi_frags,
 		      bool publish_tun)
@@ -752,7 +781,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	/* Re-attach the filter to persist device */
 	if (!skip_filter && (tun->filter_attached == true)) {
 		lock_sock(tfile->socket.sk);
-		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
+		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
 		if (!err)
 			goto out;
@@ -2397,6 +2426,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 +3089,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 +3103,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 +3451,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 v3 1/3] 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: unmapped there, the attach fails with -EFAULT;
> mapped, whatever bytes it holds become the filter of the new queue.
> 
> Keep the program in the kernel instead. tun->fprog_kern holds the
> instructions and each queue gets its own program, built with
> sk_attach_filter_kern().
> 
> sk_attach_filter_kern() reads no user buffer and attaches the program like
> sk_attach_filter() does. The caller must hold the socket lock. Failing the
> attach releases it; so does the socket when the filter is replaced,
> detached or the socket goes away.
> 
> The copy is freed when the filter is detached or replaced and with the
> device, and tun->fprog is left untouched so TUNGETFILTER keeps its uapi
> behaviour.
> 
> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

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

> +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);

Interesting small issue, not new with this patch:

The BPF program is not validated on copy_from_user, but on attach,
possibly after the system call has already returned with success.
Which results in a dev without filter, rather than a failed syscall.

> +	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 v3 1/3] tun: keep a kernel copy of the socket filter program

[Rongguang Wei]

on 2026/10/1 02:33, Willem de Bruijn wrote:
> 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: unmapped there, the attach fails with -EFAULT;
>> mapped, whatever bytes it holds become the filter of the new queue.
>>
>> Keep the program in the kernel instead. tun->fprog_kern holds the
>> instructions and each queue gets its own program, built with
>> sk_attach_filter_kern().
>>
>> sk_attach_filter_kern() reads no user buffer and attaches the program like
>> sk_attach_filter() does. The caller must hold the socket lock. Failing the
>> attach releases it; so does the socket when the filter is replaced,
>> detached or the socket goes away.
>>
>> The copy is freed when the filter is detached or replaced and with the
>> device, and tun->fprog is left untouched so TUNGETFILTER keeps its uapi
>> behaviour.
>>
>> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
>> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> 
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
>> +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);
> 
> Interesting small issue, not new with this patch:
> 
> The BPF program is not validated on copy_from_user, but on attach,
> possibly after the system call has already returned with success.
> Which results in a dev without filter, rather than a failed syscall.
> 
Got it, a device with no queues still returns success. I will look at it separately.
Thanks for the review.
>> +	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 v3 1/3] tun: keep a kernel copy of the socket filter program

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (2) · Low: 3 (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] After this patch alone, tun_attach() in drivers/net/tun.c still
  has the inverted check `if (!err) goto out;` right after `err =…
- [Low] The commit message gets both the old failure mode and the new
  helper's semantics wrong.
- [Low] The new exported helper sk_attach_filter_kern()
  (net/core/filter.c) builds the program with bpf_prog_create() and then…
- [Low] The new comment above tun_copy_filter()
  (drivers/net/tun.c:726-729) says the kernel copy lets the filter "be…

Pre-existing issues:
- [Medium] tun has no compat translation for the classic filter ioctls.
- [Medium] If TUNATTACHFILTER replaces an attached filter and
  sk_attach_filter_kern() fails on queue i (for example -ENOMEM from…
- [Low] When TUNATTACHFILTER is issued while tun->numqueues == 0, the
  classic BPF program is stored without ever being validated.

--

tun: keep a kernel copy of the socket filter program

This patch changes TUNATTACHFILTER to copy the classic BPF instructions
into tun->fprog_kern. Before, only the sock_fprog header was stored, and
it pointed into the ioctl caller's address space. It also adds a new
exported helper, sk_attach_filter_kern(), which builds and attaches a
program from a kernel sock_fprog_kern, and tun_attach() and
tun_attach_filter() now use it.

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

> tun_attach() reads it again whenever a queue is attached to the
> persistent device later on: unmapped there, the attach fails with -EFAULT;
> mapped, whatever bytes it holds become the filter of the new queue.

[Severity: Low]
Is this an accurate description of the old behaviour? tun_attach() has an
inverted "if (!err) goto out;", so an -EFAULT, or an -EINVAL caused by
garbage bytes, falls through instead of failing the attach.

For a new tfile, xdp_rxq_info_reg() and "err = 0" then overwrite err. So
TUNSETIFF succeeds, the queue is attached, and no filter is installed.

A successful read of mapped, valid bytes returns 0 without attaching the
queue at all.

The follow-up patch's message talks about "a re-attach that used to
succeed without installing any filter". That seems to contradict this
one.

> sk_attach_filter_kern() reads no user buffer and attaches the program like
> sk_attach_filter() does.

Do the two really behave the same way? sk_attach_filter_kern() builds the
program with bpf_prog_create(), which sets fp->orig_prog = NULL.
sk_attach_filter() goes through __get_filter()->bpf_prog_store_orig_filter()
instead. As a result, SO_GET_FILTER and sock_diag readback give different
results depending on which helper attached the filter.

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

[ ... ]

> @@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>  }
>  
> +/* Copy the filter that @argp points at into the kernel, so that it can be
> + * installed again later, from any context. tun->fprog and tun->fprog_kern
> + * are updated only once the copy succeeded.
> + */

[Severity: Low]
Is "from any context" accurate here? The only consumer,
sk_attach_filter_kern(), needs sleepable process context and the socket
lock:

sk_attach_filter_kern()
  bpf_prog_create()
    bpf_prog_alloc()          <- GFP_KERNEL based allocation
  __sk_attach_prog()
    kmalloc_obj()             <- GFP_KERNEL
    rcu_dereference_protected(..., lockdep_sock_is_held(sk))

Was the intended meaning "from any process", or "independent of the ioctl
caller's address space"?

> +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 tun has no compat handling
for the classic filter ioctls. TUNATTACHFILTER, TUNDETACHFILTER and
TUNGETFILTER encode sizeof(struct sock_fprog) in the command number. That
size is 8 bytes for 32-bit userspace and 16 bytes natively, so the command
numbers differ.

tun_chr_compat_ioctl() only converts these:

	case TUNSETIFF:
	case TUNGETIFF:
	case TUNSETTXFILTER:
	case TUNGETSNDBUF:
	case TUNSETSNDBUF:
	case SIOCGIFHWADDR:
	case SIOCSIFHWADDR:
		arg = (unsigned long)compat_ptr(arg);
		break;

The compat filter commands therefore miss the native cases in
__tun_chr_ioctl() and end up in tun_vnet_ioctl(), which returns -EINVAL.
tun_copy_filter() is never reached with a compat layout. The result is
that 32-bit userspace on a 64-bit kernel cannot use these ioctls.

This copy is being rewritten anyway. Would it be worth adding compat
command translation here, perhaps together with the compat-aware
copy_bpf_fprog_from_user() that SO_ATTACH_FILTER uses?

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

[Severity: Low]
This is a pre-existing issue, but only the length is checked before the
program is committed to tun->fprog_kern. If TUNATTACHFILTER is issued
while tun->numqueues == 0, does the program ever get validated?

A multiqueue TAP reaches that state after TUNSETQUEUE(IFF_DETACH_QUEUE) on
every queue. __tun_detach(tfile, false) lowers numqueues but keeps
tfile->tun, so tun_get() still succeeds in __tun_chr_ioctl().

In that case the loop in tun_attach_filter() runs zero times, and
bpf_check_classic() (via bpf_prog_create()->bpf_prepare_filter()) never
runs. filter_attached is still set to true and 0 is returned.

After that, every tun_attach() fails in sk_attach_filter_kern() with
-EINVAL until TUNDETACHFILTER. The old code had the same gap: it stored an
unvalidated header when numqueues == 0.

> +
> +	insns = memdup_array_user(fprog.filter, fprog.len,
> +				  sizeof(struct sock_filter));

[ ... ]

> @@ -752,7 +781,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	/* Re-attach the filter to persist device */
>  	if (!skip_filter && (tun->filter_attached == true)) {
>  		lock_sock(tfile->socket.sk);
> -		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
>  		if (!err)
>  			goto out;

[Severity: High]
With only this patch applied, is the check after sk_attach_filter_kern()
still inverted?

The program is now built from a kernel copy, so the build succeeds for
any valid program. On a persistent TAP with filter_attached set, TUNSETIFF
without IFF_NOFILTER and TUNSETQUEUE(IFF_ATTACH_QUEUE) would then jump
straight to out and return 0.

That jump skips all of the following:

  - ptr_ring_resize()
  - the queue_index setup
  - xdp_rxq registration
  - sock_hold()/tun_napi_init() or tun_enable_queue()
  - publishing tfile->tun and tun->tfiles[]
  - the numqueues increment

tun_set_iff() and tun_set_queue() report success, but the fd stays
unattached, and later reads and writes fail with -EBADFD.

The reverse also happens: when the build fails, the code falls through and
attaches the queue with no filter.

The next patch in the series, "tun: fix inverted error check when
re-attaching the filter", changes this to "if (err) goto out;". It also
adds a rollback_filter sk_detach_filter() on the later failure paths, so
the code is correct at the end of the series.

Could that fix be folded into this patch, or ordered before it? Otherwise
a bisect can land on a kernel where re-attaching to a filtered persistent
TAP silently does nothing.

[ ... ]

> @@ -3070,7 +3103,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 the
remaining queues in this case? TUNATTACHFILTER replaces an attached
filter, and sk_attach_filter_kern() fails on queue i, for example with
-ENOMEM from __sk_filter_charge() at the optmem limit.

tun_detach_filter(tun, i) only detaches queues 0..i-1, then frees the new
kernel copy and clears filter_attached. Queues i..numqueues-1 keep the
previous sk_filter.

The previous copy has already been freed in tun_copy_filter():

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

So the old state can't be restored, and queues attached later get no
filter. The device ends up with some queues unfiltered and others still
using the old filter.

The old code had the same prefix-only rollback, since it overwrote
tun->fprog in place before attaching.

[ ... ]

> 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;
> +
> +	err = __sk_attach_prog(prog, sk);

[Severity: Low]
bpf_prog_create() is documented as creating an unattached filter, and it
deliberately skips orig_prog:

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

This helper now attaches such a program to sk->sk_filter. Does
sk_get_filter() then return -EACCES for these sockets?

net/core/filter.c:sk_get_filter() {
	...
	ret = -EACCES;
	fprog = filter->prog->orig_prog;
	if (!fprog)
		goto out;
	...
}

sock_diag would also leave the filter out. sk_attach_filter() keeps
orig_prog through bpf_prog_store_orig_filter().

tun sockets can't reach SO_GET_FILTER today. However, this helper is
exported and declared next to sk_attach_filter() in include/linux/filter.h.
Should the difference be documented, or should orig_prog be stored here as
well?

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

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

[Rongguang Wei]

on 2026/10/1 22:10, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (2) · Low: 3 (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] After this patch alone, tun_attach() in drivers/net/tun.c still
>   has the inverted check `if (!err) goto out;` right after `err =
Make the check first is not an option either (it would turn an attach 
that used to succeed without a filter into a failure). It seems the only
way is that the fix be folded into this patch, so no step of the series
takes goto out with err == 0.
> - [Low] The commit message gets both the old failure mode and the new
>   helper's semantics wrong.
> - [Low] The new exported helper sk_attach_filter_kern()
>   (net/core/filter.c) builds the program with bpf_prog_create() and then…
> - [Low] The new comment above tun_copy_filter()
>   (drivers/net/tun.c:726-729) says the kernel copy lets the filter "be…
I will reword it.
> 
> Pre-existing issues:
> - [Medium] tun has no compat translation for the classic filter ioctls.
I can send the compat translation as a separate patch.
> - [Medium] If TUNATTACHFILTER replaces an attached filter and
>   sk_attach_filter_kern() fails on queue i (for example -ENOMEM from…
The old code also overwrote tun->fprog before attaching, and the prefix-only
rollback is unchanged. A proper fix is to install the new copy on every queue 
and only then replace the saved one. Separate patch.
> - [Low] When TUNATTACHFILTER is issued while tun->numqueues == 0, the
>   classic BPF program is stored without ever being validated.The old code stored an unvalidated header in the same situation. 
Validating the program at copy time can be a separate change.
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This patch changes TUNATTACHFILTER to copy the classic BPF instructions
> into tun->fprog_kern. Before, only the sock_fprog header was stored, and
> it pointed into the ioctl caller's address space. It also adds a new
> exported helper, sk_attach_filter_kern(), which builds and attaches a
> program from a kernel sock_fprog_kern, and tun_attach() and
> tun_attach_filter() now use it.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
>> tun_attach() reads it again whenever a queue is attached to the
>> persistent device later on: unmapped there, the attach fails with -EFAULT;
>> mapped, whatever bytes it holds become the filter of the new queue.
> 
> [Severity: Low]
> Is this an accurate description of the old behaviour? tun_attach() has an
> inverted "if (!err) goto out;", so an -EFAULT, or an -EINVAL caused by
> garbage bytes, falls through instead of failing the attach.
> 
> For a new tfile, xdp_rxq_info_reg() and "err = 0" then overwrite err. So
> TUNSETIFF succeeds, the queue is attached, and no filter is installed.
> 
> A successful read of mapped, valid bytes returns 0 without attaching the
> queue at all.
> 
> The follow-up patch's message talks about "a re-attach that used to
> succeed without installing any filter". That seems to contradict this
> one.
> 
>> sk_attach_filter_kern() reads no user buffer and attaches the program like
>> sk_attach_filter() does.
> 
> Do the two really behave the same way? sk_attach_filter_kern() builds the
> program with bpf_prog_create(), which sets fp->orig_prog = NULL.
> sk_attach_filter() goes through __get_filter()->bpf_prog_store_orig_filter()
> instead. As a result, SO_GET_FILTER and sock_diag readback give different
> results depending on which helper attached the filter.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 5a302709a68aa..22fb34e745bd9 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>>  }
>>  
>> +/* Copy the filter that @argp points at into the kernel, so that it can be
>> + * installed again later, from any context. tun->fprog and tun->fprog_kern
>> + * are updated only once the copy succeeded.
>> + */
> 
> [Severity: Low]
> Is "from any context" accurate here? The only consumer,
> sk_attach_filter_kern(), needs sleepable process context and the socket
> lock:
> 
> sk_attach_filter_kern()
>   bpf_prog_create()
>     bpf_prog_alloc()          <- GFP_KERNEL based allocation
>   __sk_attach_prog()
>     kmalloc_obj()             <- GFP_KERNEL
>     rcu_dereference_protected(..., lockdep_sock_is_held(sk))
> 
> Was the intended meaning "from any process", or "independent of the ioctl
> caller's address space"?
> 
>> +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 tun has no compat handling
> for the classic filter ioctls. TUNATTACHFILTER, TUNDETACHFILTER and
> TUNGETFILTER encode sizeof(struct sock_fprog) in the command number. That
> size is 8 bytes for 32-bit userspace and 16 bytes natively, so the command
> numbers differ.
> 
> tun_chr_compat_ioctl() only converts these:
> 
> 	case TUNSETIFF:
> 	case TUNGETIFF:
> 	case TUNSETTXFILTER:
> 	case TUNGETSNDBUF:
> 	case TUNSETSNDBUF:
> 	case SIOCGIFHWADDR:
> 	case SIOCSIFHWADDR:
> 		arg = (unsigned long)compat_ptr(arg);
> 		break;
> 
> The compat filter commands therefore miss the native cases in
> __tun_chr_ioctl() and end up in tun_vnet_ioctl(), which returns -EINVAL.
> tun_copy_filter() is never reached with a compat layout. The result is
> that 32-bit userspace on a 64-bit kernel cannot use these ioctls.
> 
> This copy is being rewritten anyway. Would it be worth adding compat
> command translation here, perhaps together with the compat-aware
> copy_bpf_fprog_from_user() that SO_ATTACH_FILTER uses?
> 
>> +
>> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
>> +		return -EINVAL;
> 
> [Severity: Low]
> This is a pre-existing issue, but only the length is checked before the
> program is committed to tun->fprog_kern. If TUNATTACHFILTER is issued
> while tun->numqueues == 0, does the program ever get validated?
> 
> A multiqueue TAP reaches that state after TUNSETQUEUE(IFF_DETACH_QUEUE) on
> every queue. __tun_detach(tfile, false) lowers numqueues but keeps
> tfile->tun, so tun_get() still succeeds in __tun_chr_ioctl().
> 
> In that case the loop in tun_attach_filter() runs zero times, and
> bpf_check_classic() (via bpf_prog_create()->bpf_prepare_filter()) never
> runs. filter_attached is still set to true and 0 is returned.
> 
> After that, every tun_attach() fails in sk_attach_filter_kern() with
> -EINVAL until TUNDETACHFILTER. The old code had the same gap: it stored an
> unvalidated header when numqueues == 0.
> 
>> +
>> +	insns = memdup_array_user(fprog.filter, fprog.len,
>> +				  sizeof(struct sock_filter));
> 
> [ ... ]
> 
>> @@ -752,7 +781,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>>  	/* Re-attach the filter to persist device */
>>  	if (!skip_filter && (tun->filter_attached == true)) {
>>  		lock_sock(tfile->socket.sk);
>> -		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>> +		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>>  		if (!err)
>>  			goto out;
> 
> [Severity: High]
> With only this patch applied, is the check after sk_attach_filter_kern()
> still inverted?
> 
> The program is now built from a kernel copy, so the build succeeds for
> any valid program. On a persistent TAP with filter_attached set, TUNSETIFF
> without IFF_NOFILTER and TUNSETQUEUE(IFF_ATTACH_QUEUE) would then jump
> straight to out and return 0.
> 
> That jump skips all of the following:
> 
>   - ptr_ring_resize()
>   - the queue_index setup
>   - xdp_rxq registration
>   - sock_hold()/tun_napi_init() or tun_enable_queue()
>   - publishing tfile->tun and tun->tfiles[]
>   - the numqueues increment
> 
> tun_set_iff() and tun_set_queue() report success, but the fd stays
> unattached, and later reads and writes fail with -EBADFD.
> 
> The reverse also happens: when the build fails, the code falls through and
> attaches the queue with no filter.
> 
> The next patch in the series, "tun: fix inverted error check when
> re-attaching the filter", changes this to "if (err) goto out;". It also
> adds a rollback_filter sk_detach_filter() on the later failure paths, so
> the code is correct at the end of the series.
> 
> Could that fix be folded into this patch, or ordered before it? Otherwise
> a bisect can land on a kernel where re-attaching to a filtered persistent
> TAP silently does nothing.
> > [ ... ]
> 
>> @@ -3070,7 +3103,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 the
> remaining queues in this case? TUNATTACHFILTER replaces an attached
> filter, and sk_attach_filter_kern() fails on queue i, for example with
> -ENOMEM from __sk_filter_charge() at the optmem limit.
> 
> tun_detach_filter(tun, i) only detaches queues 0..i-1, then frees the new
> kernel copy and clears filter_attached. Queues i..numqueues-1 keep the
> previous sk_filter.
> 
> The previous copy has already been freed in tun_copy_filter():
> 
> 	kfree(tun->fprog_kern.filter);
> 	tun->fprog_kern.len = fprog.len;
> 	tun->fprog_kern.filter = insns;
> 	tun->fprog = fprog;
> 
> So the old state can't be restored, and queues attached later get no
> filter. The device ends up with some queues unfiltered and others still
> using the old filter.
> 
> The old code had the same prefix-only rollback, since it overwrote
> tun->fprog in place before attaching.
> 
> [ ... ]
> 
>> 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;
>> +
>> +	err = __sk_attach_prog(prog, sk);
> 
> [Severity: Low]
> bpf_prog_create() is documented as creating an unattached filter, and it
> deliberately skips orig_prog:
> 
> net/core/filter.c:bpf_prog_create() {
> 	...
> 	/* Since unattached filters are not copied back to user
> 	 * space through sk_get_filter(), we do not need to hold
> 	 * a copy here, and can spare us the work.
> 	 */
> 	fp->orig_prog = NULL;
> 	...
> }
> 
> This helper now attaches such a program to sk->sk_filter. Does
> sk_get_filter() then return -EACCES for these sockets?
> 
> net/core/filter.c:sk_get_filter() {
> 	...
> 	ret = -EACCES;
> 	fprog = filter->prog->orig_prog;
> 	if (!fprog)
> 		goto out;
> 	...
> }
> 
> sock_diag would also leave the filter out. sk_attach_filter() keeps
> orig_prog through bpf_prog_store_orig_filter().
> 
> tun sockets can't reach SO_GET_FILTER today. However, this helper is
> exported and declared next to sk_attach_filter() in include/linux/filter.h.
> Should the difference be documented, or should orig_prog be stored here as
> well?
> 
pw-bot: cr

[PATCH net v3 2/3] tun: fix inverted error check when re-attaching the filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

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

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

This has to come after the patch that keeps a kernel copy of the program:
with the check fixed but the program still read from the caller's address
space, a re-attach that used to succeed without installing any filter would
fail with -EFAULT or -EINVAL.

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

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 22fb34e745bd..c796048742f9 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -757,6 +757,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 {
 	struct tun_file *tfile = file->private_data;
 	struct net_device *dev = tun->dev;
+	bool rollback_filter = false;
 	int err;
 
 	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
@@ -783,8 +784,9 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 		lock_sock(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 &&
@@ -846,6 +848,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
 	tun_set_real_num_queues(tun);
 out:
+	if (err && rollback_filter) {
+		lock_sock(tfile->socket.sk);
+		sk_detach_filter(tfile->socket.sk);
+		release_sock(tfile->socket.sk);
+	}
 	return err;
 }
 
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v3 2/3] tun: fix inverted error check when re-attaching the filter

[Willem de Bruijn]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> tun_attach() returns early when sk_attach_filter_kern() succeeds instead of
> when it fails, so a queue attached to a persistent TAP is never published
> in tun->tfiles[] while TUNSETIFF still reported success.
> 
> Fix the check, so that a failed re-attach aborts the attach, and detach the
> filter again if a later step of tun_attach() fails.
> 
> This has to come after the patch that keeps a kernel copy of the program:
> with the check fixed but the program still read from the caller's address
> space, a re-attach that used to succeed without installing any filter would
> fail with -EFAULT or -EINVAL.
> 
> 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 v3 2/3] tun: fix inverted error check when re-attaching the filter

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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] The series cannot be bisected, and the commit message only
  states the ordering constraint in one direction.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the TAP
  filter lifecycle treats disabled queues differently on detach and on…

--

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

This commit fixes the inverted error check after sk_attach_filter_kern()
in tun_attach(). Before the fix, a successful re-attach returned early and
the queue was never published in tun->tfiles[]. The patch also detaches
the filter again if a later step of tun_attach() fails.

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

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

[ ... ]

> @@ -783,8 +784,9 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  		lock_sock(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;

[Severity: Medium]
The commit message says:

    This has to come after the patch that keeps a kernel copy of the
    program: ...

Doesn't the dependency also run the other way?

At the baseline, tun_attach() called sk_attach_filter(&tun->fprog, ...),
which reads a stale user pointer. The inverted "if (!err) goto out" only
fired when that pointer was still mapped and held a valid program.

After "tun: keep a kernel copy of the socket filter program", the call is
sk_attach_filter_kern(&tun->fprog_kern, ...). That function works on a copy
that tun_attach_filter() has already validated. In practice it succeeds,
unless it hits -ENOMEM or SOCK_FILTER_LOCKED.

In that intermediate commit, every re-attach with tun->filter_attached set
would take goto out with err == 0. Both of these paths reach it:

  tun_set_iff()->tun_attach(tun, file, ifr->ifr_flags & IFF_NOFILTER, ...)
  tun_set_queue()->tun_attach(tun, file, false, ...)

TUNSETIFF, or TUNSETQUEUE with IFF_ATTACH_QUEUE, would return 0. The queue
would never show up in tun->tfiles[], and tfile->tun would not be
published.

This patch, the next one in the series, fixes that. However, both patches
carry the same Fixes: 54f968d6efdb tag, and neither one says it needs the
other.

The first patch looks like the standalone fix because it stops reading the
user pointer. If a backport takes only that patch, could re-attach break
for every TAP that has a filter?

Would it make sense to squash the two patches, or to note the dependency in
both commit messages?

> +		rollback_filter = true;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The TAP
filter lifecycle seems to treat disabled queues differently on detach and
on re-attach.

TUNDETACHFILTER only walks the active queues:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
	case TUNDETACHFILTER:
		...
		tun_detach_filter(tun, tun->numqueues);
    ...
}

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

For IFF_DETACH_QUEUE, __tun_detach() runs with clean == false. That path
calls tun_disable_queue() and tun_napi_disable() but never
sk_detach_filter(), so a tfile parked on tun->disabled keeps its
sk_filter.

tun_attach() only touches the filter when tun->filter_attached is true. It
never clears a leftover filter.

Consider this sequence on a multiqueue TAP:

  TUNATTACHFILTER           (queue Q is active and gets the filter)
  IFF_DETACH_QUEUE on Q     (Q moves to tun->disabled, filter kept)
  TUNDETACHFILTER           (Q skipped, filter_attached = false)
  IFF_ATTACH_QUEUE on Q     (re-attach branch in tun_attach() skipped)

Would Q then be active with the old classic BPF filter still installed,
even though the device no longer has a filter? If so, packets on Q would
still be filtered or truncated.

The baseline behaves the same way here, so this patch does not change
it.

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

Re: [PATCH net v3 2/3] tun: fix inverted error check when re-attaching the filter

[Rongguang Wei]

on 2026/10/1 22:10, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (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] The series cannot be bisected, and the commit message only
>   states the ordering constraint in one direction.
This is the same point as the High in the review of patch 1.
Make the two patches are merged to fix.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the TAP
>   filter lifecycle treats disabled queues differently on detach and on…
> 
I will send that as a separate change.
> --
> 
> tun: fix inverted error check when re-attaching the filter
> 
> This commit fixes the inverted error check after sk_attach_filter_kern()
> in tun_attach(). Before the fix, a successful re-attach returned early and
> the queue was never published in tun->tfiles[]. The patch also detaches
> the filter again if a later step of tun_attach() fails.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 22fb34e745bd9..c796048742f9b 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -783,8 +784,9 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>>  		lock_sock(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;
> 
> [Severity: Medium]
> The commit message says:
> 
>     This has to come after the patch that keeps a kernel copy of the
>     program: ...
> 
> Doesn't the dependency also run the other way?
> 
> At the baseline, tun_attach() called sk_attach_filter(&tun->fprog, ...),
> which reads a stale user pointer. The inverted "if (!err) goto out" only
> fired when that pointer was still mapped and held a valid program.
> 
> After "tun: keep a kernel copy of the socket filter program", the call is
> sk_attach_filter_kern(&tun->fprog_kern, ...). That function works on a copy
> that tun_attach_filter() has already validated. In practice it succeeds,
> unless it hits -ENOMEM or SOCK_FILTER_LOCKED.
> 
> In that intermediate commit, every re-attach with tun->filter_attached set
> would take goto out with err == 0. Both of these paths reach it:
> 
>   tun_set_iff()->tun_attach(tun, file, ifr->ifr_flags & IFF_NOFILTER, ...)
>   tun_set_queue()->tun_attach(tun, file, false, ...)
> 
> TUNSETIFF, or TUNSETQUEUE with IFF_ATTACH_QUEUE, would return 0. The queue
> would never show up in tun->tfiles[], and tfile->tun would not be
> published.
> 
> This patch, the next one in the series, fixes that. However, both patches
> carry the same Fixes: 54f968d6efdb tag, and neither one says it needs the
> other.
> 
> The first patch looks like the standalone fix because it stops reading the
> user pointer. If a backport takes only that patch, could re-attach break
> for every TAP that has a filter?
> 
> Would it make sense to squash the two patches, or to note the dependency in
> both commit messages?
> 
>> +		rollback_filter = true;
>>  	}
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch. The TAP
> filter lifecycle seems to treat disabled queues differently on detach and
> on re-attach.
> 
> TUNDETACHFILTER only walks the active queues:
> 
> drivers/net/tun.c:__tun_chr_ioctl() {
>     ...
> 	case TUNDETACHFILTER:
> 		...
> 		tun_detach_filter(tun, tun->numqueues);
>     ...
> }
> 
> drivers/net/tun.c:tun_detach_filter() {
>     ...
> 	for (i = 0; i < n; i++) {
> 		tfile = rtnl_dereference(tun->tfiles[i]);
> 		lock_sock(tfile->socket.sk);
> 		sk_detach_filter(tfile->socket.sk);
> 		release_sock(tfile->socket.sk);
> 	}
>     ...
> }
> 
> For IFF_DETACH_QUEUE, __tun_detach() runs with clean == false. That path
> calls tun_disable_queue() and tun_napi_disable() but never
> sk_detach_filter(), so a tfile parked on tun->disabled keeps its
> sk_filter.
> 
> tun_attach() only touches the filter when tun->filter_attached is true. It
> never clears a leftover filter.
> 
> Consider this sequence on a multiqueue TAP:
> 
>   TUNATTACHFILTER           (queue Q is active and gets the filter)
>   IFF_DETACH_QUEUE on Q     (Q moves to tun->disabled, filter kept)
>   TUNDETACHFILTER           (Q skipped, filter_attached = false)
>   IFF_ATTACH_QUEUE on Q     (re-attach branch in tun_attach() skipped)
> 
> Would Q then be active with the old classic BPF filter still installed,
> even though the device no longer has a filter? If so, packets on Q would
> still be filtered or truncated.
> 
> The baseline behaves the same way here, so this patch does not change
> it.
> 

[PATCH net v3 3/3] selftests: net: add TAP socket filter attach tests

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

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

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

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

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

diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
index abe488bac50b..6f6023b16d1c 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,159 @@ TEST_F(tun, reattach_close_delete)
 	EXPECT_EQ(tun_delete(self->ifname), 0);
 }
 
+/* accept: return skb->len */
+static const struct sock_filter filter_accept[] = {
+	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
+	BPF_STMT(BPF_RET | BPF_A, 0),
+};
+
+/* 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: re-reading the user
+ * address used to fail with -EFAULT.
+ */
+TEST_F(tun, reattach_filter_without_user_buffer)
+{
+	struct sock_fprog gf = { 0 };
+	short flags = 0;
+	void *prog;
+	int ret;
+
+	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
+	ASSERT_NE(prog, NULL);
+	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
+
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+
+	/* The process that called TUNATTACHFILTER no longer maps this memory */
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
+
+	ret = tun_attach(self->fd, self->ifname);
+	EXPECT_EQ(ret, 0);
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
+	ASSERT_EQ(munmap(prog, getpagesize()), 0);
+}
+
+/* A new queue attached with IFF_NOFILTER must not get the filter that is
+ * configured on the device.
+ */
+TEST_F(tun, attach_filter_nofilter_flag)
+{
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	int fd;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_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 };
+	short flags = 0;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
+
+	errno = 0;
+	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	EXPECT_EQ(filter_get(self->fd, &gf), 0);
+	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
+
+	flags = tun_get_iff_flags(self->fd);
+	EXPECT_GE(flags, 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+}
+
 FIXTURE(tun_vnet_udptnl)
 {
 	char ifname[IFNAMSIZ];
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v3 3/3] selftests: net: add TAP socket filter attach tests

[Willem de Bruijn]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> reattach_filter_without_user_buffer() attaches a filter, makes the buffer
> the program was copied from unreadable and then attaches the queue again,
> which used to fail with -EFAULT.
> 
> attach_filter_bad_len_keeps_program() checks that a rejected
> TUNATTACHFILTER leaves both the saved program and the installed filter
> alone.
> 
> attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
> ways to attach a queue without a filter.
> 
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

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


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

iff a respin is needed, consider adding the extra test to make sure that
tun_attach did not just succeed without attaching the program, as it does
before patch 2.

EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0);

> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
> +
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
> +	ASSERT_EQ(munmap(prog, getpagesize()), 0);
> +}

Re: [PATCH net v3 3/3] selftests: net: add TAP socket filter attach tests

[Rongguang Wei]

on 2026/10/1 03:20, Willem de Bruijn wrote:
> Rongguang Wei wrote:
>> From: Rongguang Wei <weirongguang@kylinos.cn>
>>
>> reattach_filter_without_user_buffer() attaches a filter, makes the buffer
>> the program was copied from unreadable and then attaches the queue again,
>> which used to fail with -EFAULT.
>>
>> attach_filter_bad_len_keeps_program() checks that a rejected
>> TUNATTACHFILTER leaves both the saved program and the installed filter
>> alone.
>>
>> attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
>> ways to attach a queue without a filter.
>>
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> 
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
> 
>> +TEST_F(tun, reattach_filter_without_user_buffer)
>> +{
>> +	struct sock_fprog gf = { 0 };
>> +	short flags = 0;
>> +	void *prog;
>> +	int ret;
>> +
>> +	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
>> +	ASSERT_NE(prog, NULL);
>> +	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
>> +
>> +	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
>> +
>> +	/* The process that called TUNATTACHFILTER no longer maps this memory */
>> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
>> +
>> +	ret = tun_attach(self->fd, self->ifname);
>> +	EXPECT_EQ(ret, 0);
>> +
>> +	flags = tun_get_iff_flags(self->fd);
>> +	EXPECT_GE(flags, 0);
>> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
> 
> iff a respin is needed, consider adding the extra test to make sure that
> tun_attach did not just succeed without attaching the program, as it does
> before patch 2.
> 
> EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0);
> 
Yes, I will add it in V4. Thank you.
>> +
>> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
>> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
>> +
>> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
>> +	ASSERT_EQ(munmap(prog, getpagesize()), 0);
>> +}

Re: [PATCH net v3 3/3] 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] reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:603-631) never checks that…
- [Low] In attach_filter_bad_len_keeps_program()
  (tools/testing/selftests/net/tun.c:695-697), the final TUNGETIFF…
- [Low] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:614-627), only `ret == 0` actually…
- [Low] tun_copy_filter() (drivers/net/tun.c:730-752), added in commit
  "tun: keep a kernel copy of the socket filter program" within the…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests for TUNATTACHFILTER. They cover re-attaching
a queue after the user buffer holding the filter has been made unreadable,
attaching with IFF_NOFILTER, and re-attaching after TUNDETACHFILTER. They
also check that a TUNATTACHFILTER with a bad length leaves the saved
program alone.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50bb..6f6023b16d1cc 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 };
> +	short flags = 0;
> +	void *prog;
> +	int ret;
> +
> +	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
> +	ASSERT_NE(prog, NULL);
> +	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
> +
> +	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
> +
> +	/* The process that called TUNATTACHFILTER no longer maps this memory */
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
> +
> +	ret = tun_attach(self->fd, self->ifname);
> +	EXPECT_EQ(ret, 0);
> +
> +	flags = tun_get_iff_flags(self->fd);
> +	EXPECT_GE(flags, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);

[Severity: Low]
Would this test catch the bug fixed earlier in this series by "tun: fix
inverted error check when re-attaching the filter"?

Take a kernel that has the kernel copy patch but still has the old check
in tun_attach():

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

There, sk_attach_filter_kern() succeeds and tun_attach() returns 0 through
goto out. tun_enable_queue() never runs, tun->tfiles[] is not published,
numqueues is not incremented, and tfile->detached stays set.

Every assertion here still passes in that case:

- ret is 0.
- TUNGETIFF works because the non-clean __tun_detach() leaves tfile->tun
  set.
- IFF_NOFILTER is clear because sk_filter is still installed.
- gf.len reads the unchanged tun->fprog.

TUNGETIFF already reports the detached state:

	if (tfile->detached)
		ifr.ifr_flags |= IFF_DETACH_QUEUE;

Could EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0) be added here? That would
give the inverted check fix some regression coverage.

> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Low]
Do the IFF_NOFILTER and gf.len checks actually see whether the kernel copy
was installed again?

tun_detach() here uses IFF_DETACH_QUEUE, which goes through
__tun_detach(tfile, false). That path only does this:

	} else {
		tun_disable_queue(tun, tfile);
		tun_napi_disable(tfile);
	}

It never calls sk_detach_filter(), so the socket keeps the sk_filter from
the first TUNATTACHFILTER. That means IFF_NOFILTER stays clear whether or
not this block in tun_attach() ran again:

	if (!skip_filter && (tun->filter_attached == true)) {
		lock_sock(tfile->socket.sk);
		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);

TUNGETFILTER copies tun->fprog, and only tun_copy_filter() writes that. So
gf.len doesn't depend on the re-attach either.

If filter_attached were ignored, or the re-install block were dropped, the
ret == 0 check would be the only one still testing anything. The comment
above the test talks about a queue from a process that "does not
necessarily map the buffer", but the test never exercises that path.

Would it be more convincing to open a new queue after the
mprotect(PROT_NONE)? For example, open /dev/net/tun and call TUNSETIFF
without IFF_NOFILTER. That socket starts with no sk_filter, so a clear
IFF_NOFILTER would show that the kernel copy was installed.

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

[ ... ]

> +TEST_F(tun, attach_filter_bad_len_keeps_program)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
> +	EXPECT_EQ(errno, EINVAL);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
> +
> +	flags = tun_get_iff_flags(self->fd);
> +	EXPECT_GE(flags, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);

[Severity: Low]
Can this IFF_NOFILTER check detect the state this test is meant to
protect?

TUNGETIFF derives IFF_NOFILTER only from the per-socket filter:

	if (!tfile->socket.sk->sk_filter)
		ifr.ifr_flags |= IFF_NOFILTER;

self->fd is tfiles[0]. Some rejections happen before any socket is
touched: the len check in tun_copy_filter(), or a failure of the first
sk_attach_filter_kern() in tun_attach_filter() followed by
tun_detach_filter(tun, 0). Neither removes the sk_filter of self->fd.

With n == 0, tun_detach_filter() touches no socket, but it still does
this:

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

That state only matters for a queue attached later, and the test never
attaches another queue. TUNGETFILTER reads tun->fprog, not
tun->fprog_kern, so gf.len doesn't cover it either.

On the pre-series baseline, len == 0 overwrote tun->fprog and cleared
filter_attached, and this IFF_NOFILTER assertion still passed there. Only
the gf.len check caught it.

A failure on a later queue does detach tfiles[0] and the queues before it.
The rejections this test exercises can't reach that case, though.

Would re-attaching a queue, or opening a new one, after the failed ioctl
and then checking IFF_NOFILTER show that filter_attached and fprog_kern
were preserved?

> +}

[ ... ]

[Severity: Low]
One more question about the series. This one is about tun_copy_filter(),
added by "tun: keep a kernel copy of the socket filter program" rather
than by this patch:

drivers/net/tun.c:tun_copy_filter() {
	...
	insns = memdup_array_user(fprog.filter, fprog.len,
				  sizeof(struct sock_filter));
	...
}

Should this copy be charged to the caller's memory cgroup?

memdup_array_user() goes through memdup_user(), which allocates with
GFP_USER | __GFP_NOWARN and no __GFP_ACCOUNT. The buffer can be up to
BPF_MAXINSNS * sizeof(struct sock_filter), which is 32 KiB. It is freed
only by tun_detach_filter() or tun_free_netdev(), so it can live as long
as the device.

The netdev and bpf_prog allocations are memcg accounted, but this one is
not. A user with CAP_NET_ADMIN in a user namespace could create many
persistent TAP devices, and each one could pin up to 32 KiB of unaccounted
slab.

Would kmalloc_array(..., GFP_KERNEL_ACCOUNT) plus copy_from_user() close
the gap? There is precedent for unaccounted classic filter copies in
net/core, since bpf_prog_store_orig_filter() uses GFP_KERNEL, so this is
only a minor inconsistency.

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

Re: [PATCH net v3 0/3] tun: fix re-attaching the socket filter

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.