Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

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] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:614-641), only the tun_attach()…
- [Low] attach_filter_bad_len_keeps_descriptor() only checks "the
  installed filter" through IFF_NOFILTER on self->fd…
- [Low] attach_filter_nofilter_flag()
  (tools/testing/selftests/net/tun.c:646-665) only checks that a queue…
- [Low] In reattach_filter_without_user_buffer() the inline comment at
  tools/testing/selftests/net/tun.c:627 says "The process that called…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests to tools/testing/selftests/net/tun.c for
TUNATTACHFILTER handling. They cover four cases:

- re-attaching a queue after the user buffer that held the filter is
  made unreadable
- attaching a queue with IFF_NOFILTER
- attaching a queue after TUNDETACHFILTER
- rejecting a zero-length filter

[ ... ]

[ ... ]

[Severity: Low]
This isn't a bug, but is this comment accurate? mprotect(PROT_NONE) makes
the page unreadable, but it stays mapped. It is only unmapped at the end of
the test by munmap().

The header comment above the test says the mapping is "made unreadable",
which matches the code.

copy_from_user() faults the same way in both cases, so the test still works.

[Severity: Low]
Can these last two checks tell whether tun_attach() actually re-installed
the filter?

The tun_detach() helper uses TUNSETQUEUE with IFF_DETACH_QUEUE, which ends
up in __tun_detach(tfile, false). That path disables the queue but never
calls sk_detach_filter(). The socket therefore keeps the filter from the
first TUNATTACHFILTER:

drivers/net/tun.c:__tun_detach() {
    ...
		} else {
			tun_disable_queue(tun, tfile);
			tun_napi_disable(tfile);
		}
    ...
}

TUNGETIFF reports IFF_NOFILTER only when sk_filter is NULL:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
		if (!tfile->socket.sk->sk_filter)
			ifr.ifr_flags |= IFF_NOFILTER;
    ...
}

So the IFF_NOFILTER check passes whether or not this block in tun_attach()
ran:

drivers/net/tun.c:tun_attach() {
    ...
	if (!skip_filter && (tun->filter_attached == true)) {
		lock_sock(tfile->socket.sk);
		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
    ...
}

TUNGETFILTER returns tun->fprog, which is the saved user descriptor, not
fprog_kern:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
	case TUNGETFILTER:
    ...
		if (copy_to_user(argp, &tun->fprog, sizeof(tun->fprog)))
    ...
}

So gf.len == 3 doesn't show which instructions the queue is running.

Only the ret == 0 check can tell a fixed kernel from a broken one. If the
re-install block were skipped, or filter_attached were wrongly false, the
rest of the test would still pass.

The comment describes a queue "attached long after the filter was
configured". The test, however, reuses a queue that already has a filter.

Could the test also attach a new queue with TUNSETIFF and without
IFF_NOFILTER? On that queue, fprog_kern is the only source of the filter.

It could also overwrite the still-readable buffer, for example with
filter_drop. That would cover the other hazard described in "tun: keep a
kernel copy of the socket filter program".

[Severity: Low]
Is there a positive control for this test? tun_set_iff() passes the
IFF_NOFILTER bit to tun_attach() as skip_filter:

drivers/net/tun.c:tun_set_iff() {
    ...
		err = tun_attach(tun, file, ifr->ifr_flags & IFF_NOFILTER,
    ...
}

None of the new tests opens a new queue with TUNSETIFF and without
IFF_NOFILTER after TUNATTACHFILTER, then checks that the device filter is
installed on it. Both fixture queues are created in FIXTURE_SETUP, before
any filter exists.

Suppose filter_attached were never set, or the re-install branch in
tun_attach() never ran. The new socket here would have no filter, and this
check would still pass.

Would it help to also open a queue without IFF_NOFILTER and check that
IFF_NOFILTER is clear on it?

[ ... ]

[Severity: Low]
The commit message says this test checks that a rejected TUNATTACHFILTER
"leaves both the saved descriptor and the installed filter alone". Can this
IFF_NOFILTER check detect a lost filter?

The check reads sk_filter on self->fd. A failed attach on queue 0 never
touches sk_filter, even on the kernel before this series.

In the old code, len=0 was copied into tun->fprog. tun_attach_filter() then
failed at i=0. tun_detach_filter(tun, 0) detached no sockets, but it still
cleared the device state:

drivers/net/tun.c:tun_detach_filter() {
    ...
	tun->filter_attached = false;
}

The socket kept its filter, so this check passed even though the device had
silently lost its re-attach state. On the old kernel, only the gf.len check
fails.

tun_attach() reads tun->filter_attached and tun->fprog_kern on later
attaches. The test never attaches a queue again after the failed ioctl.

A regression could clear filter_attached or free fprog_kern before the
length check while keeping tun->fprog. That regression would pass this test.

Could the test detach and re-attach the queue (or add a new one) after the
failed TUNATTACHFILTER, then check IFF_NOFILTER? That would show the
installed filter is really kept.

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