Re: [PATCH net-next] net: netconsole: Populate dynamic entry even if netpoll fails

[Breno Leitao <leitao@debian.org>]

Hello Paolo,

On Tue, Aug 13, 2024 at 01:55:27PM +0200, Paolo Abeni wrote:
> On 8/9/24 18:19, Breno Leitao wrote:> @@ -1304,8 +1308,6 @@ static int
> __init init_netconsole(void)
> >   		while ((target_config = strsep(&input, ";"))) {
> >   			nt = alloc_param_target(target_config, count);
> >   			if (IS_ERR(nt)) {
> > -				if (IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> > -					continue;
> >   				err = PTR_ERR(nt);
> >   				goto fail;
> >   			}

First of all, thanks for the in-depth review and suggestion.


> AFAICS the above introduces a behavior change: if CONFIG_NETCONSOLE_DYNAMIC
> is enabled, and the options parsing fails for any targets in the command
> line, all the targets will be removed.
> 
> I think the old behavior is preferable - just skip the targets with wrong
> options.

Thinking about it again, and I think I agree with you here. I will
update.

> Side note: I think the error paths in __netpoll_setup() assume the struct
> netpoll will be freed in case of error, e.g. the device refcount is released
> but np->dev is not cleared, I fear we could hit a reference underflow on
> <setup error>, <disable>

That is a good catch, and I was even able to simulate it, by forcing two
errors:

Something as:

--- a/net/core/netpoll.c
	+++ b/net/core/netpoll.c
	@@ -637,7 +637,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
		}

		if (!ndev->npinfo) {
	-               npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
	+               npinfo = NULL;

	diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
	index 41a61fa88c32..2c6190808e75 100644
	--- a/drivers/net/netconsole.c
	+++ b/drivers/net/netconsole.c
	@@ -1327,12 +1330,17 @@ static int __init init_netconsole(void)
		}

		err = dynamic_netconsole_init();
	+       err = 1;
		if (err)
			goto undonotifier;

	
This caused the following issue:
	
	[   19.530831] netconsole: network logging stopped on interface eth0 as it unregistered
	[   19.531205] ref_tracker: reference already released.
	[   19.531426] ref_tracker: allocated in:
	[   19.531505]  netpoll_setup+0xfd/0x7f0
	[   19.531505]  init_netconsole+0x300/0x960
	....
	[   19.534532] ------------[ cut here ]------------
	[   19.534784] WARNING: CPU: 5 PID: 1 at lib/ref_tracker.c:255 ref_tracker_free+0x4e5/0x740
	[   19.535103] Modules linked in:
	....
	[   19.542116]  ? ref_tracker_free+0x4e5/0x740
	[   19.542286]  ? refcount_inc+0x40/0x40
	[   19.542571]  ? do_netpoll_cleanup+0x4e/0xb0
	[   19.542752]  ? netconsole_process_cleanups_core+0xcd/0x260
	[   19.542961]  ? netconsole_netdev_event+0x3ab/0x3e0
	[   19.543199]  ? unregister_netdevice_notifier+0x27c/0x2f0
	[   19.543456]  ? init_netconsole+0xe4/0x960
	[   19.543615]  ? do_one_initcall+0x1a8/0x5d0
	[   19.543764]  ? do_initcall_level+0x133/0x1e0
	[   19.543963]  ? do_initcalls+0x43/0x80
	....

That said, now, the list contains enabled and disabled targets. All the
disable targets have netpoll disabled, thus, we don't handle network
operations in the disabled targets.

This is my new proposal, based on your feedback, how does it look like?

Thanks for the in-depth review,
--breno

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 30b6aac08411..60325383ab6d 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1008,6 +1008,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	mutex_lock(&target_cleanup_list_lock);
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
+		if (!nt->enabled)
+			continue;
 		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
 			switch (event) {
@@ -1258,11 +1260,15 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 		goto fail;
 
 	err = netpoll_setup(&nt->np);
-	if (err)
+	if (!err)
+		nt->enabled = true;
+	else if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
+		/* only fail if dynamic reconfiguration is set,
+		 * otherwise, keep the target in the list, but disabled.
+		 */
 		goto fail;
 
 	populate_configfs_item(nt, cmdline_count);
-	nt->enabled = true;
 
 	return nt;
 
@@ -1274,7 +1280,8 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 /* Cleanup netpoll for given target (from boot/module param) and free it */
 static void free_param_target(struct netconsole_target *nt)
 {
-	netpoll_cleanup(&nt->np);
+	if (nt->enabled)
+		netpoll_cleanup(&nt->np);
 	kfree(nt);
 }