Re: [PATCH net v4 2/2] selftests: net: add TAP socket filter attach tests
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-04 06:39:57
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ + /* 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