[PATCH net-next v2 0/3] netconsole: Populate dynamic entry even if netpoll fails

[PATCH net-next v2 0/3] netconsole: Populate dynamic entry even if netpoll fails

[Breno Leitao]

The current implementation of netconsole removes the entry and fails
entirely if netpoll fails to initialize. This approach is suboptimal, as
it prevents reconfiguration or re-enabling of the target through
configfs.

While this issue might seem minor if it were rare, it actually occurs
frequently when the network module is configured as a loadable module.

In such cases, the network is unavailable when netconsole initializes,
causing netpoll to fail. This failure forces users to reconfigure the
target from scratch, discarding any settings provided via the command
line.

The proposed change would keep the target available in configfs, albeit
in a disabled state. This modification allows users to adjust settings
or simply re-enable the target once the network module has loaded,
providing a more flexible and user-friendly solution.

Changelog:

v2:
  * Avoid late cleanup, and always returning an np in a clear slate when
    failing (Paolo)
  * Added another commit to log (pr_err) when netconsole doesn't fail,
    avoiding silent failures.

v1:
  * https://lore.kernel.org/all/20240809161935.3129104-1-leitao@debian.org/

Breno Leitao (3):
  netpoll: Ensure clean state on setup failures
  netconsole: pr_err() when netpoll_setup fails
  netconsole: Populate dynamic entry even if netpoll fails

 drivers/net/netconsole.c | 17 +++++++++++++----
 net/core/netpoll.c       | 16 +++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

-- 
2.43.5

[PATCH net-next v2 1/3] netpoll: Ensure clean state on setup failures

[Breno Leitao]

Modify netpoll_setup() and __netpoll_setup() to ensure that the netpoll
structure (np) is left in a clean state if setup fails for any reason.
This prevents carrying over misconfigured fields in case of partial
setup success.

Key changes:
- np->dev is now set only after successful setup, ensuring it's always
  NULL if netpoll is not configured or if netpoll_setup() fails.
- np->local_ip is zeroed if netpoll setup doesn't complete successfully.
- Added DEBUG_NET_WARN_ON_ONCE() checks to catch unexpected states.
- Reordered some operations in __netpoll_setup() for better logical flow.

These changes improve the reliability of netpoll configuration, since it
assures that the structure is fully initialized or totally unset.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index a58ea724790c..c5577d250a21 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -626,12 +626,10 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
-	np->dev = ndev;
-	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
-
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
-		       np->dev_name);
+		       ndev->name);
 		err = -ENOTSUPP;
 		goto out;
 	}
@@ -649,7 +647,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 
 		refcount_set(&npinfo->refcnt, 1);
 
-		ops = np->dev->netdev_ops;
+		ops = ndev->netdev_ops;
 		if (ops->ndo_netpoll_setup) {
 			err = ops->ndo_netpoll_setup(ndev, npinfo);
 			if (err)
@@ -660,6 +658,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 		refcount_inc(&npinfo->refcnt);
 	}
 
+	np->dev = ndev;
+	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 	npinfo->netpoll = np;
 
 	/* last thing to do is link it to the net device structure */
@@ -677,6 +677,7 @@ EXPORT_SYMBOL_GPL(__netpoll_setup);
 int netpoll_setup(struct netpoll *np)
 {
 	struct net_device *ndev = NULL;
+	bool ip_overwritten = false;
 	struct in_device *in_dev;
 	int err;
 
@@ -740,6 +741,7 @@ int netpoll_setup(struct netpoll *np)
 				goto put;
 			}
 
+			ip_overwritten = true;
 			np->local_ip.ip = ifa->ifa_local;
 			np_info(np, "local IP %pI4\n", &np->local_ip.ip);
 		} else {
@@ -757,6 +759,7 @@ int netpoll_setup(struct netpoll *np)
 					    !!(ipv6_addr_type(&np->remote_ip.in6) & IPV6_ADDR_LINKLOCAL))
 						continue;
 					np->local_ip.in6 = ifp->addr;
+					ip_overwritten = true;
 					err = 0;
 					break;
 				}
