Re: [PATCH net v3 3/3] selftests: net: add TAP socket filter attach tests

[netdev-bot+sashiko@kernel.org]

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] reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:603-631) never checks that…
- [Low] In attach_filter_bad_len_keeps_program()
  (tools/testing/selftests/net/tun.c:695-697), the final TUNGETIFF…
- [Low] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c:614-627), only `ret == 0` actually…
- [Low] tun_copy_filter() (drivers/net/tun.c:730-752), added in commit
  "tun: keep a kernel copy of the socket filter program" within the…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests for TUNATTACHFILTER. They cover re-attaching
a queue after the user buffer holding the filter has been made unreadable,
attaching with IFF_NOFILTER, and re-attaching after TUNDETACHFILTER. They
also check that a TUNATTACHFILTER with a bad length leaves the saved
program alone.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50bb..6f6023b16d1cc 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> +TEST_F(tun, reattach_filter_without_user_buffer)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +	void *prog;
> +	int ret;
> +
> +	prog = filter_alloc(filter_accept, ARRAY_SIZE(filter_accept));
> +	ASSERT_NE(prog, NULL);
> +	ASSERT_EQ(filter_attach(self->fd, prog, ARRAY_SIZE(filter_accept)), 0);
> +
> +	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
> +
> +	/* The process that called TUNATTACHFILTER no longer maps this memory */
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_NONE), 0);
> +
> +	ret = tun_attach(self->fd, self->ifname);
> +	EXPECT_EQ(ret, 0);
> +
> +	flags = tun_get_iff_flags(self->fd);
> +	EXPECT_GE(flags, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);

[Severity: Low]
Would this test catch the bug fixed earlier in this series by "tun: fix
inverted error check when re-attaching the filter"?

Take a kernel that has the kernel copy patch but still has the old check
in tun_attach():

	err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
	...
	if (!err)
		goto out;

There, sk_attach_filter_kern() succeeds and tun_attach() returns 0 through
goto out. tun_enable_queue() never runs, tun->tfiles[] is not published,
numqueues is not incremented, and tfile->detached stays set.

Every assertion here still passes in that case:

- ret is 0.
- TUNGETIFF works because the non-clean __tun_detach() leaves tfile->tun
  set.
- IFF_NOFILTER is clear because sk_filter is still installed.
- gf.len reads the unchanged tun->fprog.

TUNGETIFF already reports the detached state:

	if (tfile->detached)
		ifr.ifr_flags |= IFF_DETACH_QUEUE;

Could EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0) be added here? That would
give the inverted check fix some regression coverage.

> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Low]
Do the IFF_NOFILTER and gf.len checks actually see whether the kernel copy
was installed again?

tun_detach() here uses IFF_DETACH_QUEUE, which goes through
__tun_detach(tfile, false). That path only does this:

	} else {
		tun_disable_queue(tun, tfile);
		tun_napi_disable(tfile);
	}

It never calls sk_detach_filter(), so the socket keeps the sk_filter from
the first TUNATTACHFILTER. That means IFF_NOFILTER stays clear whether or
not this block in tun_attach() ran again:

	if (!skip_filter && (tun->filter_attached == true)) {
		lock_sock(tfile->socket.sk);
		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);

TUNGETFILTER copies tun->fprog, and only tun_copy_filter() writes that. So
gf.len doesn't depend on the re-attach either.

If filter_attached were ignored, or the re-install block were dropped, the
ret == 0 check would be the only one still testing anything. The comment
above the test talks about a queue from a process that "does not
necessarily map the buffer", but the test never exercises that path.

Would it be more convincing to open a new queue after the
mprotect(PROT_NONE)? For example, open /dev/net/tun and call TUNSETIFF
without IFF_NOFILTER. That socket starts with no sk_filter, so a clear
IFF_NOFILTER would show that the kernel copy was installed.

> +
> +	ASSERT_EQ(mprotect(prog, getpagesize(), PROT_READ | PROT_WRITE), 0);
> +	ASSERT_EQ(munmap(prog, getpagesize()), 0);
> +}

[ ... ]

> +TEST_F(tun, attach_filter_bad_len_keeps_program)
> +{
> +	struct sock_fprog gf = { 0 };
> +	short flags = 0;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept, ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, filter_accept, 0), -1);
> +	EXPECT_EQ(errno, EINVAL);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));
> +
> +	flags = tun_get_iff_flags(self->fd);
> +	EXPECT_GE(flags, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);

[Severity: Low]
Can this IFF_NOFILTER check detect the state this test is meant to
protect?

TUNGETIFF derives IFF_NOFILTER only from the per-socket filter:

	if (!tfile->socket.sk->sk_filter)
		ifr.ifr_flags |= IFF_NOFILTER;

self->fd is tfiles[0]. Some rejections happen before any socket is
touched: the len check in tun_copy_filter(), or a failure of the first
sk_attach_filter_kern() in tun_attach_filter() followed by
tun_detach_filter(tun, 0). Neither removes the sk_filter of self->fd.

With n == 0, tun_detach_filter() touches no socket, but it still does
this:

	kfree(tun->fprog_kern.filter);
	tun->fprog_kern.filter = NULL;
	tun->fprog_kern.len = 0;
	tun->filter_attached = false;

That state only matters for a queue attached later, and the test never
attaches another queue. TUNGETFILTER reads tun->fprog, not
tun->fprog_kern, so gf.len doesn't cover it either.

On the pre-series baseline, len == 0 overwrote tun->fprog and cleared
filter_attached, and this IFF_NOFILTER assertion still passed there. Only
the gf.len check caught it.

A failure on a later queue does detach tfiles[0] and the queues before it.
The rejections this test exercises can't reach that case, though.

Would re-attaching a queue, or opening a new one, after the failed ioctl
and then checking IFF_NOFILTER show that filter_attached and fprog_kern
were preserved?

> +}

[ ... ]

[Severity: Low]
One more question about the series. This one is about tun_copy_filter(),
added by "tun: keep a kernel copy of the socket filter program" rather
than by this patch:

drivers/net/tun.c:tun_copy_filter() {
	...
	insns = memdup_array_user(fprog.filter, fprog.len,
				  sizeof(struct sock_filter));
	...
}

Should this copy be charged to the caller's memory cgroup?

memdup_array_user() goes through memdup_user(), which allocates with
GFP_USER | __GFP_NOWARN and no __GFP_ACCOUNT. The buffer can be up to
BPF_MAXINSNS * sizeof(struct sock_filter), which is 32 KiB. It is freed
only by tun_detach_filter() or tun_free_netdev(), so it can live as long
as the device.

The netdev and bpf_prog allocations are memcg accounted, but this one is
not. A user with CAP_NET_ADMIN in a user namespace could create many
persistent TAP devices, and each one could pin up to 32 KiB of unaccounted
slab.

Would kmalloc_array(..., GFP_KERNEL_ACCOUNT) plus copy_from_user() close
the gap? There is precedent for unaccounted classic filter copies in
net/core, since bpf_prog_store_orig_filter() uses GFP_KERNEL, so this is
only a minor inconsistency.

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