Re: [PATCH net-next v4 2/7] netconsole: use the address family instead of the ipv6 flag

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] netconsole: writing "0.0.0.0" (or "::") to local_ip no longer
  requests automatic source-address selection; the zero address is now…
- [Medium] At this commit a configfs target can be enabled with local_ip
  and remote_ip of different families, and the transmit path then reads…
- [Low] The readers converted by this patch treat the newly three-valued
  `family` field as two-valued: local_ip_show()…

--

netconsole: use the address family instead of the ipv6 flag

This drops the per-target nt->ipv6 boolean and derives IPv4 vs IPv6
behaviour from the family field stored in struct inet_addr instead.  The
netcons_local_ip_unset() helper is removed and the "local address not
configured" test becomes local_ip.family == AF_UNSPEC.  A few questions
about behaviour changes that come with this conversion follow.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 432b66cf111a5..351754d53cf97 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c

[ ... ]

> @@ -476,23 +474,6 @@ static int netcons_take_ipv4(struct netconsole_target *nt,
>  	return 0;
>  }
>  
> -/*
> - * Test whether the caller left nt->local_ip unset, so that
> - * netcons_netpoll_setup() should auto-populate it from the egress device.
> - *
> - * nt->local_ip is a union of __be32 (IPv4) and struct in6_addr (IPv6),
> - * so an IPv6 address whose first 4 bytes are zero (e.g. ::1, ::2,
> - * IPv4-mapped ::ffff:a.b.c.d) must not be tested via the IPv4 arm —
> - * doing so would misclassify a caller-supplied address as unset and
> - * silently overwrite it with whatever address the device exposes.
> - */
> -static bool netcons_local_ip_unset(const struct netconsole_target *nt)
> -{
> -	if (nt->ipv6)
> -		return ipv6_addr_any(&nt->local_ip.in6);
> -	return !nt->local_ip.ip;
> -}
> -
>  static int netcons_netpoll_setup(struct netconsole_target *nt)
>  {
>  	struct net *net = current->nsproxy->net_ns;
> @@ -538,16 +519,13 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
>  		rtnl_lock();
>  	}
>  
> -	if (netcons_local_ip_unset(nt)) {
> -		if (!nt->ipv6) {
> -			err = netcons_take_ipv4(nt, ndev);
> -			if (err)
> -				goto put;
> -		} else {
> +	if (nt->local_ip.family == AF_UNSPEC) {
> +		if (nt->remote_ip.family == AF_INET6)
>  			err = netcons_take_ipv6(nt, ndev);
> -			if (err)
> -				goto put;
> -		}
> +		else
> +			err = netcons_take_ipv4(nt, ndev);
> +		if (err)
> +			goto put;
>  		ip_overwritten = true;
>  	}

[Severity: Medium]

Is the replacement of netcons_local_ip_unset() with the AF_UNSPEC test
intended to also change the user-visible meaning of an all-zero local
address?

Before this patch the auto-populate test was purely value based
(!nt->local_ip.ip for IPv4, ipv6_addr_any() for IPv6), which matches the
long-standing netpoll behaviour where a zero local address requests
selection from the egress device.  After this patch the only way to reach
netcons_take_ipv4()/netcons_take_ipv6() is family == AF_UNSPEC, but
netpoll_parse_ip_addr() stamps a concrete family on every successful
parse, including "0.0.0.0" and "::":

	if (in4_pton(str, len, (void *)&addr->ip, -1, &end) > 0 &&
	    (!end || *end == 0 || *end == '\n')) {
		addr->family = AF_INET;
		return 0;
	}

So writing "0.0.0.0" to local_ip, or booting with
netconsole=@0.0.0.0/eth0,6666@a.b.c.d/, now enables the target without
error and every frame is built with a zero source address in push_ipv4()
and in the UDP pseudo-header, which the receiver or reverse path
filtering will drop, silently losing console output.

Related to that: the only remaining writer of AF_UNSPEC is the failure
path in netcons_netpoll_setup():

	if (ip_overwritten)
		memset(&nt->local_ip, 0, sizeof(nt->local_ip));

Once local_ip has been written once from configfs, is there any way for
userspace to get back to the auto-selection state?

The commit message says:

    We no longer need netcons_local_ip_unset() to check that all bytes are
    zeroes, as that is now denoted by (family == AF_UNSPEC).

Could this be reworded to state that the zeros-means-unset input is no
longer accepted, rather than presenting the two tests as equivalent?  A
later patch in the series ("docs: netconsole: document local_ip
auto-selection") documents the new behaviour, but the behaviour change
itself is not mentioned here.

> @@ -729,22 +707,22 @@ static void netconsole_print_banner(struct netconsole_target *nt)
>  	struct netpoll *np = &nt->np;
>  
>  	np_info(np, "local port %d\n", nt->local_port);
> -	if (nt->ipv6)
> +	if (nt->local_ip.family == AF_INET6)
>  		np_info(np, "local IPv6 address %pI6c\n", &nt->local_ip.in6);
>  	else
>  		np_info(np, "local IPv4 address %pI4\n", &nt->local_ip.ip);
>  	np_info(np, "interface name '%s'\n", np->dev_name);
>  	np_info(np, "local ethernet address '%pM'\n", np->dev_mac);
>  	np_info(np, "remote port %d\n", nt->remote_port);
> -	if (nt->ipv6)
> +	if (nt->remote_ip.family == AF_INET6)
>  		np_info(np, "remote IPv6 address %pI6c\n", &nt->remote_ip.in6);
>  	else
>  		np_info(np, "remote IPv4 address %pI4\n", &nt->remote_ip.ip);
>  	np_info(np, "remote ethernet address %pM\n", nt->remote_mac);
>  }
>  
> -/* Parse the string and populate the `inet_addr` struct. Return 0 if IPv4 is
> - * populated, 1 if IPv6 is populated, and -1 upon failure.
> +/* Parse the string and populate the `inet_addr` struct. Return 0 on success
> + * and -1 upon failure.
>   */
>  static int netpoll_parse_ip_addr(const char *str, struct inet_addr *addr)
>  {

[ ... ]

> @@ -874,7 +852,7 @@ static ssize_t local_ip_show(struct config_item *item, char *buf)
>  {
>  	struct netconsole_target *nt = to_target(item);
>  
> -	if (nt->ipv6)
> +	if (nt->local_ip.family == AF_INET6)
>  		return sysfs_emit(buf, "%pI6c\n", &nt->local_ip.in6);
>  	else
>  		return sysfs_emit(buf, "%pI4\n", &nt->local_ip.ip);
> @@ -884,7 +862,7 @@ static ssize_t remote_ip_show(struct config_item *item, char *buf)
>  {
>  	struct netconsole_target *nt = to_target(item);
>  
> -	if (nt->ipv6)
> +	if (nt->remote_ip.family == AF_INET6)
>  		return sysfs_emit(buf, "%pI6c\n", &nt->remote_ip.in6);
>  	else
>  		return sysfs_emit(buf, "%pI4\n", &nt->remote_ip.ip);

[Severity: Low]

The family field now has three states, per the comment on struct
inet_addr:

	/* Address family: AF_UNSPEC when unset, else AF_INET or AF_INET6 */
	u8			family;

but local_ip_show(), remote_ip_show() and netconsole_print_banner() all
use a two-way test, so AF_UNSPEC falls into the IPv4 else-branch.  Should
an unset address print as 0.0.0.0?

For a target with an IPv6 remote_ip and no local_ip written, cat local_ip
returns "0.0.0.0" while cat remote_ip returns an IPv6 literal, and before
this patch the same target had nt->ipv6 set so local_ip read back as
"::".  The 0.0.0.0 result is also indistinguishable from a deliberately
configured zero address, so a save-and-restore of the configuration turns
an auto-select target into a literal zero source address.

This is handled later in the series by "netconsole: show empty string for
an unset IP address", which adds an AF_UNSPEC case to both show handlers
and "local IP unset"/"remote IP unset" to the banner, so it only affects
the intermediate tree state.

[ ... ]

> @@ -1257,7 +1232,6 @@ 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;
>  
>  	dynamic_netconsole_mutex_lock();
>  	if (nt->state == STATE_ENABLED) {
> @@ -1266,10 +1240,8 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
>  		goto out_unlock;
>  	}
>  
> -	ipv6 = netpoll_parse_ip_addr(buf, &nt->remote_ip);
> -	if (ipv6 == -1)
> +	if (netpoll_parse_ip_addr(buf, &nt->remote_ip) < 0)
>  		goto out_unlock;
> -	nt->ipv6 = !!ipv6;
>  
>  	ret = count;
>  out_unlock:

[Severity: Medium]

With the shared nt->ipv6 flag gone, local_ip_store() and remote_ip_store()
each set only their own address's family, and nothing couples them.  What
keeps a configfs target from being enabled with local_ip as IPv4 and
remote_ip as IPv6?

netcons_netpoll_setup() at this commit only has the AF_UNSPEC
auto-populate branch, no family agreement check, so such a target reaches
STATE_ENABLED.  The transmit path then keys everything off remote_ip
while reading the local_ip union through the IPv6 arm:

drivers/net/netconsole.c:push_ipv6() {
	...
	ip6h->saddr = nt->local_ip.in6;
	ip6h->daddr = nt->remote_ip.in6;
	...
}

Only 4 bytes were written into that union by local_ip_store(), so the
emitted IPv6 source address is the IPv4 value followed by 12 bytes of
whatever the union previously held (zeros, or bytes of an earlier IPv6
address).  netpoll_udp_checksum() below feeds the same mismatched arm to
csum_ipv6_magic().

> @@ -2078,7 +2050,7 @@ static void netpoll_udp_checksum(struct netconsole_target *nt,
>  
>  	/* check needs to be set, since it will be consumed in csum_partial */
>  	udph->check = 0;
> -	if (nt->ipv6)
> +	if (nt->remote_ip.family == AF_INET6)
>  		udph->check = csum_ipv6_magic(&nt->local_ip.in6,
>  					      &nt->remote_ip.in6,
>  					      udp_len, IPPROTO_UDP,

[ ... ]

> @@ -2601,13 +2567,11 @@ static int netconsole_parser_cmdline(struct netconsole_target *nt, char *opt)
>  	if (!delim)
>  		goto parse_failed;
>  	*delim = 0;
> -	ipv6 = netpoll_parse_ip_addr(cur, &nt->remote_ip);
> -	if (ipv6 < 0)
> +	if (netpoll_parse_ip_addr(cur, &nt->remote_ip) < 0)
>  		goto parse_failed;
> -	else if (ipversion_set && nt->ipv6 != (bool)ipv6)
> +	if (nt->local_ip.family != AF_UNSPEC &&
> +	    nt->local_ip.family != nt->remote_ip.family)
>  		goto parse_failed;
> -	else
> -		nt->ipv6 = (bool)ipv6;
>  	cur = delim + 1;

The family agreement invariant that the transmit code depends on is
enforced here for cmdline targets, but not for configfs targets at this
commit.  Would it be worth folding the equivalent check into
netcons_netpoll_setup() in the same patch that removes nt->ipv6?  A
following patch in the series ("netconsole: reject a target mixing IPv4
and IPv6 addresses") does add it, so the window is limited to this
intermediate state.

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