@@ -787,6 +790,9 @@ int netpoll_setup(struct netpoll *np)
 	return 0;
 
 put:
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
+	if (ip_overwritten)
+		memset(&np->local_ip, 0, sizeof(np->local_ip));
 	netdev_put(ndev, &np->dev_tracker);
 unlock:
 	rtnl_unlock();
-- 
2.43.5

Re: [PATCH net-next v2 1/3] netpoll: Ensure clean state on setup failures

[Jakub Kicinski]

On Mon, 19 Aug 2024 03:36:11 -0700 Breno Leitao wrote:
> +	DEBUG_NET_WARN_ON_ONCE(np->dev);
>  	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
>  		np_err(np, "%s doesn't support polling, aborting\n",
> -		       np->dev_name);
> +		       ndev->name);
>  		err = -ENOTSUPP;
>  		goto out;
>  	}

>  put:
> +	DEBUG_NET_WARN_ON_ONCE(np->dev);

nit: having two warnings feels a bit excessive, let's pick one location?

Re: [PATCH net-next v2 1/3] netpoll: Ensure clean state on setup failures

[Breno Leitao]

Hello Jakub,

On Tue, Aug 20, 2024 at 04:20:10PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Aug 2024 03:36:11 -0700 Breno Leitao wrote:
> > +	DEBUG_NET_WARN_ON_ONCE(np->dev);
> >  	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
> >  		np_err(np, "%s doesn't support polling, aborting\n",
> > -		       np->dev_name);
> > +		       ndev->name);
> >  		err = -ENOTSUPP;
> >  		goto out;
> >  	}
> 
> >  put:
> > +	DEBUG_NET_WARN_ON_ONCE(np->dev);
> 
> nit: having two warnings feels a bit excessive, let's pick one location?

Sure, I think it might be better to keep it in the fail (put:) case, so,
we make sure that netpoll is in a clear state in the failure path.

Thanks
--breno

[PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup fails

[Breno Leitao]

netpoll_setup() can fail in several ways, some of which print an error
message, while others simply return without any message. For example,
__netpoll_setup() returns in a few places without printing anything.

To address this issue, modify the code to print an error message on
netconsole if the target is not enabled. This will help us identify and
troubleshoot netcnsole issues related to netpoll setup failures
more easily.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 72384c1ecc5c..9b5f605fe87a 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -414,8 +414,10 @@ static ssize_t enabled_store(struct config_item *item,
 		netpoll_print_options(&nt->np);
 
 		ret = netpoll_setup(&nt->np);
-		if (ret)
+		if (ret) {
+			pr_err("Not enabling netconsole. Netpoll setup failed\n");
 			goto out_unlock;
+		}
 
 		nt->enabled = true;
 		pr_info("network logging started\n");
-- 
2.43.5

Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup fails

[Jakub Kicinski]

On Mon, 19 Aug 2024 03:36:12 -0700 Breno Leitao wrote:
> netpoll_setup() can fail in several ways, some of which print an error
> message, while others simply return without any message. For example,
> __netpoll_setup() returns in a few places without printing anything.
> 
> To address this issue, modify the code to print an error message on
> netconsole if the target is not enabled. This will help us identify and
> troubleshoot netcnsole issues related to netpoll setup failures
> more easily.

Only if memory allocation fails, it seems, and memory allocation
failures with GFP_KERNEL will be quite noisy.

BTW I looked thru 4 random implementations of ndo_netpoll_setup
and they look almost identical :S Perhaps they can be refactored?

Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup fails

[Breno Leitao]

On Tue, Aug 20, 2024 at 04:24:09PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Aug 2024 03:36:12 -0700 Breno Leitao wrote:
> > netpoll_setup() can fail in several ways, some of which print an error
> > message, while others simply return without any message. For example,
> > __netpoll_setup() returns in a few places without printing anything.
> > 
> > To address this issue, modify the code to print an error message on
> > netconsole if the target is not enabled. This will help us identify and
> > troubleshoot netcnsole issues related to netpoll setup failures
> > more easily.
> 
> Only if memory allocation fails, it seems, and memory allocation
> failures with GFP_KERNEL will be quite noisy.

Or anything that fails in ->ndo_netpoll_setup() and doesn't print
anything else.

Do you think this is useless?

> BTW I looked thru 4 random implementations of ndo_netpoll_setup
> and they look almost identical :S Perhaps they can be refactored?

correct.  This should be refactored.

In fact, since you opened this topic, there are a few things that also
come to my mind

1) Possible reduce refill_skb() work in the critical path (UDP send
path), moving it to a workqueue?

When sending a message, netpoll tries fill the whole skb poll, and then try to
allocate a new skb before sending the packet. 

netconsole needs to write a message, which calls netpoll_send_udp()

	send_ext_msg_udp() {
		netpoll_send_udp() {
			refill_skbs() {
				while (skb_pool.qlen < MAX_SKBS) {
					skb = alloc_skb(MAX_SKB_SIZE, GFP_ATOMIC);
				}
			}
			skb = alloc_skb(len, GFP_ATOMIC);
				if (!skb)
					skb = skb_dequeue(&skb_pool);
			}
		}
	}
		
