[PATCH net 1/2] net: tun: avoid disabling NAPI twice

[PATCH net 1/2] net: tun: avoid disabling NAPI twice

[Jakub Kicinski]

Eric reports that syzbot made short work out of my speculative
fix. Indeed when queue gets detached its tfile->tun remains,
so we would try to stop NAPI twice with a detach(), close()
sequence.

Alternative fix would be to move tun_napi_disable() to
tun_detach_all() and let the NAPI run after the queue
has been detached.

Fixes: a8fc8cb5692a ("net: tun: stop NAPI when detaching queues")
Reported-by: syzbot <syzkaller@googlegroups.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Cc: Petar Penkov <ppenkov@aviatrix.com>
---
CC: ppenkov@aviatrix.com
---
 drivers/net/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e2eb35887394..259b2b84b2b3 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -640,7 +640,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
 	tun = rtnl_dereference(tfile->tun);
 
 	if (tun && clean) {
-		tun_napi_disable(tfile);
+		if (!tfile->detached)
+			tun_napi_disable(tfile);
 		tun_napi_del(tfile);
 	}
 
-- 
2.36.1

[PATCH net 2/2] selftest: tun: add test for NAPI dismantle

[Jakub Kicinski]

Being lazy does not pay, add the test for various
ordering of tun queue close / detach / destroy.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/Makefile |   2 +-
 tools/testing/selftests/net/tun.c    | 162 +++++++++++++++++++++++++++
 2 files changed, 163 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/tun.c

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 7ea54af55490..ddad703ace34 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -54,7 +54,7 @@ TEST_GEN_FILES += ipsec
 TEST_GEN_FILES += ioam6_parser
 TEST_GEN_FILES += gro
 TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
-TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
+TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls tun
 TEST_GEN_FILES += toeplitz
 TEST_GEN_FILES += cmsg_sender
 TEST_GEN_FILES += stress_reuseport_listen
diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
new file mode 100644
index 000000000000..fa83918b62d1
--- /dev/null
+++ b/tools/testing/selftests/net/tun.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <linux/if.h>
+#include <linux/if_tun.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+
+#include "../kselftest_harness.h"
+
+static int tun_attach(int fd, char *dev)
+{
+	struct ifreq ifr;
+
+	memset(&ifr, 0, sizeof(ifr));
+	strcpy(ifr.ifr_name, dev);
+	ifr.ifr_flags = IFF_ATTACH_QUEUE;
+
+	return ioctl(fd, TUNSETQUEUE, (void *) &ifr);
+}
+
+static int tun_detach(int fd, char *dev)
+{
+	struct ifreq ifr;
+
+	memset(&ifr, 0, sizeof(ifr));
+	strcpy(ifr.ifr_name, dev);
+	ifr.ifr_flags = IFF_DETACH_QUEUE;
+
+	return ioctl(fd, TUNSETQUEUE, (void *) &ifr);
+}
+
+static int tun_alloc(char *dev)
+{
+	struct ifreq ifr;
+	int fd, err;
+
+	fd = open("/dev/net/tun", O_RDWR);
+	if (fd < 0) {
+		fprintf(stderr, "can't open tun: %s\n", strerror(errno));
+		return fd;
+	}
+
+	memset(&ifr, 0, sizeof(ifr));
+	strcpy(ifr.ifr_name, dev);
+	ifr.ifr_flags = IFF_TAP | IFF_NAPI | IFF_MULTI_QUEUE;
+
+	err = ioctl(fd, TUNSETIFF, (void *) &ifr);
+	if (err < 0) {
+		fprintf(stderr, "can't TUNSETIFF: %s\n", strerror(errno));
+		close(fd);
+		return err;
+	}
+	strcpy(dev, ifr.ifr_name);
+	return fd;
+}
+
+static int tun_delete(char *dev)
+{
+	struct {
+		struct nlmsghdr  nh;
+		struct ifinfomsg ifm;
+		unsigned char    data[64];
+	} req;
+	struct rtattr *rta;
+	int ret, rtnl;
+
+	rtnl = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
+	if (rtnl < 0) {
+		fprintf(stderr, "can't open rtnl: %s\n", strerror(errno));
+		return 1;
+	}
+
+	memset(&req, 0, sizeof(req));
+	req.nh.nlmsg_len = NLMSG_ALIGN(NLMSG_LENGTH(sizeof(req.ifm)));
+	req.nh.nlmsg_flags = NLM_F_REQUEST;
+	req.nh.nlmsg_type = RTM_DELLINK;
+
+	req.ifm.ifi_family = AF_UNSPEC;
+
+	rta = (struct rtattr *)(((char *)&req) + NLMSG_ALIGN(req.nh.nlmsg_len));
+	rta->rta_type = IFLA_IFNAME;
+	rta->rta_len = RTA_LENGTH(IFNAMSIZ);
+	req.nh.nlmsg_len += rta->rta_len;
+	memcpy(RTA_DATA(rta), dev, IFNAMSIZ);
+
+	ret = send(rtnl, &req, req.nh.nlmsg_len, 0);
+	if (ret < 0)
+		fprintf(stderr, "can't send: %s\n", strerror(errno));
+	ret = (unsigned int)ret != req.nh.nlmsg_len;
+
+	close(rtnl);
+	return ret;
+}
+
+FIXTURE(tun)
+{
+	char ifname[IFNAMSIZ];
+	int fd, fd2;
+};
+
+FIXTURE_SETUP(tun)
+{
+	memset(self->ifname, 0, sizeof(self->ifname));
+
+	self->fd = tun_alloc(self->ifname);
+	ASSERT_GE(self->fd, 0);
+
+	self->fd2 = tun_alloc(self->ifname);
+	ASSERT_GE(self->fd2, 0);
+}
+
+FIXTURE_TEARDOWN(tun)
+{
+	if (self->fd >= 0)
+		close(self->fd);
+	if (self->fd2 >= 0)
+		close(self->fd2);
+}
+
+TEST_F(tun, delete_detach_close) {
+	EXPECT_EQ(tun_delete(self->ifname), 0);
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), -1);
+	EXPECT_EQ(errno, 22);
+}
+
+TEST_F(tun, detach_delete_close) {
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_delete(self->ifname), 0);
+}
+
+TEST_F(tun, detach_close_delete) {
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	close(self->fd);
+	self->fd = -1;
+	EXPECT_EQ(tun_delete(self->ifname), 0);
+}
+
+TEST_F(tun, reattach_delete_close) {
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_attach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_delete(self->ifname), 0);
+}
+
+TEST_F(tun, reattach_close_delete) {
+	EXPECT_EQ(tun_detach(self->fd, self->ifname), 0);
+	EXPECT_EQ(tun_attach(self->fd, self->ifname), 0);
+	close(self->fd);
+	self->fd = -1;
+	EXPECT_EQ(tun_delete(self->ifname), 0);
+}
+
+TEST_HARNESS_MAIN
-- 
2.36.1

