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

[netdev-bot+sashiko@kernel.org]

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