Re: [PATCH v2 3/4] tun: keep a kernel copy of the socket filter program

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] This commit carries `Fixes: 54f968d6efdb`, so it will be picked
  for stable.
- [Low] For TUNATTACHFILTER, tun_copy_filter() replaces the core
  bpf_check_basics_ok() validation with its own checks, and the two…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a failed
  filter replacement leaves the queues in mixed states.
- [Medium] This problem wasn't introduced by this patch, but
  TUNDETACHFILTER only detaches the filter from the tun->numqueues…
- [Low] This problem wasn't introduced by this patch, but when every
  queue of a multiqueue TAP is disabled with IFF_DETACH_QUEUE,…

--

tun: keep a kernel copy of the socket filter program

With this commit, TUNATTACHFILTER copies the classic BPF instructions
into tun->fprog_kern instead of keeping only the user pointer in
tun->fprog. tun_attach() and tun_attach_filter() then build each queue's
filter from that kernel copy through sk_attach_filter_kern(). The copy
is freed on detach, when it is replaced, and in tun_free_netdev().

Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/ (local)

[ ... ]

[Severity: Low]
Does this change the errno TUNATTACHFILTER returns when fprog.filter is
NULL and fprog.len is nonzero?

Before this patch, sk_attach_filter()->__get_filter()->bpf_check_basics_ok()
rejected a NULL filter with -EINVAL, the same errno SO_ATTACH_FILTER
returns. Here only the length is checked. memdup_array_user(NULL, ...)
then fails in copy_from_user(), and the ioctl returns -EFAULT.

The bpf_check_basics_ok() call inside bpf_prog_create() only sees the
kernel copy, which is never NULL, so it can't catch this case.

Would adding a !fprog.filter test to the length check keep the old
-EINVAL?

[Severity: Low]
Should this commit record that it depends on the helper commit? It
carries Fixes: 54f968d6efdb, so it will be picked for stable.

sk_attach_filter_kern() is only added by the previous commit in the
series, "net: filter: add sk_attach_filter_kern() function". That commit
has no Fixes: tag, no Cc: stable and no stable dependency annotation.

This commit's message doesn't say it depends on that commit, or on "tun:
fix inverted error check when re-attaching the filter".

If it is backported by itself, tun_attach() and tun_attach_filter() won't
build. sk_attach_filter_kern is implicitly declared, and the symbol is
undefined when tun is built as a module.

Would a Cc: stable tag with a dependency note on the helper commit
prevent that?

 		release_sock(tfile->socket.sk);
 		if (err)
 			goto out;

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but does TUNDETACHFILTER leave
the filter on queues that are disabled at that point?

tun_detach_filter() only walks the first n == tun->numqueues entries:

	for (i = 0; i < n; i++) {
		tfile = rtnl_dereference(tun->tfiles[i]);
		lock_sock(tfile->socket.sk);
		sk_detach_filter(tfile->socket.sk);
		...

A queue disabled through TUNSETQUEUE with IFF_DETACH_QUEUE
(tun_set_queue()->__tun_detach(tfile, false)) keeps its sk_filter.

When that queue is re-enabled with IFF_ATTACH_QUEUE, tun_attach() skips
the filter block because filter_attached is false. It doesn't clear the
leftover filter either.

tun_net_xmit() checks tfile->socket.sk->sk_filter directly. So after
TUNDETACHFILTER succeeds, the re-enabled queue still applies the old
filter.

[Severity: Medium]
This is a pre-existing issue, but can a failed filter replacement leave
the queues in mixed states?

Say filter A is attached and TUNATTACHFILTER is issued with program B.
tun_copy_filter() frees A's kernel copy and stores B. tun_attach_filter()
then fails on queue i. That happens if bpf_check_classic() rejects B
(i == 0), or if bpf_prog_alloc() or __sk_filter_charge() fails at any i.

tun_detach_filter(tun, i) detaches only queues [0, i). It also frees B's
copy and sets filter_attached = false.

Queues [i, numqueues) keep running filter A, but the device-level state
says no filter is attached. Queues attached later through tun_attach()
get no filter at all.

The code behaved the same way before this patch.

[Severity: Low]
This isn't a bug introduced by this patch, but what happens when every
queue of a multiqueue TAP has been disabled with IFF_DETACH_QUEUE?

tun->numqueues is 0 then, but tfile->tun is still set, so tun_get() still
reaches TUNATTACHFILTER.

tun_copy_filter() checks only the length and copies the instructions.
The loop in tun_attach_filter() runs zero times, so filter_attached is
set to true and 0 is returned. bpf_check_classic() never runs.

A later IFF_ATTACH_QUEUE calls sk_attach_filter_kern() from tun_attach().
bpf_prepare_filter() then rejects the saved program with -EINVAL. The
queue can't be re-enabled until the owner issues TUNDETACHFILTER or a
valid TUNATTACHFILTER.

This patch narrows the problem a little: before its length check, this
path accepted even len == 0.

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