Would it be better if the hot path just get one of the skbs from the
pool, and refill it in a workqueue? If the skb_poll() is empty, then
alloc_skb(len, GFP_ATOMIC) !?


2) Report statistic back from netpoll_send_udp(). netpoll_send_skb()
return values are being discarded, so, it is hard to know if the packet
was transmitted or got something as NET_XMIT_DROP, NETDEV_TX_BUSY,
NETDEV_TX_OK.

It is unclear where this should be reported two. Maybe a configfs entry?

Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup fails

[Jakub Kicinski]

On Wed, 21 Aug 2024 01:41:55 -0700 Breno Leitao wrote:
> On Tue, Aug 20, 2024 at 04:24:09PM -0700, Jakub Kicinski wrote:
> > On Mon, 19 Aug 2024 03:36:12 -0700 Breno Leitao wrote:  
> > > netpoll_setup() can fail in several ways, some of which print an error
> > > message, while others simply return without any message. For example,
> > > __netpoll_setup() returns in a few places without printing anything.
> > > 
> > > To address this issue, modify the code to print an error message on
> > > netconsole if the target is not enabled. This will help us identify and
> > > troubleshoot netcnsole issues related to netpoll setup failures
> > > more easily.  
> > 
> > Only if memory allocation fails, it seems, and memory allocation
> > failures with GFP_KERNEL will be quite noisy.  
> 
> Or anything that fails in ->ndo_netpoll_setup() and doesn't print
> anything else.

Which also only fails because of memory allocation AFAICT.

> Do you think this is useless?

I think it's better to push up more precise message into the fail sites.

> > BTW I looked thru 4 random implementations of ndo_netpoll_setup
> > and they look almost identical :S Perhaps they can be refactored?  
> 
> correct.  This should be refactored.
> 
> In fact, since you opened this topic, there are a few things that also
> come to my mind
> 
> 1) Possible reduce refill_skb() work in the critical path (UDP send
> path), moving it to a workqueue?
> 
> When sending a message, netpoll tries fill the whole skb poll, and then try to
> allocate a new skb before sending the packet. 
> 
> netconsole needs to write a message, which calls netpoll_send_udp()
> 
> 	send_ext_msg_udp() {
> 		netpoll_send_udp() {
> 			refill_skbs() {
> 				while (skb_pool.qlen < MAX_SKBS) {
> 					skb = alloc_skb(MAX_SKB_SIZE, GFP_ATOMIC);
> 				}
> 			}
> 			skb = alloc_skb(len, GFP_ATOMIC);
> 				if (!skb)
> 					skb = skb_dequeue(&skb_pool);
> 			}
> 		}
> 	}
> 		
> Would it be better if the hot path just get one of the skbs from the
> pool, and refill it in a workqueue? If the skb_poll() is empty, then
> alloc_skb(len, GFP_ATOMIC) !?

Yeah, that seems a bit odd. If you can't find anything in the history
that would explain this design - refactoring SG.

> 2) Report statistic back from netpoll_send_udp(). netpoll_send_skb()
> return values are being discarded, so, it is hard to know if the packet
> was transmitted or got something as NET_XMIT_DROP, NETDEV_TX_BUSY,
> NETDEV_TX_OK.
> 
> It is unclear where this should be reported two. Maybe a configfs entry?

