[PATCH net-next v2 0/5] netconsole: reuse netpoll_parse_ip_addr in configfs helpers

[PATCH net-next v2 0/5] netconsole: reuse netpoll_parse_ip_addr in configfs helpers

[Breno Leitao]

This patchset refactors the IP address parsing logic in the netconsole
driver to eliminate code duplication and improve maintainability. The
changes centralize IPv4 and IPv6 address parsing into a single function
(netpoll_parse_ip_addr). For that, it needs to teach
netpoll_parse_ip_addr() to handle strings with newlines, which is the
type of string coming from configfs.

Background

The netconsole driver currently has duplicate IP address parsing logic
in both local_ip_store() and remote_ip_store() functions. This
duplication increases the risk of inconsistencies and makes the code
harder to maintain.

Benefits

* Reduced code duplication: ~40 lines of duplicate parsing logic eliminated
 * Improved robustness: Centralized parsing reduces the chance of inconsistencies
 * Easier to maintain: Code follow more the netdev way

PS: The patches are very well contained in other to help review.

---
Changes in v2:
- Moved the netpoll_parse_ip_addr() to outside the dynamic block (Jakub)
- Link to v1: https://lore.kernel.org/r/20250718-netconsole_ref-v1-0-86ef253b7a7a@debian.org

---
Breno Leitao (5):
      netpoll: Remove unused fields from inet_addr union
      netconsole: move netpoll_parse_ip_addr() earlier for reuse
      netconsole: add support for strings with new line in netpoll_parse_ip_addr
      netconsole: use netpoll_parse_ip_addr in local_ip_store
      netconsole: use netpoll_parse_ip_addr in local_ip_store

 drivers/net/netconsole.c | 85 ++++++++++++++++++------------------------------
 include/linux/netpoll.h  |  3 --
 2 files changed, 31 insertions(+), 57 deletions(-)
---
base-commit: d61f6cb6f6ef3c70d2ccc0d9c85c508cb8017da9
change-id: 20250718-netconsole_ref-c1f7254cfb51

Best regards,
--  
Breno Leitao <leitao@debian.org>

[PATCH net-next v2 1/5] netpoll: Remove unused fields from inet_addr union

[Breno Leitao]

Clean up the inet_addr union by removing unused fields that are
redundant with existing members:

This simplifies the union structure while maintaining all necessary
functionality for both IPv4 and IPv6 address handling.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 include/linux/netpoll.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 735e65c3cc114..b5ea9882eda8b 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -15,10 +15,7 @@
 #include <linux/refcount.h>
 
 union inet_addr {
-	__u32		all[4];
 	__be32		ip;
-	__be32		ip6[4];
-	struct in_addr	in;
 	struct in6_addr	in6;
 };
 

-- 
2.47.1

[PATCH net-next v2 2/5] netconsole: move netpoll_parse_ip_addr() earlier for reuse

[Breno Leitao]

Move netpoll_parse_ip_addr() earlier in the file to be reused in
other functions, such as local_ip_store(). This avoids duplicate
address parsing logic and centralizes validation for both IPv4
and IPv6 string input.

No functional changes intended.

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index e3722de08ea9f..8d1b93264e0fd 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -300,6 +300,26 @@ static void netconsole_print_banner(struct netpoll *np)
 	np_info(np, "remote ethernet address %pM\n", np->remote_mac);
 }
 
