netdev - Re: [PATCH v2 net-next 2/3] pfcp: Convert pfcp_net_exit() to ->exit

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: <20250423145736.95775-1-kuniyu@amazon.com>
Date: Wed, 23 Apr 2025 07:12:55 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v2 net-next 2/3] pfcp: Convert pfcp_net_exit() to ->exit_rtnl().

From: Jakub Kicinski <kuba@...nel.org>
Date: Wed, 23 Apr 2025 06:40:26 -0700
> On Thu, 17 Apr 2025 17:32:33 -0700 Kuniyuki Iwashima wrote:
> > -static void __net_exit pfcp_net_exit(struct net *net)
> > +static void __net_exit pfcp_net_exit_rtnl(struct net *net,
> > +					  struct list_head *dev_to_kill)
> >  {
> >  	struct pfcp_net *pn = net_generic(net, pfcp_net_id);
> >  	struct pfcp_dev *pfcp, *pfcp_next;
> > -	struct net_device *dev;
> > -	LIST_HEAD(list);
> > -
> > -	rtnl_lock();
> > -	for_each_netdev(net, dev)
> > -		if (dev->rtnl_link_ops == &pfcp_link_ops)
> > -			pfcp_dellink(dev, &list);
> >  
> >  	list_for_each_entry_safe(pfcp, pfcp_next, &pn->pfcp_dev_list, list)
> > -		pfcp_dellink(pfcp->dev, &list);
> > -
> > -	unregister_netdevice_many(&list);
> > -	rtnl_unlock();
> > +		pfcp_dellink(pfcp->dev, dev_to_kill);
> 
> Kuniyuki, I got distracted by the fact the driver is broken but I think
> this isn't right.

I guess it was broken recently ?  at least I didn't see null-deref
while testing ffc90e9ca61b ("pfcp: Destroy device along with udp
socket's netns dismantle.").


> The devices do not migrate to the local pcfp_dev_list
> when their netns is changed. They always stay on the list of original
> netns. Which I guess may make some sense as that's where their socket
> is? So we need to scan both the pcfp_dev_list _and_ the local netdevs
> that are pfcp. Am I misunderstanding something?

If the netns is queued up for cleanup_net(), the local netdevs
are handled later by default_device_exit_batch().

If the module is unloaded, we call pfcp_net_exit_rtnl() for
all netns including all local netdevs.

I remember bareudp did like that and I changed geneve and gtp
as well.

I applied the quivalent diff below on ffc90e9ca61b, and during
netns dismantle, 2 local netdevs were removed properly (one
was originally created there, another was moved to the netns).

---8<---
[root@...alhost ~]# ip netns add ns1
[root@...alhost ~]# ip netns add ns2
[root@...alhost ~]# ip -n ns1 link add netns ns2 name pfcp0 type pfcp
[   74.564522] newlink: dev: pfcp0 at ff11000103ca9000
[root@...alhost ~]# ip -n ns2 link add netns ns2 name pfcp1 type pfcp
[   79.510334] newlink: dev: pfcp1 at ff11000103caa000
[root@...alhost ~]# ip netns del ns2
[   83.953871] dellink: dev: pfcp1 at ff11000103caa000 from cleanup_net+0x20e/0x3b0
[   83.980520] dellink: dev: pfcp0 at ff11000103ca9000 from default_device_exit_batch+0x244/0x300
---8<---

---8<---
diff --git a/drivers/net/pfcp.c b/drivers/net/pfcp.c
index 68d0d9e92a22..f7f254d7d031 100644
--- a/drivers/net/pfcp.c
+++ b/drivers/net/pfcp.c
@@ -206,6 +206,7 @@ static int pfcp_newlink(struct net *net, struct net_device *dev,
 		goto exit_del_pfcp_sock;
 	}
 
+	printk(KERN_ERR "newlink: dev: %s at %px\n", dev->name, dev);
 	pn = net_generic(net, pfcp_net_id);
 	list_add(&pfcp->list, &pn->pfcp_dev_list);
 
@@ -224,6 +225,7 @@ static void pfcp_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct pfcp_dev *pfcp = netdev_priv(dev);
 
+	printk(KERN_ERR "dellink: dev: %s at %px from %pS\n", dev->name, dev, __builtin_return_address(0));
 	list_del(&pfcp->list);
 	unregister_netdevice_queue(dev, head);
 }
@@ -244,28 +246,26 @@ static int __net_init pfcp_net_init(struct net *net)
 	return 0;
 }
 
-static void __net_exit pfcp_net_exit(struct net *net)
+static void __net_exit pfcp_net_exit(struct net *net, struct list_head *list)
 {
 	struct pfcp_net *pn = net_generic(net, pfcp_net_id);
 	struct pfcp_dev *pfcp, *pfcp_next;
-	struct net_device *dev;
-	LIST_HEAD(list);
-
-	rtnl_lock();
-	for_each_netdev(net, dev)
-		if (dev->rtnl_link_ops == &pfcp_link_ops)
-			pfcp_dellink(dev, &list);
 
 	list_for_each_entry_safe(pfcp, pfcp_next, &pn->pfcp_dev_list, list)
-		pfcp_dellink(pfcp->dev, &list);
+		pfcp_dellink(pfcp->dev, list);
+}
+
+static void __net_exit pfcp_net_exit_batch_rtnl(struct list_head *net_list, struct list_head *head)
+{
+	struct net *net;
 
-	unregister_netdevice_many(&list);
-	rtnl_unlock();
+	list_for_each_entry(net, net_list, exit_list)
+		pfcp_net_exit(net, head);
 }
 
 static struct pernet_operations pfcp_net_ops = {
 	.init	= pfcp_net_init,
-	.exit	= pfcp_net_exit,
+	.exit_batch_rtnl	= pfcp_net_exit_batch_rtnl,
 	.id	= &pfcp_net_id,
 	.size	= sizeof(struct pfcp_net),
 };
---8<---