Re: [PATCH net v7 2/2] selftests: net: add TAP socket filter attach tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] attach_filter_bad_prog_keeps_program() in
  tools/testing/selftests/net/tun.c says a refused program "must leave…

--

selftests: net: add TAP socket filter attach tests

This commit adds five TAP socket filter tests to
tools/testing/selftests/net/tun.c. They cover reinstalling the filter
from the kernel copy after the user buffer is made unreadable, attaching
queues with IFF_NOFILTER or after TUNDETACHFILTER, and TUNATTACHFILTER
calls rejected for a bad length or for bad instructions.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index afaa81c9bac53..1efff537dbae9 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> +/* 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),
> +	};
> +
> +	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);
> +
> +	/* the program attached before is untouched */
> +	EXPECT_EQ(filter_get(self->fd, &gf), 0);
> +	EXPECT_EQ(gf.len, ARRAY_SIZE(filter_accept));

[Severity: Medium]
Can this check tell whether the earlier program is still installed?

TUNGETFILTER in __tun_chr_ioctl() only copies out the saved header:

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

tun_detach_filter() is also the failure path of tun_attach_filter(). It
drops every queue's sk_filter, frees fprog_kern and clears
filter_attached, but it never touches tun->fprog:

drivers/net/tun.c:tun_detach_filter() {
	for (i = 0; i < n; i++) {
		...
		sk_detach_filter(tfile->socket.sk);
		...
	}

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

Suppose a later change made the bad program path go through
tun_detach_filter(), for example by validating after the attach and
rolling back with tun_detach_filter(tun, tun->numqueues). gf.len would
still be 3 and this test would pass. Meanwhile the queues would have lost
their filter, and queues attached later would come up without one.

The current kernel passes for the right reason. tun_copy_filter() calls
bpf_prog_validate_classic() before it changes any tun state. The test
just can't catch a regression in the behaviour it says it covers.

attach_filter_bad_len_keeps_program() handles this by checking
IFF_NOFILTER through TUNGETIFF on self->fd. It also attaches a fresh
TUNSETIFF queue and checks that the new queue gets the filter. Could the
same checks be added here, so that the test verifies what the commit
message describes?

  "checks that the program installed before is left in place"

The comment in reattach_filter_without_user_buffer() makes the same point
about this ioctl: "TUNGETFILTER only returns the header".

> +}
> +
>  FIXTURE(tun_vnet_gso)
>  {
>  	char ifname[IFNAMSIZ];

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