netdev - Re: [PATCH net-next v7 00/11] net: Improve netns handling in rtnetlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250107144714.74446-1-kuniyu@amazon.com>
Date: Tue, 7 Jan 2025 23:47:14 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <shaw.leon@...il.com>
CC: <andrew+netdev@...n.ch>, <b.a.t.m.a.n@...ts.open-mesh.org>,
	<bpf@...r.kernel.org>, <bridge@...ts.linux.dev>, <davem@...emloft.net>,
	<donald.hunter@...il.com>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<horms@...nel.org>, <idosch@...dia.com>, <jiri@...nulli.us>,
	<kuba@...nel.org>, <kuniyu@...zon.com>, <linux-can@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<linux-ppp@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
	<linux-wireless@...r.kernel.org>, <linux-wpan@...r.kernel.org>,
	<liuhangbin@...il.com>, <netdev@...r.kernel.org>,
	<osmocom-net-gprs@...ts.osmocom.org>, <pabeni@...hat.com>,
	<shuah@...nel.org>, <wireguard@...ts.zx2c4.com>
Subject: Re: [PATCH net-next v7 00/11] net: Improve netns handling in rtnetlink

From: Xiao Liang <shaw.leon@...il.com>
Date: Tue, 7 Jan 2025 20:53:19 +0800
> On Tue, Jan 7, 2025 at 4:57 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> [...]
> >
> > We can fix this by linking the dev to the socket's netns and
> > clean them up in __net_exit hook as done in bareudp and geneve.
> >
> > ---8<---
> > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> > index 89a996ad8cd0..77638a815873 100644
> > --- a/drivers/net/gtp.c
> > +++ b/drivers/net/gtp.c
> > @@ -70,6 +70,7 @@ struct pdp_ctx {
> >  /* One instance of the GTP device. */
> >  struct gtp_dev {
> >         struct list_head        list;
> > +       struct list_head        sock_list;
> >
> >         struct sock             *sk0;
> >         struct sock             *sk1u;
> > @@ -102,6 +103,7 @@ static unsigned int gtp_net_id __read_mostly;
> >
> >  struct gtp_net {
> >         struct list_head gtp_dev_list;
> > +       struct list_head gtp_sock_list;
> 
> After a closer look at the GTP driver, I'm confused about
> the gtp_dev_list here. GTP device is linked to this list at
> creation time, but netns can be changed afterwards.
> The list is used in gtp_net_exit_batch_rtnl(), but to my
> understanding net devices can already be deleted in
> default_device_exit_batch() by default.
> And I wonder if the use in gtp_genl_dump_pdp() can be
> replaced by something like for_each_netdev_rcu().

Right, it should be, or we need to set netns_local.
Will include this diff in the fix series.

---8<---
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 2460a2c13c32..f9186eda36f0 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -2278,6 +2278,7 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
 	struct gtp_dev *last_gtp = (struct gtp_dev *)cb->args[2], *gtp;
 	int i, j, bucket = cb->args[0], skip = cb->args[1];
 	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
 	struct pdp_ctx *pctx;
 	struct gtp_net *gn;
 
@@ -2287,7 +2288,10 @@ static int gtp_genl_dump_pdp(struct sk_buff *skb,
 		return 0;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(gtp, &gn->gtp_dev_list, list) {
+	for_each_netdev_rcu(net, dev) {
+		if (dev->rtnl_link_ops != &gtp_link_ops)
+			continue;
+
 		if (last_gtp && last_gtp != gtp)
 			continue;
 		else
---8<---

Otherwise, we need to move it manually like this, which is
apparently overkill and unnecessary :p

---8<---
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 2460a2c13c32..90b410b73c89 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -2501,6 +2501,46 @@ static struct pernet_operations gtp_net_ops = {
 	.size	= sizeof(struct gtp_net),
 };
 
+static int gtp_device_event(struct notifier_block *nb,
+			    unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+	struct gtp_dev *gtp;
+	struct gtp_net *gn;
+
+	if (dev->rtnl_link_ops != &gtp_link_ops)
+		goto out;
+
+	gtp = netdev_priv(dev);
+
+	switch (event) {
+	case NETDEV_UNREGISTER:
+		if (dev->reg_state != NETREG_REGISTERED)
+			goto out;
+
+		/* dev_net(dev) is changed, see __dev_change_net_namespace().
+		 * rcu_barrier() after NETDEV_UNREGISTER guarantees that no
+		 * one traversing a list in the old netns jumps to another
+		 * list in the new netns.
+		 */
+		list_del_rcu(&gtp->list);
+		break;
+	case NETDEV_REGISTER:
+		if (gtp->list.prev != LIST_POISON2)
+			goto out;
+
+		/* complete netns change. */
+		gn = net_generic(dev_net(dev), gtp_net_id);
+		list_add_rcu(&gtp->list, &gn->gtp_dev_list);
+	}
+out:
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block gtp_notifier_block = {
+	.notifier_call = gtp_device_event,
+};
+
 static int __init gtp_init(void)
 {
 	int err;
@@ -2511,10 +2551,14 @@ static int __init gtp_init(void)
 	if (err < 0)
 		goto error_out;
 
-	err = rtnl_link_register(&gtp_link_ops);
+	err = register_netdevice_notifier(&gtp_notifier_block);
 	if (err < 0)
 		goto unreg_pernet_subsys;
 
+	err = rtnl_link_register(&gtp_link_ops);
+	if (err < 0)
+		goto unreg_netdev_notifier;
+
 	err = genl_register_family(&gtp_genl_family);
 	if (err < 0)
 		goto unreg_rtnl_link;
@@ -2525,6 +2569,8 @@ static int __init gtp_init(void)
 
 unreg_rtnl_link:
 	rtnl_link_unregister(&gtp_link_ops);
+unreg_netdev_notifier:
+	register_netdevice_notifier(&gtp_notifier_block);
 unreg_pernet_subsys:
 	unregister_pernet_subsys(&gtp_net_ops);
 error_out:
@@ -2537,6 +2583,7 @@ static void __exit gtp_fini(void)
 {
 	genl_unregister_family(&gtp_genl_family);
 	rtnl_link_unregister(&gtp_link_ops);
+	register_netdevice_notifier(&gtp_notifier_block);
 	unregister_pernet_subsys(&gtp_net_ops);
 
 	pr_info("GTP module unloaded\n");
---8<---