Message-ID: <20250103-loutish-heavy-caracal-1dfb5d@leitao>
Date: Fri, 3 Jan 2025 03:41:17 -0800
From: Breno Leitao <leitao@...ian.org>
To: Uday Shankar <ushankar@...estorage.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] netconsole: allow selection of egress interface via MAC
 address

Hello Uday,

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.

Agree, and I think this might be a real problem. Thanks for addressing it.

> 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.

This will change slightly local_mac meaning. At the same time, I am not
sure local_mac is a very useful field as-is. The configuration might be
a bit confusing using `local_mac` to define the target interface. I am
wondering if creating a new field might be more appropriate. Maybe
`dev_mac`? (I am not super confident this approach is better TBH, but, it
seems easier to reason about).

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 4ea44a2f48f7..865c43a97f70 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c

> @@ -211,6 +211,8 @@ static struct netconsole_target *alloc_and_init(void)
> +	/* the "don't use" or N/A value for this field */

This comment is not very clear. What do you mean exactly?

> +	eth_broadcast_addr(nt->np.local_mac);

Why not just memzeroing the memory?

> 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
> @@ -501,7 +501,8 @@ void netpoll_print_options(struct netpoll *np)
>  		np_info(np, "local IPv6 address %pI6c\n", &np->local_ip.in6);
>  	else
>  		np_info(np, "local IPv4 address %pI4\n", &np->local_ip.ip);
> -	np_info(np, "interface '%s'\n", np->dev_name);
> +	np_info(np, "interface name '%s'\n", np->dev_name);
> +	np_info(np, "local ethernet address '%pM'\n", np->local_mac);
>  	np_info(np, "remote port %d\n", np->remote_port);
>  	if (np->ipv6)
>  		np_info(np, "remote IPv6 address %pI6c\n", &np->remote_ip.in6);
> @@ -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));

nit: You can reset np->dev_name and np->dev_name and just overwrite one of
them depending on what was set. Something as:

	eth_broadcast_addr(np->local_mac);
	np->dev_name[0] = '\0';

	if (!strchr(cur, ':'))
		strscpy(np->dev_name, cur, sizeof(np->dev_name));
	else {
		if (!mac_pton(cur, np->local_mac))
			goto parse_failed.


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

> @@ -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)
> +
> +static char *local_dev(struct netpoll *np, char *buf)
> +{
> +	if (np->dev_name[0]) {
> +		return np->dev_name;

nit: You don't need the {} here.

> +	}
> +
> +	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: keep the revert XMAS tree format here. Also renaming it to
local_dev_mac or something similar might be a good idea.

>  	skb_queue_head_init(&np->skb_pool);
>  
>  	rtnl_lock();
> +	struct net *net = current->nsproxy->net_ns;

Does this need to be done inside the rtnl lock? I tried to search, and
it seems you can get it outside of the lock.

nit: You can move declaration of `*net` to 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);
>  	}

nit: You can get rid of all braces above.

I haven't run the code yet, but, I will be doing it new week.

Thanks for addressing this limitation,
--breno

Powered by blists - more mailing lists