Re: [PATCH net-next v4 5/7] netconsole: show empty string for an unset IP address

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] ABI show/store asymmetry for the configfs attributes local_ip
  and remote_ip: after this patch local_ip_show()/remote_ip_show() emit…
- [Low] The patch changes the on-read format of two documented configfs
  attributes (local_ip, remote_ip) from an always-parseable IPv4/IPv6…

--

netconsole: show empty string for an unset IP address

An unset local_ip/remote_ip is now denoted by AF_UNSPEC, and this patch
makes the configfs show handlers print an empty string instead of
"0.0.0.0" for that state. netconsole_print_banner() prints an explicit
"unset" message for the same case.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index abcc8515ddd2b..1f18d0a7d2ddb 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c

[ ... ]

> @@ -863,6 +867,8 @@ static ssize_t local_ip_show(struct config_item *item, char *buf)
>  {
>  	struct netconsole_target *nt = to_target(item);
>  
> +	if (nt->local_ip.family == AF_UNSPEC)
> +		return sysfs_emit(buf, "\n");

[Severity: Medium]
Can the value that local_ip_show() now produces be written back into the
same attribute?

A freshly created target has family == AF_UNSPEC, so local_ip_show()
emits just "\n". Feeding that back through local_ip_store() reaches
netpoll_parse_ip_addr():

drivers/net/netconsole.c:netpoll_parse_ip_addr() {
	len = strlen(str);
	if (!len)
		return -1;

	if (str[len - 1] == '\n')
		len -= 1;

	if (in4_pton(str, len, (void *)&addr->ip, -1, &end) > 0 &&
	...
}

An input of "" fails the !len test, and "\n" is trimmed to len 0 so both
in4_pton() and in6_pton() fail. local_ip_store() then returns -EINVAL:

drivers/net/netconsole.c:local_ip_store() {
	...
	if (netpoll_parse_ip_addr(buf, &nt->local_ip) < 0)
		goto out_unlock;	/* ret == -EINVAL */
	...
}

Before this change the show side printed "0.0.0.0", which the store side
accepted, so "cat local_ip > local_ip" and generic save/restore of all
target attributes round-tripped.

Should netpoll_parse_ip_addr() (or the store handlers) gain an
empty-string branch that sets family = AF_UNSPEC?  As it stands there
seems to be no way to move an address back to the unset state through
the configfs ABI, and this remains true at the end of the series
(62342076a29c).

[Severity: Low]
Should Documentation/networking/netconsole.rst be updated along with
this?  The attribute table there still describes both attributes as
plain read-write IP addresses:

	local_ip	Source IP address to use		(read-write)
	remote_ip	Remote agent's IP address		(read-write)

and the example section still shows "cat cmdline0/remote_ip" producing an
address.  The later patch in the series, "docs: netconsole: document
local_ip auto-selection", describes the auto-derivation of an unset
local_ip, but it does not mention that reading an unset local_ip or
remote_ip now yields an empty value, nor that such a value cannot be
written back.

>  	if (nt->local_ip.family == AF_INET6)
>  		return sysfs_emit(buf, "%pI6c\n", &nt->local_ip.in6);
>  	else
> @@ -873,6 +879,8 @@ static ssize_t remote_ip_show(struct config_item *item, char *buf)
>  {
>  	struct netconsole_target *nt = to_target(item);
>  
> +	if (nt->remote_ip.family == AF_UNSPEC)
> +		return sysfs_emit(buf, "\n");

[Severity: Medium]
Same question for remote_ip: remote_ip_store() -> netpoll_parse_ip_addr()
rejects the "\n" that remote_ip_show() emits, so
"cat remote_ip > remote_ip" returns -EINVAL on a target whose remote
address was never set.

>  	if (nt->remote_ip.family == AF_INET6)
>  		return sysfs_emit(buf, "%pI6c\n", &nt->remote_ip.in6);
>  	else

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-netcons_ipv6-v4-0-bdd183c844d3%40gmail.com