Re: [PATCH net v9 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] The two new negative tests, attach_filter_bad_len_keeps_program()
  and attach_filter_bad_prog_keeps_program() in…
- [Low] The header comment of reattach_filter_without_user_buffer() in
  tools/testing/selftests/net/tun.c says 'a second queue is then…

--

selftests: net: add TAP socket filter attach tests

This adds TUNATTACHFILTER selftests to tools/testing/selftests/net/tun.c.
One reattaches a queue after the user buffer holding the program has been
made unreadable. Others check that IFF_NOFILTER and TUNDETACHFILTER keep a
filter off later queues, and that a bad length or bad instructions are
refused without touching the installed program.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index 34de2580e32ec..3730fba859a46 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> @@ -548,6 +550,253 @@ TEST_F(tun, reattach_close_delete)

[ ... ]

> +/*
> + * A queue can be attached long after the filter was configured, from a
> + * process that does not necessarily map the buffer the program was copied
> + * from, so the kernel has to keep its own copy of the program.  Here the
> + * mapping is made unreadable before the queue is attached again, and a
> + * second queue is then attached from a fresh socket, which starts without a
> + * filter of its own.
> + */

[Severity: Low]
This isn't a bug, but is this really the second queue? FIXTURE_SETUP(tun)
has already attached self->fd and self->fd2:

    self->fd = tun_alloc(self->ifname);
    ...
    self->fd2 = tun_alloc(self->ifname);

That makes the fd that this test opens and attaches with TUNSETIFF the third
queue. The commit message uses the same wording:

  "It then attaches a second queue from a fresh socket, which starts
  without a filter of its own"

> +TEST_F(tun, reattach_filter_without_user_buffer)
> +{

[ ... ]

> +/* A TUNATTACHFILTER with instructions that bpf_check_classic() rejects must
> + * be refused, and must leave the program installed already in place.
> + */
> +TEST_F(tun, attach_filter_bad_prog_keeps_program)
> +{
> +	struct sock_fprog gf = { 0 };
> +	struct sock_filter bad[] = {
> +		BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
> +	};
> +	struct ifreq ifr = { 0 };
> +	short flags = 0;
> +	int fd;
> +
> +	ASSERT_EQ(filter_attach(self->fd, filter_accept,
> +				ARRAY_SIZE(filter_accept)), 0);
> +
> +	errno = 0;
> +	EXPECT_EQ(filter_attach(self->fd, bad, ARRAY_SIZE(bad)), -1);
> +	EXPECT_EQ(errno, EINVAL);

[Severity: Low]
This test and attach_filter_bad_len_keeps_program() both send the bad
ioctl while the two queues from FIXTURE_SETUP(tun) are still attached, so
numqueues is 2.

Would both tests still pass if the early checks in tun_copy_filter() were
removed?

drivers/net/tun.c:tun_copy_filter() {
    ...
	if (!fp.len || fp.len > BPF_MAXINSNS)
		return -EINVAL;
    ...
	err = bpf_prog_validate_classic(insns, fp.len);
    ...
}

Without those checks, tun_attach_filter() would call
sk_attach_filter_kern() on queue 0. bpf_prog_create() rejects len 0 in
bpf_check_basics_ok(). It rejects the bad jump program in
bpf_check_classic(), called from bpf_prepare_filter().

Both paths return -EINVAL before queue 0's sk_filter is changed. The
while (i--) rollback does nothing, and the saved program is not replaced.
So the tests would still see EINVAL, gf.len == 3, a clear IFF_NOFILTER, and
the new queue still getting the filter.

The early check only changes the result when tun->numqueues == 0. A queue
detached with TUNSETQUEUE/IFF_DETACH_QUEUE keeps tfile->tun, because
__tun_detach() only clears it when clean is true. TUNATTACHFILTER therefore
still reaches tun_attach_filter(), where the loop never runs:

	for (i = 0; i < tun->numqueues; i++) {
		...
	}

	tun->filter_attached = true;
	return 0;

Without the early check, the bad program would be saved there. The next
queue attach would then fail in sk_attach_filter_kern().

Could a variant be added that detaches both self->fd and self->fd2 first?
It would expect the bad TUNATTACHFILTER to fail with EINVAL, then reattach
and check that the old filter is installed again. That would test the
"refused before it is saved and before any queue is touched" behaviour that
the tun_copy_filter() comment describes.

[ ... ]

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