+static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
+{
+	const char *end;
+
+	if (!strchr(str, ':') &&
+	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
+		if (!*end)
+			return 0;
+	}
+	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
+#if IS_ENABLED(CONFIG_IPV6)
+		if (!*end)
+			return 1;
+#else
+		return -1;
+#endif
+	}
+	return -1;
+}
+
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 
 /*
@@ -1742,26 +1762,6 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
 
-static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
-{
-	const char *end;
-
-	if (!strchr(str, ':') &&
-	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
-		if (!*end)
-			return 0;
-	}
-	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
-#if IS_ENABLED(CONFIG_IPV6)
-		if (!*end)
-			return 1;
-#else
-		return -1;
-#endif
-	}
-	return -1;
-}
-
 static int netconsole_parser_cmdline(struct netpoll *np, char *opt)
 {
 	bool ipversion_set = false;

-- 
2.47.1

[PATCH net-next v2 3/5] netconsole: add support for strings with new line in netpoll_parse_ip_addr

[Breno Leitao]

The current IP address parsing logic fails when the input string
contains a trailing newline character. This can occur when IP
addresses are provided through configfs, which contains newlines in
a const buffer.

Teach netpoll_parse_ip_addr() how to ignore newlines at the end of the
IPs. Also, simplify the code by:

 * No need to check for separators. Try to parse ipv4, if it fails try
   ipv6 similarly to ceph_pton()
 * If ipv6 is not supported, don't call in6_pton() at all.

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 8d1b93264e0fd..f2c2b8852c603 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -303,20 +303,21 @@ static void netconsole_print_banner(struct netpoll *np)
 static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
 {
 	const char *end;
+	int len;
 
-	if (!strchr(str, ':') &&
-	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
-		if (!*end)
-			return 0;
-	}
-	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
-#if IS_ENABLED(CONFIG_IPV6)
-		if (!*end)
-			return 1;
-#else
+	len = strlen(str);
+	if (!len)
 		return -1;
+
+	if (str[len - 1] == '\n')
+		len -= 1;
+
+	if (in4_pton(str, len, (void *)addr, -1, &end) > 0)
+		return 0;
+#if IS_ENABLED(CONFIG_IPV6)
+	if (in6_pton(str, len, addr->in6.s6_addr, -1, &end) > 0)
+		return 1;
 #endif
-	}
 	return -1;
 }
 

-- 
2.47.1

[PATCH net-next v2 4/5] netconsole: use netpoll_parse_ip_addr in local_ip_store

[Breno Leitao]

Replace manual IP address parsing with a call to netpoll_parse_ip_addr
in local_ip_store(), simplifying the code and reducing the chance of
errors.

Also, remove the pr_err() if the user enters an invalid value in
configfs entries. pr_err() is not the best way to alert user that the
configuration is invalid.

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index f2c2b8852c603..b24e423a60268 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -751,6 +751,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 {
 	struct netconsole_target *nt = to_target(item);
 	ssize_t ret = -EINVAL;
+	int ipv6;
 
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
@@ -759,23 +760,10 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	}
 
-	if (strnchr(buf, count, ':')) {
-		const char *end;
-
-		if (in6_pton(buf, count, nt->np.local_ip.in6.s6_addr, -1, &end) > 0) {
-			if (*end && *end != '\n') {
-				pr_err("invalid IPv6 address at: <%c>\n", *end);
-				goto out_unlock;
-			}
-			nt->np.ipv6 = true;
-		} else
-			goto out_unlock;
-	} else {
-		if (!nt->np.ipv6)
-			nt->np.local_ip.ip = in_aton(buf);
-		else
-			goto out_unlock;
-	}
+	ipv6 = netpoll_parse_ip_addr(buf, &nt->np.local_ip);
+	if (ipv6 == -1)
+		goto out_unlock;
+	nt->np.ipv6 = ipv6;
 
 	ret = strnlen(buf, count);
 out_unlock:

-- 
2.47.1

[PATCH net-next v2 5/5] netconsole: use netpoll_parse_ip_addr in local_ip_store

[Breno Leitao]

Replace manual IP address parsing with a call to netpoll_parse_ip_addr
in remote_ip_store(), simplifying the code and reducing the chance of
errors.

The error message got removed, since it is not a good practice to
pr_err() if used pass a wrong value in configfs.

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index b24e423a60268..7b394488384ea 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -776,6 +776,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 {
 	struct netconsole_target *nt = to_target(item);
 	ssize_t ret = -EINVAL;
+	int ipv6;
 
 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
@@ -784,23 +785,10 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	}
 
-	if (strnchr(buf, count, ':')) {
-		const char *end;
-
-		if (in6_pton(buf, count, nt->np.remote_ip.in6.s6_addr, -1, &end) > 0) {
-			if (*end && *end != '\n') {
-				pr_err("invalid IPv6 address at: <%c>\n", *end);
-				goto out_unlock;
-			}
-			nt->np.ipv6 = true;
-		} else
-			goto out_unlock;
-	} else {
-		if (!nt->np.ipv6)
-			nt->np.remote_ip.ip = in_aton(buf);
-		else
-			goto out_unlock;
-	}
+	ipv6 = netpoll_parse_ip_addr(buf, &nt->np.remote_ip);
+	if (ipv6 == -1)
+		goto out_unlock;
+	nt->np.ipv6 = ipv6;
 
 	ret = strnlen(buf, count);
 out_unlock:

-- 
2.47.1

Re: [PATCH net-next v2 4/5] netconsole: use netpoll_parse_ip_addr in local_ip_store

[Simon Horman]

On Mon, Jul 21, 2025 at 06:02:04AM -0700, Breno Leitao wrote:
> Replace manual IP address parsing with a call to netpoll_parse_ip_addr
> in local_ip_store(), simplifying the code and reducing the chance of
> errors.
> 
> Also, remove the pr_err() if the user enters an invalid value in
> configfs entries. pr_err() is not the best way to alert user that the
> configuration is invalid.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

My suggestion below not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/netconsole.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index f2c2b8852c603..b24e423a60268 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -751,6 +751,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
>  {
>  	struct netconsole_target *nt = to_target(item);
>  	ssize_t ret = -EINVAL;
> +	int ipv6;
>  
>  	mutex_lock(&dynamic_netconsole_mutex);
>  	if (nt->enabled) {
> @@ -759,23 +760,10 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
>  		goto out_unlock;
>  	}
>  
> -	if (strnchr(buf, count, ':')) {
> -		const char *end;
> -
> -		if (in6_pton(buf, count, nt->np.local_ip.in6.s6_addr, -1, &end) > 0) {
> -			if (*end && *end != '\n') {
> -				pr_err("invalid IPv6 address at: <%c>\n", *end);
> -				goto out_unlock;
> -			}
> -			nt->np.ipv6 = true;
> -		} else
> -			goto out_unlock;
> -	} else {
> -		if (!nt->np.ipv6)
> -			nt->np.local_ip.ip = in_aton(buf);
> -		else
> -			goto out_unlock;
> -	}
> +	ipv6 = netpoll_parse_ip_addr(buf, &nt->np.local_ip);
> +	if (ipv6 == -1)
> +		goto out_unlock;
> +	nt->np.ipv6 = ipv6;

I don't think this needs to block progress.
And if you disagree that is fine too.
But I would have expressed this as:

	nt->np.ipv6 = !!ipv6;

Because nt->np.ipv6 is a bool and ipv6 is an int.

Likewise for patch 5/5.

...

Re: [PATCH net-next v2 3/5] netconsole: add support for strings with new line in netpoll_parse_ip_addr

[Simon Horman]

On Mon, Jul 21, 2025 at 06:02:03AM -0700, Breno Leitao wrote:
> The current IP address parsing logic fails when the input string
> contains a trailing newline character. This can occur when IP
> addresses are provided through configfs, which contains newlines in
> a const buffer.
> 
> Teach netpoll_parse_ip_addr() how to ignore newlines at the end of the
> IPs. Also, simplify the code by:
> 
>  * No need to check for separators. Try to parse ipv4, if it fails try
>    ipv6 similarly to ceph_pton()
>  * If ipv6 is not supported, don't call in6_pton() at all.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

My suggestion below not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/netconsole.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 8d1b93264e0fd..f2c2b8852c603 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -303,20 +303,21 @@ static void netconsole_print_banner(struct netpoll *np)
>  static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
>  {
>  	const char *end;
> +	int len;
>  
> -	if (!strchr(str, ':') &&
> -	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
> -		if (!*end)
> -			return 0;
> -	}
> -	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
> -#if IS_ENABLED(CONFIG_IPV6)
> -		if (!*end)
> -			return 1;
> -#else
> +	len = strlen(str);
> +	if (!len)
>  		return -1;
> +
> +	if (str[len - 1] == '\n')
> +		len -= 1;
> +
> +	if (in4_pton(str, len, (void *)addr, -1, &end) > 0)
> +		return 0;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	if (in6_pton(str, len, addr->in6.s6_addr, -1, &end) > 0)
> +		return 1;
>  #endif

I don't think it needs to block progress.
But FWIIW, I think it would be nice to increase
build coverage and express this as:

	if (IS_ENABLED(CONFIG_IPV6) &&
	    in6_pton(str, len, addr->in6.s6_addr, -1, &end) > 0)
		return 1;

Re: [PATCH net-next v2 5/5] netconsole: use netpoll_parse_ip_addr in local_ip_store

[Simon Horman]

On Mon, Jul 21, 2025 at 06:02:05AM -0700, Breno Leitao wrote:
> Replace manual IP address parsing with a call to netpoll_parse_ip_addr
> in remote_ip_store(), simplifying the code and reducing the chance of
> errors.
> 
> The error message got removed, since it is not a good practice to
> pr_err() if used pass a wrong value in configfs.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 1/5] netpoll: Remove unused fields from inet_addr union

[Simon Horman]

On Mon, Jul 21, 2025 at 06:02:01AM -0700, Breno Leitao wrote:
> Clean up the inet_addr union by removing unused fields that are
> redundant with existing members:
> 
> This simplifies the union structure while maintaining all necessary
> functionality for both IPv4 and IPv6 address handling.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 2/5] netconsole: move netpoll_parse_ip_addr() earlier for reuse

[Simon Horman]

On Mon, Jul 21, 2025 at 06:02:02AM -0700, Breno Leitao wrote:
> Move netpoll_parse_ip_addr() earlier in the file to be reused in
> other functions, such as local_ip_store(). This avoids duplicate
> address parsing logic and centralizes validation for both IPv4
> and IPv6 string input.
> 
> No functional changes intended.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 4/5] netconsole: use netpoll_parse_ip_addr in local_ip_store

[Breno Leitao]

Hello Simon

On Wed, Jul 23, 2025 at 03:54:00PM +0100, Simon Horman wrote:
> > @@ -759,23 +760,10 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
> >  		goto out_unlock;
> >  	}
> >  
> > -	if (strnchr(buf, count, ':')) {
> > -		const char *end;
> > -
> > -		if (in6_pton(buf, count, nt->np.local_ip.in6.s6_addr, -1, &end) > 0) {
> > -			if (*end && *end != '\n') {
> > -				pr_err("invalid IPv6 address at: <%c>\n", *end);
> > -				goto out_unlock;
> > -			}
> > -			nt->np.ipv6 = true;
> > -		} else
> > -			goto out_unlock;
> > -	} else {
> > -		if (!nt->np.ipv6)
> > -			nt->np.local_ip.ip = in_aton(buf);
> > -		else
> > -			goto out_unlock;
> > -	}
> > +	ipv6 = netpoll_parse_ip_addr(buf, &nt->np.local_ip);
> > +	if (ipv6 == -1)
> > +		goto out_unlock;
> > +	nt->np.ipv6 = ipv6;
> 
> I don't think this needs to block progress.
> And if you disagree that is fine too.
> But I would have expressed this as:
> 
> 	nt->np.ipv6 = !!ipv6;
> 
> Because nt->np.ipv6 is a bool and ipv6 is an int.
> 
> Likewise for patch 5/5.

Agree with the suggestion. I will update the patchset with your
suggestion.

Thank you very much for your review, again!
--breno

Re: [PATCH net-next v2 3/5] netconsole: add support for strings with new line in netpoll_parse_ip_addr

[Breno Leitao]

On Wed, Jul 23, 2025 at 03:54:11PM +0100, Simon Horman wrote:
> On Mon, Jul 21, 2025 at 06:02:03AM -0700, Breno Leitao wrote:

> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -303,20 +303,21 @@ static void netconsole_print_banner(struct netpoll *np)
> >  static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
> >  {
> >  	const char *end;
> > +	int len;
> >  
> > -	if (!strchr(str, ':') &&
> > -	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
> > -		if (!*end)
> > -			return 0;
> > -	}
> > -	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
> > -#if IS_ENABLED(CONFIG_IPV6)
> > -		if (!*end)
> > -			return 1;
> > -#else
> > +	len = strlen(str);
> > +	if (!len)
> >  		return -1;
> > +
> > +	if (str[len - 1] == '\n')
> > +		len -= 1;
> > +
> > +	if (in4_pton(str, len, (void *)addr, -1, &end) > 0)
> > +		return 0;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +	if (in6_pton(str, len, addr->in6.s6_addr, -1, &end) > 0)
> > +		return 1;
> >  #endif
> 
> I don't think it needs to block progress.
> But FWIIW, I think it would be nice to increase
> build coverage and express this as:

Agree. While testing with IPv6 disabled, the netcons selftest exploded,
so, this explose a bug in the selftest. This is now fixed in:

	https://lore.kernel.org/all/20250723-netcons_test_ipv6-v1-1-41c9092f93f9@debian.org/

Thanks for the review,
--breno

Re: [PATCH net-next v2 3/5] netconsole: add support for strings with new line in netpoll_parse_ip_addr

[Simon Horman]

On Wed, Jul 23, 2025 at 10:37:59AM -0700, Breno Leitao wrote:
> On Wed, Jul 23, 2025 at 03:54:11PM +0100, Simon Horman wrote:
> > On Mon, Jul 21, 2025 at 06:02:03AM -0700, Breno Leitao wrote:
> 
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -303,20 +303,21 @@ static void netconsole_print_banner(struct netpoll *np)
> > >  static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
> > >  {
> > >  	const char *end;
> > > +	int len;
> > >  
> > > -	if (!strchr(str, ':') &&
> > > -	    in4_pton(str, -1, (void *)addr, -1, &end) > 0) {
> > > -		if (!*end)
> > > -			return 0;
> > > -	}
> > > -	if (in6_pton(str, -1, addr->in6.s6_addr, -1, &end) > 0) {
> > > -#if IS_ENABLED(CONFIG_IPV6)
> > > -		if (!*end)
> > > -			return 1;
> > > -#else
> > > +	len = strlen(str);
> > > +	if (!len)
> > >  		return -1;
> > > +
> > > +	if (str[len - 1] == '\n')
> > > +		len -= 1;
> > > +
> > > +	if (in4_pton(str, len, (void *)addr, -1, &end) > 0)
> > > +		return 0;
> > > +#if IS_ENABLED(CONFIG_IPV6)
> > > +	if (in6_pton(str, len, addr->in6.s6_addr, -1, &end) > 0)
> > > +		return 1;
> > >  #endif
> > 
> > I don't think it needs to block progress.
> > But FWIIW, I think it would be nice to increase
> > build coverage and express this as:
> 
> Agree. While testing with IPv6 disabled, the netcons selftest exploded,
> so, this explose a bug in the selftest. This is now fixed in:
> 
> 	https://lore.kernel.org/all/20250723-netcons_test_ipv6-v1-1-41c9092f93f9@debian.org/

Nice, good to find bugs.