* [PATCH net 1/2] net: tun: avoid disabling NAPI twice
@ 2022-06-29 18:19 Jakub Kicinski
2022-06-29 18:19 ` [PATCH net 2/2] selftest: tun: add test for NAPI dismantle Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-06-29 18:19 UTC (permalink / raw)
To: edumazet; +Cc: netdev, davem, pabeni, Jakub Kicinski, syzbot, Petar Penkov
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net 2/2] selftest: tun: add test for NAPI dismantle
2022-06-29 18:19 [PATCH net 1/2] net: tun: avoid disabling NAPI twice Jakub Kicinski
@ 2022-06-29 18:19 ` Jakub Kicinski
2022-06-30 10:27 ` [PATCH net 1/2] net: tun: avoid disabling NAPI twice Eric Dumazet
2022-06-30 18:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-06-29 18:19 UTC (permalink / raw)
To: edumazet; +Cc: netdev, davem, pabeni, Jakub Kicinski, shuah, linux-kselftest
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net 1/2] net: tun: avoid disabling NAPI twice
2022-06-29 18:19 [PATCH net 1/2] net: tun: avoid disabling NAPI twice Jakub Kicinski
2022-06-29 18:19 ` [PATCH net 2/2] selftest: tun: add test for NAPI dismantle Jakub Kicinski
@ 2022-06-30 10:27 ` Eric Dumazet
2022-06-30 18:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Eric Dumazet @ 2022-06-30 10:27 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, David Miller, Paolo Abeni, syzbot, Petar Penkov
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net 1/2] net: tun: avoid disabling NAPI twice
2022-06-29 18:19 [PATCH net 1/2] net: tun: avoid disabling NAPI twice Jakub Kicinski
2022-06-29 18:19 ` [PATCH net 2/2] selftest: tun: add test for NAPI dismantle Jakub Kicinski
2022-06-30 10:27 ` [PATCH net 1/2] net: tun: avoid disabling NAPI twice Eric Dumazet
@ 2022-06-30 18:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-06-30 18:50 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: edumazet, netdev, davem, pabeni, syzkaller, ppenkov
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-30 18:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-29 18:19 [PATCH net 1/2] net: tun: avoid disabling NAPI twice Jakub Kicinski
2022-06-29 18:19 ` [PATCH net 2/2] selftest: tun: add test for NAPI dismantle Jakub Kicinski
2022-06-30 10:27 ` [PATCH net 1/2] net: tun: avoid disabling NAPI twice Eric Dumazet
2022-06-30 18:50 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.