Also sounds good. We don't use configfs much in networking so IDK if
it's okay to use it for stats. But no other obviously better place
comes to mind for me.

Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup fails

[Breno Leitao]

Hello Jakub,

On Wed, Aug 21, 2024 at 03:54:04PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Aug 2024 01:41:55 -0700 Breno Leitao wrote:

> > Do you think this is useless?
> 
> I think it's better to push up more precise message into the fail sites.

Makese sense, I will remove it, and add the failing message once we
refactor ndo_netpoll_setup() callbacks.

> > Would it be better if the hot path just get one of the skbs from the
> > pool, and refill it in a workqueue? If the skb_poll() is empty, then
> > alloc_skb(len, GFP_ATOMIC) !?
> 
> Yeah, that seems a bit odd. If you can't find anything in the history
> that would explain this design - refactoring SG.

Thanks. I will add it to my todo list.

> > 2) Report statistic back from netpoll_send_udp(). netpoll_send_skb()
> > return values are being discarded, so, it is hard to know if the packet
> > was transmitted or got something as NET_XMIT_DROP, NETDEV_TX_BUSY,
> > NETDEV_TX_OK.
> > 
> > It is unclear where this should be reported two. Maybe a configfs entry?
> 
> Also sounds good. We don't use configfs much in networking so IDK if
> it's okay to use it for stats. But no other obviously better place
> comes to mind for me.

Exactly, configfs seems a bit weird, but, at the same time, I don't have
a better idea. Let me send a patch for this one, and we can continue the
discussion over there.

Thanks
--breno

[PATCH net-next v2 3/3] netconsole: Populate dynamic entry even if netpoll fails

[Breno Leitao]

Currently, netconsole discards targets that fail during initialization,
causing two issues:

1) Inconsistency between target list and configfs entries
  * user pass cmdline0, cmdline1. If cmdline0 fails, then cmdline1
    becomes cmdline0 in configfs.

2) Inability to manage failed targets from userspace
  * If user pass a target that fails with netpoll (interface not loaded at
    netcons initialization time, such as interface is a module), then
    the target will not exist in the configfs, so, user cannot re-enable
    or modify it from userspace.

Failed targets are now added to the target list and configfs, but
remain disabled until manually enabled or reconfigured. This change does
not change the behaviour if CONFIG_NETCONSOLE_DYNAMIC is not set.

CC: Aijay Adams <aijay@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 9b5f605fe87a..82e178b34e4b 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1260,11 +1260,18 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 		goto fail;
 
 	err = netpoll_setup(&nt->np);
-	if (err)
-		goto fail;
+	if (!err) {
+		nt->enabled = true;
+	} else {
+		pr_err("Not enabling netconsole. Netpoll setup failed\n");
+		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;
 
-- 
2.43.5

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

[Jakub Kicinski]

On Mon, 19 Aug 2024 03:36:13 -0700 Breno Leitao wrote:
> -	if (err)
> -		goto fail;
> +	if (!err) {
> +		nt->enabled = true;
> +	} else {
> +		pr_err("Not enabling netconsole. Netpoll setup failed\n");
> +		if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> +			/* only fail if dynamic reconfiguration is set,
> +			 * otherwise, keep the target in the list, but disabled.
> +			 */
> +			goto fail;
> +	}

This will be better written as:

	if (err) {
		/* handle err */
	}

	nt->enabled = true;

As for the message would it be more helpful to indicate target will be
disabled? Move the print after the check for dynamic and say "Netpoll
setup failed, netconsole target will be disabled" ?

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

[Breno Leitao]

Hello Jakub,

On Tue, Aug 20, 2024 at 04:27:25PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Aug 2024 03:36:13 -0700 Breno Leitao wrote:
> > -	if (err)
> > -		goto fail;
> > +	if (!err) {
> > +		nt->enabled = true;
> > +	} else {
> > +		pr_err("Not enabling netconsole. Netpoll setup failed\n");
> > +		if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> > +			/* only fail if dynamic reconfiguration is set,
> > +			 * otherwise, keep the target in the list, but disabled.
> > +			 */
> > +			goto fail;
> > +	}
> 
> This will be better written as:
> 
> 	if (err) {
> 		/* handle err */
> 	}
> 
> 	nt->enabled = true;

I tried to do something like this, but, I was not able to come up with
something like this.

There are three cases we need to check here:

netpoll_setup returned 0:
	nt->enabled = true           				(1)
netpoll_setup returned !=0
	if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)
		continue with nt->enabled disabeld  		(2)
	if IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC) is disabld:
		goto fail.					(3)


