Re: [PATCH] netconsole: allow selection of egress interface via MAC address

[Simon Horman <horms@kernel.org>]

On Tue, Dec 10, 2024 at 07:18:52PM -0700, Uday Shankar wrote:
> Currently, netconsole has two methods of configuration - kernel command
> line parameter and configfs. The former interface allows for netconsole
> activation earlier during boot, so it is preferred for debugging issues
> which arise before userspace is up/the configfs interface can be used.
> The kernel command line parameter syntax requires specifying the egress
> interface name. This requirement makes it hard to use for a couple
> reasons:
> - The egress interface name can be hard or impossible to predict. For
>   example, installing a new network card in a system can change the
>   interface names assigned by the kernel.
> - When constructing the kernel parameter, one may have trouble
>   determining the original (kernel-assigned) name of the interface
>   (which is the name that should be given to netconsole) if some stable
>   interface naming scheme is in effect. A human can usually look at
>   kernel logs to determine the original name, but this is very painful
>   if automation is constructing the parameter.
> 
> For these reasons, allow selection of the egress interface via MAC
> address. To maintain parity between interfaces, the local_mac entry in
> configfs is also made read-write and can be used to select the local
> interface, though this use case is less interesting than the one
> highlighted above.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>

Hi Uday,

Overall this looks good to me. But I have some minor feedback.

Firstly, this patch doesn't apply to net-next.
Which means that the Netdev CI doesn't run.
So, please rebase and post a v2.
But please first wait for review from others.

Also, as this is a new feature, I wonder if a selftest should be added.
Perhaps some variant of netcons_basic.sh as has been done here:

* [PATCH net-next 0/4] netconsole: selftest for userdata overflow
  https://lore.kernel.org/netdev/20241204-netcons_overflow_test-v1-0-a85a8d0ace21@debian.org/

...

> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 2e459b9d88eb..485093387b9f 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c

...

> @@ -570,11 +571,20 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
>  	cur++;
>  
>  	if (*cur != ',') {
> -		/* parse out dev name */
> +		/* parse out dev_name or local_mac */
>  		if ((delim = strchr(cur, ',')) == NULL)
>  			goto parse_failed;
>  		*delim = 0;
> -		strscpy(np->dev_name, cur, sizeof(np->dev_name));
> +		if (!strchr(cur, ':')) {
> +			strscpy(np->dev_name, cur, sizeof(np->dev_name));
> +			eth_broadcast_addr(np->local_mac);
> +		} else {
> +			if (!mac_pton(cur, np->local_mac)) {
> +				goto parse_failed;
> +			}

nit: No need for braces in the conditional above:

			if (!mac_pton(cur, np->local_mac))
				goto parse_failed;

> +			/* force use of local_mac for device lookup */
> +			np->dev_name[0] = '\0';
> +		}
>  		cur = delim;
>  	}
>  	cur++;

...

> @@ -674,29 +685,46 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>  }
>  EXPORT_SYMBOL_GPL(__netpoll_setup);
>  
> +/* upper bound on length of %pM output */
> +#define MAX_MAC_ADDR_LEN (4 * ETH_ALEN)

I think 3 * ETH_ALEN is enough for the hex digits, colons (':') and
trailing NUL character ('\0').

And I think that defining it as such would allow it to be reused in
local_mac_store.

Also, this seems to occur a few times throughout the tree.
Perhaps adding it somewhere more global would make sense.

> +
> +static char *local_dev(struct netpoll *np, char *buf)
> +{
> +	if (np->dev_name[0]) {
> +		return np->dev_name;
> +	}

nit: No need for braces in the conditional above.

> +
> +	snprintf(buf, MAX_MAC_ADDR_LEN, "%pM", np->local_mac);
> +	return buf;
> +}
> +
>  int netpoll_setup(struct netpoll *np)
>  {
>  	struct net_device *ndev = NULL;
>  	bool ip_overwritten = false;
>  	struct in_device *in_dev;
>  	int err;
> +	char buf[MAX_MAC_ADDR_LEN];

nit: Please maintain reverse xmas tree order - longest line to shortest -
     for local variable declarations.
>  
>  	skb_queue_head_init(&np->skb_pool);
>  
>  	rtnl_lock();
> +	struct net *net = current->nsproxy->net_ns;

Please declare local variables at the top of the function.

>  	if (np->dev_name[0]) {
> -		struct net *net = current->nsproxy->net_ns;
>  		ndev = __dev_get_by_name(net, np->dev_name);
> +	} else if (is_valid_ether_addr(np->local_mac)) {
> +		ndev = dev_getbyhwaddr_rcu(net, ARPHRD_ETHER, np->local_mac);
>  	}
>  	if (!ndev) {
> -		np_err(np, "%s doesn't exist, aborting\n", np->dev_name);
> +		np_err(np, "%s doesn't exist, aborting\n", local_dev(np, buf));
>  		err = -ENODEV;
>  		goto unlock;
>  	}
>  	netdev_hold(ndev, &np->dev_tracker, GFP_KERNEL);
>  
>  	if (netdev_master_upper_dev_get(ndev)) {
> -		np_err(np, "%s is a slave device, aborting\n", np->dev_name);
> +		np_err(np, "%s is a slave device, aborting\n",
> +		       local_dev(np, buf));
>  		err = -EBUSY;
>  		goto put;
>  	}

...