Re: [PATCH v2 4/4] selftests: net: add TAP socket filter attach tests

on 2026/9/30 10:48, Willem de Bruijn wrote:

Rongguang Wei wrote:

quoted

From: Rongguang Wei <redacted>

reattach_filter_without_user_buffer() attaches a filter, makes the buffer
the program was copied from unreadable and then attaches the queue again,
which used to fail with -EFAULT.

attach_filter_bad_len_keeps_descriptor() checks that a rejected
TUNATTACHFILTER leaves both the saved descriptor and the installed filter
alone.

attach_filter_nofilter_flag() and detach_filter_clears_reattach() cover the
ways to attach a queue without a filter.

Signed-off-by: Rongguang Wei <redacted>

quoted

+/* accept: return skb->len */
+static const struct sock_filter filter_accept[] = {
+	BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0),
+	BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0),
+	BPF_STMT(BPF_RET | BPF_A, 0),
+};
+
+/* drop: return 0 */
+static const struct sock_filter filter_drop[] = {
+	BPF_STMT(BPF_RET | BPF_K, 0),
+};

Are both programs needed? One will do, right?

Yes, I dropped filter_drop and use filter_accept.

quoted

+static int tap_get_iff(int fd, short *flags)

nit: tun_get_iff? Or even tun_get_iff_flags.

Also, could just return flags if positive, no call-by-reference needed.

Rename to tun_get_iff_flags() now and returns the flags, the call sites like:

	flags = tun_get_iff_flags(fd);
	EXPECT_GE(flags, 0);

quoted

+{
+	struct ifreq ifr = { 0 };
+
+	if (ioctl(fd, TUNGETIFF, (void *)&ifr) < 0)
+		return -1;
+
+	*flags = ifr.ifr_flags;
+
+	return 0;
+}
+

quoted

+/* A new queue attached with IFF_NOFILTER must not get the filter that is
+ * configured on the device.
+ */
+TEST_F(tun, attach_filter_nofilter_flag)
+{
+	struct ifreq ifr = { 0 };
+	short flags = 0;
+	int fd;
+
+	ASSERT_EQ(filter_attach(self->fd, filter_drop, ARRAY_SIZE(filter_drop)), 0);
+
+	fd = open("/dev/net/tun", O_RDWR);
+	ASSERT_GE(fd, 0);
+
+	strcpy(ifr.ifr_name, self->ifname);
+	ifr.ifr_flags = IFF_TAP | IFF_MULTI_QUEUE | IFF_NOFILTER;
+	EXPECT_GE(ioctl(fd, TUNSETIFF, (void *)&ifr), 0);
+
+	EXPECT_EQ(tap_get_iff(fd, &flags), 0);
+	EXPECT_NE(flags & IFF_NOFILTER, 0);

nit: here and elsewhere: avoid the double negative?

quoted

	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER)

Done, all three of them.

quoted

+/* A TUNATTACHFILTER with a bad length must not clobber the saved descriptor */

What is the descriptor in this context? Do you mean installed program?

Yes, the installed program.

Thank you, I will send the series as v3, which also puts the kernel copy of the program
before the corrected error check, as suggested in the CI review of patch 1.

quoted

+TEST_F(tun, attach_filter_bad_len_keeps_descriptor)
+{
+	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));
+
+	EXPECT_EQ(tap_get_iff(self->fd, &flags), 0);
+	EXPECT_EQ(flags & IFF_NOFILTER, 0);
+}
+
 FIXTURE(tun_vnet_udptnl)
 {
 	char ifname[IFNAMSIZ];
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus