[PATCH net-next v2 0/4] net: inline rollback_registered() functions

[PATCH net-next v2 0/4] net: inline rollback_registered() functions

[Jakub Kicinski]

After recent changes to the error path of register_netdevice()
we no longer need a version of unregister_netdevice_many() which
does not set net_todo. We can inline the rollback_registered()
functions into respective unregister_netdevice() calls.

v2: - add missing list_del() in the last patch

Jakub Kicinski (4):
  net: move net_set_todo inside rollback_registered()
  net: inline rollback_registered()
  net: move rollback_registered_many()
  net: inline rollback_registered_many()

 net/core/dev.c | 210 +++++++++++++++++++++++--------------------------
 1 file changed, 98 insertions(+), 112 deletions(-)

-- 
2.26.2

[PATCH net-next v2 1/4] net: move net_set_todo inside rollback_registered()

[Jakub Kicinski]

Commit 93ee31f14f6f ("[NET]: Fix free_netdev on register_netdev
failure.") moved net_set_todo() outside of rollback_registered()
so that rollback_registered() can be used in the failure path of
register_netdevice() but without risking a double free.

Since commit cf124db566e6 ("net: Fix inconsistent teardown and
release of private netdev state."), however, we have a better
way of handling that condition, since destructors don't call
free_netdev() directly.

After the change in commit c269a24ce057 ("net: make free_netdev()
more lenient with unregistering devices") we can now move
net_set_todo() back.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6b90520a01b1..5f928b51c6b0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9546,8 +9546,10 @@ static void rollback_registered_many(struct list_head *head)
 
 	synchronize_net();
 
-	list_for_each_entry(dev, head, unreg_list)
+	list_for_each_entry(dev, head, unreg_list) {
 		dev_put(dev);
+		net_set_todo(dev);
+	}
 }
 
 static void rollback_registered(struct net_device *dev)
@@ -10104,7 +10106,6 @@ int register_netdevice(struct net_device *dev)
 		/* Expect explicit free_netdev() on failure */
 		dev->needs_free_netdev = false;
 		rollback_registered(dev);
-		net_set_todo(dev);
 		goto out;
 	}
 	/*
@@ -10727,8 +10728,6 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
 		list_move_tail(&dev->unreg_list, head);
 	} else {
 		rollback_registered(dev);
-		/* Finish processing unregister after unlock */
-		net_set_todo(dev);
 	}
 }
 EXPORT_SYMBOL(unregister_netdevice_queue);
@@ -10742,12 +10741,8 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
  */
 void unregister_netdevice_many(struct list_head *head)
 {
-	struct net_device *dev;
-
 	if (!list_empty(head)) {
 		rollback_registered_many(head);
-		list_for_each_entry(dev, head, unreg_list)
-			net_set_todo(dev);
 		list_del(head);
 	}
 }
-- 
2.26.2

[PATCH net-next v2 2/4] net: inline rollback_registered()

[Jakub Kicinski]

rollback_registered() is a local helper, it's common for driver
code to call unregister_netdevice_queue(dev, NULL) when they
want to unregister netdevices under rtnl_lock. Inline
rollback_registered() and adjust the only remaining caller.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5f928b51c6b0..85e9d4b7ddf2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9552,15 +9552,6 @@ static void rollback_registered_many(struct list_head *head)
 	}
 }
 
-static void rollback_registered(struct net_device *dev)
-{
-	LIST_HEAD(single);
-
-	list_add(&dev->unreg_list, &single);
-	rollback_registered_many(&single);
-	list_del(&single);
-}
-
 static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 	struct net_device *upper, netdev_features_t features)
 {
@@ -10105,7 +10096,7 @@ int register_netdevice(struct net_device *dev)
 	if (ret) {
 		/* Expect explicit free_netdev() on failure */
 		dev->needs_free_netdev = false;
-		rollback_registered(dev);
+		unregister_netdevice_queue(dev, NULL);
 		goto out;
 	}
 	/*
@@ -10727,7 +10718,11 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
 	if (head) {
 		list_move_tail(&dev->unreg_list, head);
 	} else {
-		rollback_registered(dev);
+		LIST_HEAD(single);
+
+		list_add(&dev->unreg_list, &single);
+		rollback_registered_many(&single);
+		list_del(&single);
 	}
 }
 EXPORT_SYMBOL(unregister_netdevice_queue);
-- 
2.26.2

[PATCH net-next v2 3/4] net: move rollback_registered_many()

[Jakub Kicinski]

Move rollback_registered_many() and add a temporary
forward declaration to make merging the code into
unregister_netdevice_many() easier to review.

No functional changes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 188 +++++++++++++++++++++++++------------------------
 1 file changed, 95 insertions(+), 93 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 85e9d4b7ddf2..a7841d03c910 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9459,99 +9459,6 @@ static void net_set_todo(struct net_device *dev)
 	dev_net(dev)->dev_unreg_count++;
 }
 
-static void rollback_registered_many(struct list_head *head)
-{
-	struct net_device *dev, *tmp;
-	LIST_HEAD(close_head);
-
-	BUG_ON(dev_boot_phase);
-	ASSERT_RTNL();
-
-	list_for_each_entry_safe(dev, tmp, head, unreg_list) {
-		/* Some devices call without registering
-		 * for initialization unwind. Remove those
-		 * devices and proceed with the remaining.
-		 */
-		if (dev->reg_state == NETREG_UNINITIALIZED) {
-			pr_debug("unregister_netdevice: device %s/%p never was registered\n",
-				 dev->name, dev);
-
-			WARN_ON(1);
-			list_del(&dev->unreg_list);
-			continue;
-		}
-		dev->dismantle = true;
-		BUG_ON(dev->reg_state != NETREG_REGISTERED);
-	}
-
-	/* If device is running, close it first. */
-	list_for_each_entry(dev, head, unreg_list)
-		list_add_tail(&dev->close_list, &close_head);
-	dev_close_many(&close_head, true);
-
-	list_for_each_entry(dev, head, unreg_list) {
-		/* And unlink it from device chain. */
-		unlist_netdevice(dev);
-
-		dev->reg_state = NETREG_UNREGISTERING;
-	}
-	flush_all_backlogs();
-
-	synchronize_net();
-
-	list_for_each_entry(dev, head, unreg_list) {
-		struct sk_buff *skb = NULL;
-
-		/* Shutdown queueing discipline. */
-		dev_shutdown(dev);
-
-		dev_xdp_uninstall(dev);
-
-		/* Notify protocols, that we are about to destroy
-		 * this device. They should clean all the things.
-		 */
-		call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
-
-		if (!dev->rtnl_link_ops ||
-		    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
-			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
-						     GFP_KERNEL, NULL, 0);
-
-		/*
-		 *	Flush the unicast and multicast chains
-		 */
-		dev_uc_flush(dev);
-		dev_mc_flush(dev);
-
-		netdev_name_node_alt_flush(dev);
-		netdev_name_node_free(dev->name_node);
-
-		if (dev->netdev_ops->ndo_uninit)
-			dev->netdev_ops->ndo_uninit(dev);
-
-		if (skb)
-			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL);
-
-		/* Notifier chain MUST detach us all upper devices. */
-		WARN_ON(netdev_has_any_upper_dev(dev));
-		WARN_ON(netdev_has_any_lower_dev(dev));
-
-		/* Remove entries from kobject tree */
-		netdev_unregister_kobject(dev);
-#ifdef CONFIG_XPS
-		/* Remove XPS queueing entries */
-		netif_reset_xps_queues_gt(dev, 0);
-#endif
-	}
-
-	synchronize_net();
-
-	list_for_each_entry(dev, head, unreg_list) {
-		dev_put(dev);
-		net_set_todo(dev);
-	}
-}
-
 static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 	struct net_device *upper, netdev_features_t features)
 {
@@ -10698,6 +10605,8 @@ void synchronize_net(void)
 }
 EXPORT_SYMBOL(synchronize_net);
 
+static void rollback_registered_many(struct list_head *head);
+
 /**
  *	unregister_netdevice_queue - remove device from the kernel
  *	@dev: device
@@ -10743,6 +10652,99 @@ void unregister_netdevice_many(struct list_head *head)
 }
 EXPORT_SYMBOL(unregister_netdevice_many);
 
+static void rollback_registered_many(struct list_head *head)
+{
+	struct net_device *dev, *tmp;
+	LIST_HEAD(close_head);
+
+	BUG_ON(dev_boot_phase);
+	ASSERT_RTNL();
+
+	list_for_each_entry_safe(dev, tmp, head, unreg_list) {
+		/* Some devices call without registering
+		 * for initialization unwind. Remove those
+		 * devices and proceed with the remaining.
+		 */
+		if (dev->reg_state == NETREG_UNINITIALIZED) {
+			pr_debug("unregister_netdevice: device %s/%p never was registered\n",
+				 dev->name, dev);
+
+			WARN_ON(1);
+			list_del(&dev->unreg_list);
+			continue;
+		}
+		dev->dismantle = true;
+		BUG_ON(dev->reg_state != NETREG_REGISTERED);
+	}
+
+	/* If device is running, close it first. */
+	list_for_each_entry(dev, head, unreg_list)
+		list_add_tail(&dev->close_list, &close_head);
+	dev_close_many(&close_head, true);
+
+	list_for_each_entry(dev, head, unreg_list) {
+		/* And unlink it from device chain. */
+		unlist_netdevice(dev);
+
+		dev->reg_state = NETREG_UNREGISTERING;
+	}
+	flush_all_backlogs();
+
+	synchronize_net();
+
+	list_for_each_entry(dev, head, unreg_list) {
+		struct sk_buff *skb = NULL;
+
+		/* Shutdown queueing discipline. */
+		dev_shutdown(dev);
+
+		dev_xdp_uninstall(dev);
+
+		/* Notify protocols, that we are about to destroy
+		 * this device. They should clean all the things.
+		 */
+		call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
+
+		if (!dev->rtnl_link_ops ||
+		    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
+			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
+						     GFP_KERNEL, NULL, 0);
+
+		/*
+		 *	Flush the unicast and multicast chains
+		 */
+		dev_uc_flush(dev);
+		dev_mc_flush(dev);
+
+		netdev_name_node_alt_flush(dev);
+		netdev_name_node_free(dev->name_node);
+
+		if (dev->netdev_ops->ndo_uninit)
+			dev->netdev_ops->ndo_uninit(dev);
+
+		if (skb)
+			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL);
+
+		/* Notifier chain MUST detach us all upper devices. */
+		WARN_ON(netdev_has_any_upper_dev(dev));
+		WARN_ON(netdev_has_any_lower_dev(dev));
+
+		/* Remove entries from kobject tree */
+		netdev_unregister_kobject(dev);
+#ifdef CONFIG_XPS
+		/* Remove XPS queueing entries */
+		netif_reset_xps_queues_gt(dev, 0);
+#endif
+	}
+
+	synchronize_net();
+
+	list_for_each_entry(dev, head, unreg_list) {
+		dev_put(dev);
+		net_set_todo(dev);
+	}
+}
+
 /**
  *	unregister_netdev - remove device from the kernel
  *	@dev: device
-- 
2.26.2

[PATCH net-next v2 4/4] net: inline rollback_registered_many()

[Jakub Kicinski]

Similar to the change for rollback_registered() -
rollback_registered_many() was a part of unregister_netdevice_many()
minus the net_set_todo(), which is no longer needed.

Functionally this patch moves the list_empty() check back after:

	BUG_ON(dev_boot_phase);
	ASSERT_RTNL();

but I can't find any reason why that would be an issue.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a7841d03c910..dee6488f8a31 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5709,7 +5709,7 @@ static void flush_all_backlogs(void)
 	}
 
 	/* we can have in flight packet[s] on the cpus we are not flushing,
-	 * synchronize_net() in rollback_registered_many() will take care of
+	 * synchronize_net() in unregister_netdevice_many() will take care of
 	 * them
 	 */
 	for_each_cpu(cpu, &flush_cpus)
@@ -10605,8 +10605,6 @@ void synchronize_net(void)
 }
 EXPORT_SYMBOL(synchronize_net);
 
-static void rollback_registered_many(struct list_head *head);
-
 /**
  *	unregister_netdevice_queue - remove device from the kernel
  *	@dev: device
@@ -10630,8 +10628,7 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
 		LIST_HEAD(single);
 
 		list_add(&dev->unreg_list, &single);
-		rollback_registered_many(&single);
-		list_del(&single);
+		unregister_netdevice_many(&single);
 	}
 }
 EXPORT_SYMBOL(unregister_netdevice_queue);
@@ -10644,15 +10641,6 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
  *  we force a list_del() to make sure stack wont be corrupted later.
  */
 void unregister_netdevice_many(struct list_head *head)
