Re: [PATCH net-next v4 3/7] netconsole: reject enabling a target with no remote IP address

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] Documentation gap: after this patch `echo 1 > enabled` on a
  freshly created dynamic configfs target hard-fails with -EDESTADDRREQ…

Pre-existing issues:
- [Low] Concern (validated as NOT a defect after series verification,
  retained only per the validation-policy retention rule): claim that…

--

netconsole: reject enabling a target with no remote IP address

This adds an early check in netcons_netpoll_setup() so that enabling a
target whose remote address was never set fails with -EDESTADDRREQ instead
of sending netconsole traffic to 0.0.0.0. Only the configfs (dynamic
target) path is affected, since the command-line path already requires a
remote address.

No Link: tags present in the commit header.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 351754d53cf97..4af4f3039d4cd 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -483,6 +483,11 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
>  	bool ip_overwritten = false;
>  	int err;
>  
> +	if (nt->remote_ip.family == AF_UNSPEC) {
> +		np_err(np, "remote IP address not configured, aborting\n");
> +		return -EDESTADDRREQ;
> +	}
> +

[Severity: Low]
Should Documentation/networking/netconsole.rst be updated along with this?

The "Dynamic reconfiguration" section still says:

  Note that newly created targets have default parameter values (as mentioned
  above) and are disabled by default -- they must first be enabled by writing
  "1" to the "enabled" attribute (usually after setting parameters accordingly)
  as described below.

and the attribute table still lists remote_ip as an ordinary read-write
parameter:

	remote_ip	Remote agent's IP address		(read-write)

After this change the following sequence fails with -EDESTADDRREQ:

  mkdir /sys/kernel/config/netconsole/t1
  echo 1 > t1/enabled

Reading the rest of the series, the last patch ("docs: netconsole: document
local_ip auto-selection") only adds a paragraph stating that local_ip is
optional; nothing marks remote_ip as required before enabling, and the new
errno is not mentioned anywhere. Would a sentence in that section saying
remote_ip must be set before enabling be worth adding?

[Severity: Low]
This isn't a bug introduced by this patch, but a note on how the new gate
interacts with an explicitly written 0.0.0.0.

The check keys on nt->remote_ip.family == AF_UNSPEC, while
netpoll_parse_ip_addr() sets AF_INET for an explicit value:

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

So this still succeeds and sends to 0.0.0.0:

  echo 0.0.0.0 > t1/remote_ip
  echo 1 > t1/enabled

That looks intentional: the later patch "docs: netconsole: document local_ip
auto-selection" documents "Writing any value to it, including 0.0.0.0, uses
that value verbatim and disables the automatic selection", and it matches
the command-line path which also accepts an explicit tgt-ip of 0.0.0.0. The
related read-back ambiguity, where remote_ip_show() rendered AF_UNSPEC as
"0.0.0.0", is also addressed later in the series by "netconsole: show empty
string for an unset IP address". Mentioning it only so the distinction
between "unset" and "explicitly 0.0.0.0" is on record for this hunk.

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