The cases are:

1) Everything is fine
2) netpoll failed, but we want to keep configfs enabled
3) netpoll failed and we don't care about configfs.

Another way to write this is:

        err = netpoll_setup(&nt->np);
        if (err) {
                pr_err("Not enabling netconsole. Netpoll setup failed\n");
                if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
                        goto fail
        } else {
                nt->enabled = true;
        }

is it better? Or, Is there a even better way to write this?
	
> As for the message would it be more helpful to indicate target will be
> disabled? Move the print after the check for dynamic and say "Netpoll
> setup failed, netconsole target will be disabled" ?

In both cases the target will be disabled, right? In one case, it will
populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set),
otherwise it will fail completely. Either way, netconsole will be
disabled.

Let me know if I am missing something here.

Thanks for the review,
--breno

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

[Jakub Kicinski]

On Wed, 21 Aug 2024 01:21:58 -0700 Breno Leitao wrote:
> Another way to write this is:
> 
>         err = netpoll_setup(&nt->np);
>         if (err) {
>                 pr_err("Not enabling netconsole. Netpoll setup failed\n");
>                 if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
>                         goto fail
>         } else {
>                 nt->enabled = true;
>         }
> 
> is it better? Or, Is there a even better way to write this?

Yes, I think this is better! Or at least I wouldn't have made the same
mistake reading it if it was written this way :)

> > As for the message would it be more helpful to indicate target will be
> > disabled? Move the print after the check for dynamic and say "Netpoll
> > setup failed, netconsole target will be disabled" ?  
> 
> In both cases the target will be disabled, right? In one case, it will
> populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set),
> otherwise it will fail completely. Either way, netconsole will be
> disabled.

No strong feelings. I was trying to highlight that it's a single target
that ends up being disabled "netconsole disabled" sounds like the whole
netconsole module is completely out of commission.

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

[Breno Leitao]

Hello Jakub,

On Wed, Aug 21, 2024 at 03:49:26PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Aug 2024 01:21:58 -0700 Breno Leitao wrote:
> > Another way to write this is:
> > 
> >         err = netpoll_setup(&nt->np);
> >         if (err) {
> >                 pr_err("Not enabling netconsole. Netpoll setup failed\n");
> >                 if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
> >                         goto fail
> >         } else {
> >                 nt->enabled = true;
> >         }
> > 
> > is it better? Or, Is there a even better way to write this?
> 
> Yes, I think this is better! Or at least I wouldn't have made the same
> mistake reading it if it was written this way :)
> 
> > > As for the message would it be more helpful to indicate target will be
> > > disabled? Move the print after the check for dynamic and say "Netpoll
> > > setup failed, netconsole target will be disabled" ?  
> > 
> > In both cases the target will be disabled, right? In one case, it will
> > populate the cmdline0 configfs (if CONFIG_NETCONSOLE_DYNAMIC is set),
> > otherwise it will fail completely. Either way, netconsole will be
> > disabled.
> 
> No strong feelings. I was trying to highlight that it's a single target
> that ends up being disabled "netconsole disabled" sounds like the whole
> netconsole module is completely out of commission.

That is fair, let me print the cmdline number, so, we can see something
as:

	netpoll: netconsole: local port 6666
	netpoll: netconsole: local IPv6 address 2401:db00:3120:21a9:face:0:270:0
	netpoll: netconsole: interface 'ethX'
	netpoll: netconsole: remote port 1514
	netpoll: netconsole: remote IPv6 address 2803:6080:a89c:a670::1
	netpoll: netconsole: remote ethernet address 02:90:fb:66:aa:e5
	netpoll: netconsole: ethX doesn't exist, aborting
	netconsole: Not enabling netconsole for cmdline0. Netpoll setup failed