Re: [PATCH net-next v4 1/7] netconsole: add an address family to struct inet_addr
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-08 19:29:33
Also in:
linux-doc, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
-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.
quoted hunk ↗ jump to hunk
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; }
[ ... ]
quoted hunk ↗ jump to hunk
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