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

From: Kuniyuki Iwashima <hidden>
Date: 2025-01-07 14:47:39
Also in: batman, bpf, bridge, linux-can, linux-kselftest, linux-rdma, linux-wireless, lkml
Subsystem: gtp (gprs tunneling protocol), networking drivers, the rest · Maintainers: Pablo Neira Ayuso, Harald Welte, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds

From: Xiao Liang <redacted>
Date: Tue, 7 Jan 2025 20:53:19 +0800

On Tue, Jan 7, 2025 at 4:57 PM Kuniyuki Iwashima [off-list ref] wrote:
[...]

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help