Re: [PATCH net 1/2] net: tun: avoid disabling NAPI twice

[Eric Dumazet]

On Wed, Jun 29, 2022 at 8:19 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Eric reports that syzbot made short work out of my speculative
> fix. Indeed when queue gets detached its tfile->tun remains,
> so we would try to stop NAPI twice with a detach(), close()
> sequence.
>
> Alternative fix would be to move tun_napi_disable() to
> tun_detach_all() and let the NAPI run after the queue
> has been detached.
>
> Fixes: a8fc8cb5692a ("net: tun: stop NAPI when detaching queues")

Reviewed-by: Eric Dumazet <edumazet@google.com>

> Reported-by: syzbot <syzkaller@googlegroups.com>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Cc: Petar Penkov <ppenkov@aviatrix.com>
> ---
> CC: ppenkov@aviatrix.com
> ---
>  drivers/net/tun.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e2eb35887394..259b2b84b2b3 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -640,7 +640,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>         tun = rtnl_dereference(tfile->tun);
>
>         if (tun && clean) {
> -               tun_napi_disable(tfile);
> +               if (!tfile->detached)
> +                       tun_napi_disable(tfile);
>                 tun_napi_del(tfile);
>         }
>
> --
> 2.36.1
>

Re: [PATCH net 1/2] net: tun: avoid disabling NAPI twice

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 Jun 2022 11:19:10 -0700 you wrote:
> Eric reports that syzbot made short work out of my speculative
> fix. Indeed when queue gets detached its tfile->tun remains,
> so we would try to stop NAPI twice with a detach(), close()
> sequence.
> 
> Alternative fix would be to move tun_napi_disable() to
> tun_detach_all() and let the NAPI run after the queue
> has been detached.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: tun: avoid disabling NAPI twice
    https://git.kernel.org/netdev/net/c/ff1fa2081d17
  - [net,2/2] selftest: tun: add test for NAPI dismantle
    https://git.kernel.org/netdev/net/c/839b92fede7b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html