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

Kuniyuki Iwashima
2025-01-07T14:47:39Z

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