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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] A TUNATTACHFILTER that fails partway through leaves queues
  0..i-1 with no filter, while the device state still says the old…
- [Medium] With zero active queues, TUNATTACHFILTER saves a program that
  has never been built, so a program that bpf_prog_create() rejects is…

Pre-existing issues:
- [Low] TUNDETACHFILTER does not remove the filter from queues detached
  with IFF_DETACH_QUEUE.

--

tun: keep a kernel copy of the socket filter program

This keeps a kernel copy of the classic BPF program attached with
TUNATTACHFILTER in tun->fprog_kern, so that each queue builds its own
program with the new sk_attach_filter_kern(). It also fixes the inverted
error check in tun_attach(). Programs are now validated up front with the
new bpf_prog_validate_classic(), and the saved program is replaced only
after every queue has accepted the new one.

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

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 439565a264ee7..c121ad50f31bb 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -3070,10 +3141,14 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
>  		release_sock(tfile->socket.sk);
>  	}
>  
> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.filter = NULL;
> +	tun->fprog_kern.len = 0;
>  	tun->filter_attached = false;
>  }

[Severity: Low]
This isn't a bug introduced by this patch, but does TUNDETACHFILTER miss
queues that were detached with IFF_DETACH_QUEUE?

tun_detach_filter() only walks tun->tfiles[0..n-1], which are the active
queues, and then clears filter_attached. Queues on tun->disabled keep
their sk_filter, because __tun_detach(tfile, false) only calls
tun_disable_queue().

When such a queue comes back with IFF_ATTACH_QUEUE, tun_attach() only
touches the filter when filter_attached is true:

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

Wouldn't the re-attached queue then keep running the program that was
removed? TUNGETIFF would also not report IFF_NOFILTER for that queue.

>  
> -static int tun_attach_filter(struct tun_struct *tun)
> +static int tun_attach_filter(struct tun_struct *tun,
> +			     struct sock_fprog_kern *fprog_kern)
>  {
>  	int i, ret = 0;
>  	struct tun_file *tfile;
> @@ -3081,15 +3156,29 @@ static int tun_attach_filter(struct tun_struct *tun)
>  	for (i = 0; i < tun->numqueues; i++) {
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		lock_sock(tfile->socket.sk);
> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		ret = sk_attach_filter_kern(fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (ret) {
> -			tun_detach_filter(tun, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out_detach;
>  	}
>  
>  	tun->filter_attached = true;
> +	return 0;

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

You can detach every queue of a multiqueue TAP with TUNSETQUEUE and
IFF_DETACH_QUEUE. __tun_detach(tfile, false) leaves tfile->tun set, so
TUNATTACHFILTER can still be issued on a disabled queue's fd.

In that case the loop never calls sk_attach_filter_kern(), so
bpf_prog_create() never sees the program. tun_copy_filter() only ran
bpf_prog_validate_classic(). That skips the s16 jump offset check in
bpf_convert_filter() and the runtime selection.

tun_attach_filter() still sets filter_attached and returns 0. The ioctl
then frees the previously saved program and commits the unbuilt one:

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

Here is an example. Take a 4096 instruction program made of JA k=4094,
then 4094 x LDX|B|MSH k=0, then RET|K. It passes bpf_check_classic(). Each
LDX|B|MSH expands to roughly 18 eBPF instructions, so the converted JA
offset is far above S16_MAX, and bpf_convert_filter() returns -EINVAL.

Now that the inverted check in tun_attach() is fixed, wouldn't every
later IFF_ATTACH_QUEUE fail in sk_attach_filter_kern(&tun->fprog_kern,
...) until TUNDETACHFILTER is issued? The same goes for every TUNSETIFF
without IFF_NOFILTER.

The commit message says:

  Such a late failure does not replace the saved program: the copy is
  committed to tun->fprog and tun->fprog_kern only after
  tun_attach_filter() installed it on every queue.

That guarantee doesn't seem to hold when there are no active queues.

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

[Severity: Medium]
Can this rollback leave the device in a self-contradictory state?

Suppose sk_attach_filter_kern() succeeds on queues 0..i-1 and then fails
on queue i. Each successful call already went through __sk_attach_prog(),
which released the old filter F through sk_filter_uncharge(). This loop
then clears sk_filter on queues 0..i-1, while queues i..n-1 keep F.

Before this patch, tun_detach_filter() ran here and cleared
filter_attached. Now filter_attached stays true, and the ioctl leaves
tun->fprog and tun->fprog_kern set to F. TUNGETFILTER reports F, and
tun_attach() installs F on queues attached later.

Queues 0..i-1, however, run no filter at all. tun_net_xmit() only filters
when sk_filter is set:

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

TUNGETIFF also reports IFF_NOFILTER for those queues.

A bad program isn't needed to get here. Once validation has passed, queue
i can still fail with -ENOMEM from bpf_prog_alloc() (memcg charged) or
from kmalloc_obj() in __sk_attach_prog(). It can also fail when
__sk_filter_charge() would exceed optmem_max.

The comment here and the commit message say restoring the previous
program "would need an extra reference per queue". Since F's
instructions are still in tun->fprog_kern, couldn't the rollback rebuild
F with sk_attach_filter_kern(&tun->fprog_kern, sk) instead?

Another option is to build and charge all the new programs before
swapping any of them in, which would avoid this fail-open window.

> +
>  	return ret;
>  }

[ ... ]

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