Re: [PATCH net-next v4 1/7] netconsole: add an address family to struct inet_addr

[netdev-bot+sashiko]

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

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

- [Low] Stale comment: the block comment above netcons_local_ip_unset()
  in drivers/net/netconsole.c still states "nt->local_ip is a union of…

Pre-existing issues:
- [Medium] netpoll_parse_ip_addr() (drivers/net/netconsole.c:751-773)
  commits address bytes into the caller's live struct inet_addr before…

--

netconsole: add an address family to struct inet_addr

Converts union inet_addr into a struct that carries a per-address family
field (AF_UNSPEC when unset, else AF_INET/AF_INET6) and sets that field in
the netconsole paths that populate local_ip/remote_ip. The %pI4 users are
updated to point at the union member instead of the containing object. No
reader of the new field is added yet.

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

[ ... ]

> @@ -741,10 +743,10 @@ static void netconsole_print_banner(struct netconsole_target *nt)
>  	np_info(np, "remote ethernet address %pM\n", nt->remote_mac);
>  }
>  
> -/* Parse the string and populate the `inet_addr` union. Return 0 if IPv4 is
> +/* Parse the string and populate the `inet_addr` struct. Return 0 if IPv4 is
>   * populated, 1 if IPv6 is populated, and -1 upon failure.
>   */

[Severity: Low]
This comment was updated from "union" to "struct", but the block comment
above netcons_local_ip_unset() in the same file still describes the old
layout:

drivers/net/netconsole.c:
 * 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 —

After this patch nt->local_ip is a struct whose address payload no longer
starts at offset 0, and it carries its own family discriminator. Should
that comment be refreshed too?

The reasoning about "first 4 bytes" is exactly the assumption this patch
had to fix at the three %pI4 sites (local_ip_show(), remote_ip_show() and
netcons_take_ipv4()), so leaving it in place is a little confusing.

For completeness: the netcons_local_ip_unset() helper and this comment are
removed by the following patch in the series ("netconsole: use the address
family instead of the ipv6 flag"), so the stale text does not survive the
series.

> -static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
> +static int netpoll_parse_ip_addr(const char *str, struct inet_addr *addr)
>  {
>  	const char *end = NULL;
>  	int len;
> @@ -756,14 +758,18 @@ static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
>  	if (str[len - 1] == '\n')
>  		len -= 1;
>  
> -	if (in4_pton(str, len, (void *)addr, -1, &end) > 0 &&
> -	    (!end || *end == 0 || *end == '\n'))
> +	if (in4_pton(str, len, (void *)&addr->ip, -1, &end) > 0 &&
> +	    (!end || *end == 0 || *end == '\n')) {
> +		addr->family = AF_INET;
>  		return 0;
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but can a rejected write leave
the target's address half-updated?

in4_pton() copies the parsed bytes into the destination before returning,
so the trailing-character check here runs after the store has already
happened:

net/core/utils.c:in4_pton() {
	...
	ret = 1;
	memcpy(dst, dbuf, sizeof(dbuf));
out:
	if (end)
		*end = s;
	return ret;
}

For input like "1.2.3.4x", in4_pton() returns > 0 and has written four
bytes into addr->ip, then (*end == 'x') fails the check, in6_pton() also
fails, and -1 is returned. The callers parse straight into the live target
and bail out without restoring the old value:

drivers/net/netconsole.c:remote_ip_store() {
	...
	ipv6 = netpoll_parse_ip_addr(buf, &nt->remote_ip);
	if (ipv6 == -1)
		goto out_unlock;
	nt->ipv6 = !!ipv6;
	...
}

local_ip_store() does the same for nt->local_ip. So a write that returns
-EINVAL to userspace can still change the configured address that
push_ipv4()/push_ipv6() and netcons_take_ipv6() later use.

Before this patch nt->ipv6 was the field left describing the previous
contents; with this patch addr->family is now also inconsistent with the
stored bytes on that path, even though the new field documents:

include/linux/netpoll.h:
	/* Address family: AF_UNSPEC when unset, else AF_INET or AF_INET6 */
	u8			family;

Would it be reasonable for netpoll_parse_ip_addr() to parse into a local
struct inet_addr and have the callers commit both the bytes and the family
only on success?

The behaviour is unchanged at the end of the series: netpoll_parse_ip_addr()
still writes into the caller's struct before validating, and
local_ip_store()/remote_ip_store() still return the error without restoring
the previous value.

>  
>  	if (IS_ENABLED(CONFIG_IPV6) &&
> -	    in6_pton(str, len, (void *)addr, -1, &end) > 0 &&
> -	    (!end || *end == 0 || *end == '\n'))
> +	    in6_pton(str, len, (void *)&addr->in6, -1, &end) > 0 &&
> +	    (!end || *end == 0 || *end == '\n')) {
> +		addr->family = AF_INET6;
>  		return 1;
> +	}
>  
>  	return -1;
>  }

[ ... ]

> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index 1c6b1eec5efd6..de97f001a0f9c 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -16,9 +16,13 @@
>  #include <linux/ip.h>
>  #include <linux/udp.h>
>  
> -union inet_addr {
> -	__be32		ip;
> -	struct in6_addr	in6;
> +struct inet_addr {
> +	/* Address family: AF_UNSPEC when unset, else AF_INET or AF_INET6 */
> +	u8			family;
> +	union {
> +		__be32		ip;
> +		struct in6_addr	in6;
> +	};
>  };

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