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

From: Xiao Liang <[email protected]>
Date: Tue, 7 Jan 2025 20:53:19 +0800
> On Tue, Jan 7, 2025 at 4:57 PM Kuniyuki Iwashima <[email protected]> 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<---