-{
-	if (!list_empty(head)) {
-		rollback_registered_many(head);
-		list_del(head);
-	}
-}
-EXPORT_SYMBOL(unregister_netdevice_many);
-
-static void rollback_registered_many(struct list_head *head)
 {
 	struct net_device *dev, *tmp;
 	LIST_HEAD(close_head);
@@ -10660,6 +10648,9 @@ static void rollback_registered_many(struct list_head *head)
 	BUG_ON(dev_boot_phase);
 	ASSERT_RTNL();
 
+	if (list_empty(head))
+		return;
+
 	list_for_each_entry_safe(dev, tmp, head, unreg_list) {
 		/* Some devices call without registering
 		 * for initialization unwind. Remove those
@@ -10743,7 +10734,10 @@ static void rollback_registered_many(struct list_head *head)
 		dev_put(dev);
 		net_set_todo(dev);
 	}
+
+	list_del(head);
 }
+EXPORT_SYMBOL(unregister_netdevice_many);
 
 /**
  *	unregister_netdev - remove device from the kernel
-- 
2.26.2

Re: [PATCH net-next v2 0/4] net: inline rollback_registered() functions

[Edwin Peer]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Tue, Jan 19, 2021 at 12:32 PM Jakub Kicinski <kuba@kernel.org> wrote:

> After recent changes to the error path of register_netdevice()
> we no longer need a version of unregister_netdevice_many() which
> does not set net_todo. We can inline the rollback_registered()
> functions into respective unregister_netdevice() calls.
>
> v2: - add missing list_del() in the last patch
>
> Jakub Kicinski (4):
>   net: move net_set_todo inside rollback_registered()
>   net: inline rollback_registered()
>   net: move rollback_registered_many()
>   net: inline rollback_registered_many()
>
>  net/core/dev.c | 210 +++++++++++++++++++++++--------------------------
>  1 file changed, 98 insertions(+), 112 deletions(-)

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Regards,
Edwin Peer

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4160 bytes --]

Re: [PATCH net-next v2 0/4] net: inline rollback_registered() functions

[Jakub Kicinski]

On Tue, 19 Jan 2021 15:18:46 -0800 Edwin Peer wrote:
> On Tue, Jan 19, 2021 at 12:32 PM Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > After recent changes to the error path of register_netdevice()
> > we no longer need a version of unregister_netdevice_many() which
> > does not set net_todo. We can inline the rollback_registered()
> > functions into respective unregister_netdevice() calls.
> >
> > v2: - add missing list_del() in the last patch
> >
> > Jakub Kicinski (4):
> >   net: move net_set_todo inside rollback_registered()
> >   net: inline rollback_registered()
> >   net: move rollback_registered_many()
> >   net: inline rollback_registered_many()
> >
> >  net/core/dev.c | 210 +++++++++++++++++++++++--------------------------
> >  1 file changed, 98 insertions(+), 112 deletions(-)  
> 
> Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Applied, thanks!

Re: [PATCH net-next v2 0/4] net: inline rollback_registered() functions

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 19 Jan 2021 12:25:17 -0800 you wrote:
> After recent changes to the error path of register_netdevice()
> we no longer need a version of unregister_netdevice_many() which
> does not set net_todo. We can inline the rollback_registered()
> functions into respective unregister_netdevice() calls.
> 
> v2: - add missing list_del() in the last patch
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/4] net: move net_set_todo inside rollback_registered()
    https://git.kernel.org/netdev/net-next/c/2014beea7eb1
  - [net-next,v2,2/4] net: inline rollback_registered()
    https://git.kernel.org/netdev/net-next/c/037e56bd965e
  - [net-next,v2,3/4] net: move rollback_registered_many()
    https://git.kernel.org/netdev/net-next/c/bcfe2f1a3818
  - [net-next,v2,4/4] net: inline rollback_registered_many()
    https://git.kernel.org/netdev/net-next/c/0cbe1e57a7b9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html