[PATCH net v1] tun: fix inverted error check when re-attaching the filter

[PATCH net v1] tun: fix inverted error check when re-attaching the filter

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

tun_attach() return when sk_attach_filter() succeeds instead of when it
fails, so the queue is never published in tun->tfiles[] while TUNSETIFF
still reports success.

Invert the check; nothing is published at that point, so the attach can be
aborted cleanly.

Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68a..09720883b9a1 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -754,7 +754,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 		lock_sock(tfile->socket.sk);
 		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
 		release_sock(tfile->socket.sk);
-		if (!err)
+		if (err)
 			goto out;
 	}
 
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH net v1] tun: fix inverted error check when re-attaching the filter

[netdev-bot+sashiko]

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

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

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

- [Medium] The re-attach path that this patch enables rebuilds the queue
  filter from a stale userspace pointer, and reads it in the wrong…
- [Low] With the check inverted, a successful sk_attach_filter() in
  tun_attach() (drivers/net/tun.c:755) no longer returns early.

--

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

This patch inverts the error check after sk_attach_filter() in
tun_attach(). A successful filter re-attach now continues on to publish
the queue, and a failed one aborts the attach.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..09720883b9a10 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -754,7 +754,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  		lock_sock(tfile->socket.sk);
>  		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (!err)
> +		if (err)
>  			goto out;
>  	}
>  

[Severity: Low]
A successful sk_attach_filter() now falls through. If a later step in
tun_attach() fails, can the filter stay attached to a socket that was
never published?

After this point, tun_attach() can still fail here:

	if (!tfile->detached &&
	    ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
			    GFP_KERNEL, tun_ptr_free)) {
		err = -ENOMEM;
		goto out;
	}

It can also fail in xdp_rxq_info_reg() or xdp_rxq_info_reg_mem_model().
None of these error paths call sk_detach_filter(tfile->socket.sk).

By comparison, tun_attach_filter() rolls back when it fails:

		if (ret) {
			tun_detach_filter(tun, i);
			return ret;
		}

The fd can then be reused. One way is to retry TUNSETIFF with
IFF_NOFILTER, which sets skip_filter and skips the re-attach block.
Another is to attach to a different device that has no filter. Neither
clears sk_filter, since only tun_detach_filter() calls sk_detach_filter().

tun_net_xmit() would then apply the leftover filter to packets:

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

TUNGETIFF would also leave IFF_NOFILTER unset, even though it was
requested.

The commit message says "nothing is published at that point, so the
attach can be aborted cleanly". Is that only true when sk_attach_filter()
itself is the step that fails?

[Severity: Medium]
This path now installs the rebuilt filter on a live queue. Is
tun->fprog still safe to reuse at this point?

TUNATTACHFILTER in __tun_chr_ioctl() copies only the sock_fprog header:

		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
			break;

That leaves tun->fprog.filter as a __user pointer from the process that
issued the ioctl. On a persistent device, tun->filter_attached stays true
after all queues are closed.

Later calls reach the re-attach through this path. That includes
TUNSETIFF without IFF_NOFILTER, and TUNSETQUEUE with IFF_ATTACH_QUEUE,
which always passes skip_filter = false:

tun_set_iff() / tun_set_queue()
  tun_attach()
    sk_attach_filter(&tun->fprog, sk)
      __get_filter()
        copy_from_user(prog->insns, fprog->filter, fsize)

That read goes through current->mm. The caller may be a different process
from the one that set the filter. It may also be the same process after
the instruction buffer was freed or remapped.

Before this patch, the rebuilt filter never ended up on a working queue.
When the re-attach succeeded, the queue was not published. When it failed,
the queue was published without a filter and TUNSETIFF returned 0.

With the check inverted, there are two cases.

If the address is not mapped in the caller, TUNSETIFF and TUNSETQUEUE
now fail with -EFAULT. If the bytes there are not valid classic BPF, they
fail with -EINVAL. Does this mean a new opener of a persistent TAP with a
filter can no longer attach unless it passes IFF_NOFILTER?

