Re: [PATCH net v4 2/2] 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] In reattach_filter_without_user_buffer()
  (tools/testing/selftests/net/tun.c), the inline comment "The process…

--

selftests: net: add TAP socket filter attach tests

This adds four TAP selftests in tools/testing/selftests/net/tun.c for
socket filter attach. They check that a filter is reattached from the
kernel copy after the user buffer becomes unreadable, that a rejected
TUNATTACHFILTER leaves the existing program in place, and that
IFF_NOFILTER and TUNDETACHFILTER both result in a queue without a filter.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50bb..cfaba57274025 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 };
> +	struct ifreq ifr = { 0 };
> +	short flags = 0;
> +	void *prog;
> +	int fd, 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);
> +	/* The queue is attached again, not left disabled */
> +	EXPECT_EQ(flags & IFF_DETACH_QUEUE, 0);
> +	EXPECT_EQ(flags & IFF_NOFILTER, 0);
> +
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Low]
Is the comment above the mprotect() call accurate? mprotect(PROT_NONE)
leaves the page mapped and only makes it inaccessible. Later in the same
test the protection is put back and the page is unmapped:

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

Would something like "this memory is no longer readable" describe it
better?

Also, do the IFF_NOFILTER and gf.len checks on self->fd test the reattach
at all?

__tun_detach() with clean=false does not remove the socket filter from the
detached queue. So IFF_NOFILTER would still be clear on self->fd even if
tun_attach() installed nothing.

TUNGETFILTER returns the tun->fprog header, and the reattach path does not
change it. So gf.len matches no matter what happened during reattach.

The checks that do test something are ret == 0, IFF_DETACH_QUEUE == 0, and
the IFF_NOFILTER check on the fresh socket below. Could the two redundant
checks be dropped, or replaced with ones that can tell a reinstalled filter
from no filter?

On the patched kernel, sk_attach_filter_kern() succeeds from the kernel
copy. So this test does run the detached-queue success path, and the
IFF_DETACH_QUEUE check would catch the earlier "success, goto out without
publishing" bug if it came back.

> +
> +	/* A new queue starts without a filter, so a clear IFF_NOFILTER shows
> +	 * that the filter was installed again from the kernel copy.
> +	 */

[ ... ]

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