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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> 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 <weirongguang@kylinos.cn>
  
> +/* 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?

> +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.

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

> +/* 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?

	EXPECT_EQ(flags & IFF_NOFILTER, IFF_NOFILTER)

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

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

> +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
>