If the address happens to be mapped, whatever bytes it holds become the
queue's filter, not the program the administrator installed. On a
multiqueue TAP, could different queues end up with different filters?

For this re-attach to work correctly, would it need a kernel-owned copy?
That could be a saved copy of the instructions, or a refcounted struct
bpf_prog or sk_filter.

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

Re: [PATCH net v1] tun: fix inverted error check when re-attaching the filter

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The re-attach path that this patch enables rebuilds the queue
>   filter from a stale userspace pointer, and reads it in the wrong…
> - [Low] With the check inverted, a successful sk_attach_filter() in
>   tun_attach() (drivers/net/tun.c:755) no longer returns early.
> 
> --
> 
> tun: fix inverted error check when re-attaching the filter
> 
> This patch inverts the error check after sk_attach_filter() in
> tun_attach(). A successful filter re-attach now continues on to publish
> the queue, and a failed one aborts the attach.
> 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 5a302709a68aa..09720883b9a10 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -754,7 +754,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
> >  		lock_sock(tfile->socket.sk);
> >  		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> >  		release_sock(tfile->socket.sk);
> > -		if (!err)
> > +		if (err)
> >  			goto out;
> >  	}
> >  
> 
> [Severity: Low]
> A successful sk_attach_filter() now falls through. If a later step in
> tun_attach() fails, can the filter stay attached to a socket that was
> never published?
> 
> After this point, tun_attach() can still fail here:
> 
> 	if (!tfile->detached &&
> 	    ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
> 			    GFP_KERNEL, tun_ptr_free)) {
> 		err = -ENOMEM;
> 		goto out;
> 	}
> 
> It can also fail in xdp_rxq_info_reg() or xdp_rxq_info_reg_mem_model().
> None of these error paths call sk_detach_filter(tfile->socket.sk).

This is a real point.

This patch fixes the intent of the original patch by inverting the
check.

But the state change should also be reverted if tun_attach fails later
on. The only similar example is xdp_rxq_info_reg further down, which
does get reverted if xdp_rxq_info_reg_mem_model fails.
 
> By comparison, tun_attach_filter() rolls back when it fails:
> 
> 		if (ret) {
> 			tun_detach_filter(tun, i);
> 			return ret;
> 		}
> 
> The fd can then be reused. One way is to retry TUNSETIFF with
> IFF_NOFILTER, which sets skip_filter and skips the re-attach block.
> Another is to attach to a different device that has no filter. Neither
> clears sk_filter, since only tun_detach_filter() calls sk_detach_filter().
> 
> tun_net_xmit() would then apply the leftover filter to packets:
> 
> 	if (tfile->socket.sk->sk_filter) {
> 		drop_reason = sk_filter_reason(tfile->socket.sk, skb);
> 
> TUNGETIFF would also leave IFF_NOFILTER unset, even though it was
> requested.
> 
> The commit message says "nothing is published at that point, so the
> attach can be aborted cleanly". Is that only true when sk_attach_filter()
> itself is the step that fails?
> 
> [Severity: Medium]
> This path now installs the rebuilt filter on a live queue. Is
> tun->fprog still safe to reuse at this point?
> 
> TUNATTACHFILTER in __tun_chr_ioctl() copies only the sock_fprog header:
> 
> 		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
> 			break;
> 
> That leaves tun->fprog.filter as a __user pointer from the process that
> issued the ioctl. On a persistent device, tun->filter_attached stays true
> after all queues are closed.
> 
> Later calls reach the re-attach through this path. That includes
> TUNSETIFF without IFF_NOFILTER, and TUNSETQUEUE with IFF_ATTACH_QUEUE,
> which always passes skip_filter = false:
> 
> tun_set_iff() / tun_set_queue()
>   tun_attach()
>     sk_attach_filter(&tun->fprog, sk)
>       __get_filter()
>         copy_from_user(prog->insns, fprog->filter, fsize)
> 
> That read goes through current->mm. The caller may be a different process
> from the one that set the filter. It may also be the same process after
> the instruction buffer was freed or remapped.

This is a very good point.

It is a separate bug from this one, and should be a separate fix.

The BPF program should be copied into the kernel immediately if it
can be attached again later.

But, as the below text explains, flipping the test may now fail
TUNSETIFF because sk_filter_attach fails, where previously it would
continue since sk_attach_filter error is expected (even if it did
complete without the filter installed as intended). Ideally, both
fixes are sent together, but I won't hold anyone to that.

> 
> Before this patch, the rebuilt filter never ended up on a working queue.
> When the re-attach succeeded, the queue was not published. When it failed,
> the queue was published without a filter and TUNSETIFF returned 0.
> 
> With the check inverted, there are two cases.
> 
> If the address is not mapped in the caller, TUNSETIFF and TUNSETQUEUE
> now fail with -EFAULT. If the bytes there are not valid classic BPF, they
> fail with -EINVAL. Does this mean a new opener of a persistent TAP with a
> filter can no longer attach unless it passes IFF_NOFILTER?
> 
> If the address happens to be mapped, whatever bytes it holds become the
> queue's filter, not the program the administrator installed. On a
> multiqueue TAP, could different queues end up with different filters?
> 
> For this re-attach to work correctly, would it need a kernel-owned copy?
> That could be a saved copy of the instructions, or a refcounted struct
> bpf_prog or sk_filter.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923025653.59348-1-clementwei90%40163.com

Re: [PATCH net v1] tun: fix inverted error check when re-attaching the filter

[Rongguang Wei]

on 2026/9/26 01:15, Willem de Bruijn wrote:
> netdev-bot+sashiko@ wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential
>> issue(s) to consider.
>>
>> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>>
>> If you believe none of the issues are legitimate, please reply to each
>> issue of Medium severity or higher and explain why the AI is wrong.
>> If any of them are legitimate and you plan to respin, please let patchwork
>> know by including "pw-bot: cr" as a separate line at the end of your reply
>> (one such reply per series is enough).
>>
>> - [Medium] The re-attach path that this patch enables rebuilds the queue
>>   filter from a stale userspace pointer, and reads it in the wrong…
>> - [Low] With the check inverted, a successful sk_attach_filter() in
>>   tun_attach() (drivers/net/tun.c:755) no longer returns early.
>>
>> --
>>
>> tun: fix inverted error check when re-attaching the filter
>>
>> This patch inverts the error check after sk_attach_filter() in
>> tun_attach(). A successful filter re-attach now continues on to publish
>> the queue, and a failed one aborts the attach.
>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index 5a302709a68aa..09720883b9a10 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -754,7 +754,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>>>  		lock_sock(tfile->socket.sk);
>>>  		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>>>  		release_sock(tfile->socket.sk);
>>> -		if (!err)
>>> +		if (err)
>>>  			goto out;
>>>  	}
>>>  
>>
>> [Severity: Low]
>> A successful sk_attach_filter() now falls through. If a later step in
>> tun_attach() fails, can the filter stay attached to a socket that was
>> never published?
>>
>> After this point, tun_attach() can still fail here:
>>
>> 	if (!tfile->detached &&
>> 	    ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
>> 			    GFP_KERNEL, tun_ptr_free)) {
>> 		err = -ENOMEM;
>> 		goto out;
>> 	}
>>
>> It can also fail in xdp_rxq_info_reg() or xdp_rxq_info_reg_mem_model().
>> None of these error paths call sk_detach_filter(tfile->socket.sk).
> 
> This is a real point.
> 
> This patch fixes the intent of the original patch by inverting the
> check.
> 
> But the state change should also be reverted if tun_attach fails later
> on. The only similar example is xdp_rxq_info_reg further down, which
> does get reverted if xdp_rxq_info_reg_mem_model fails.
>  
>> By comparison, tun_attach_filter() rolls back when it fails:
>>
>> 		if (ret) {
>> 			tun_detach_filter(tun, i);
>> 			return ret;
>> 		}
>>
>> The fd can then be reused. One way is to retry TUNSETIFF with
>> IFF_NOFILTER, which sets skip_filter and skips the re-attach block.
>> Another is to attach to a different device that has no filter. Neither
>> clears sk_filter, since only tun_detach_filter() calls sk_detach_filter().
>>
>> tun_net_xmit() would then apply the leftover filter to packets:
>>
>> 	if (tfile->socket.sk->sk_filter) {
>> 		drop_reason = sk_filter_reason(tfile->socket.sk, skb);
>>
>> TUNGETIFF would also leave IFF_NOFILTER unset, even though it was
>> requested.
Thank you, v2 will add the missing rollback and detach the filter again if a
later step of tun_attach() fails.
>>
>> The commit message says "nothing is published at that point, so the
>> attach can be aborted cleanly". Is that only true when sk_attach_filter()
>> itself is the step that fails?
You are right and I will drop that sentence and describe the rollback instead.
>>
>> [Severity: Medium]
>> This path now installs the rebuilt filter on a live queue. Is
>> tun->fprog still safe to reuse at this point?
>>
>> TUNATTACHFILTER in __tun_chr_ioctl() copies only the sock_fprog header:
>>
>> 		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
>> 			break;
>>
>> That leaves tun->fprog.filter as a __user pointer from the process that
>> issued the ioctl. On a persistent device, tun->filter_attached stays true
>> after all queues are closed.
>>
>> Later calls reach the re-attach through this path. That includes
>> TUNSETIFF without IFF_NOFILTER, and TUNSETQUEUE with IFF_ATTACH_QUEUE,
>> which always passes skip_filter = false:
>>
>> tun_set_iff() / tun_set_queue()
>>   tun_attach()
>>     sk_attach_filter(&tun->fprog, sk)
>>       __get_filter()
>>         copy_from_user(prog->insns, fprog->filter, fsize)
>>
>> That read goes through current->mm. The caller may be a different process
>> from the one that set the filter. It may also be the same process after
>> the instruction buffer was freed or remapped.
> 
> This is a very good point.
> 
> It is a separate bug from this one, and should be a separate fix.
> 
> The BPF program should be copied into the kernel immediately if it
> can be attached again later.
> 
> But, as the below text explains, flipping the test may now fail
> TUNSETIFF because sk_filter_attach fails, where previously it would
> continue since sk_attach_filter error is expected (even if it did
> complete without the filter installed as intended). Ideally, both
> fixes are sent together, but I won't hold anyone to that.
>I intend to send it as a second patch in the series.
>>
>> Before this patch, the rebuilt filter never ended up on a working queue.
>> When the re-attach succeeded, the queue was not published. When it failed,
>> the queue was published without a filter and TUNSETIFF returned 0.
>>
>> With the check inverted, there are two cases.
>>
>> If the address is not mapped in the caller, TUNSETIFF and TUNSETQUEUE
>> now fail with -EFAULT. If the bytes there are not valid classic BPF, they
>> fail with -EINVAL. Does this mean a new opener of a persistent TAP with a
>> filter can no longer attach unless it passes IFF_NOFILTER?
>>
>> If the address happens to be mapped, whatever bytes it holds become the
>> queue's filter, not the program the administrator installed. On a
>> multiqueue TAP, could different queues end up with different filters?
>>
>> For this re-attach to work correctly, would it need a kernel-owned copy?
>> That could be a saved copy of the instructions, or a refcounted struct
>> bpf_prog or sk_filter.>> -- 
>> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923025653.59348-1-clementwei90%40163.com
> 
pw-bot: cr