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

[Kuniyuki Iwashima]

From: Xiao Liang <shaw.l...@gmail.com>
Date: Tue, 7 Jan 2025 20:53:19 +0800
> On Tue, Jan 7, 2025 at 4:57 PM Kuniyuki Iwashima <kun...@amazon.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<---