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

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

[Uday Shankar]

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>
---
 Documentation/networking/netconsole.rst |  8 +++-
 drivers/net/netconsole.c                | 50 +++++++++++++++++++++----
 include/linux/netpoll.h                 |  7 ++++
 net/core/netpoll.c                      | 49 +++++++++++++++++++-----
 4 files changed, 94 insertions(+), 20 deletions(-)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index d55c2a22ec7a..c2e269dc8d75 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -45,7 +45,7 @@ following format::
 	r             if present, prepend kernel version (release) to the message
 	src-port      source for UDP packets (defaults to 6665)
 	src-ip        source IP to use (interface address)
-	dev           network interface (eth0)
+	dev           network interface name (eth0) or MAC address
 	tgt-port      port for logging agent (6666)
 	tgt-ip        IP address for logging agent
 	tgt-macaddr   ethernet MAC address for logging agent (broadcast)
@@ -62,6 +62,10 @@ or using IPv6::
 
  insmod netconsole netconsole=@/,@fd00:1:2:3::1/
 
+or using a MAC address to select the egress interface::
+
+   linux netconsole=4444@10.0.0.1/ab:cd:ef:12:34:56,9353@10.0.0.2/12:34:56:78:9a:bc
+
 It also supports logging to multiple remote agents by specifying
 parameters for the multiple agents separated by semicolons and the
 complete string enclosed in "quotes", thusly::
@@ -133,7 +137,7 @@ The interface exposes these parameters of a netconsole target to userspace:
 	remote_port	Remote agent's UDP port			(read-write)
 	local_ip	Source IP address to use		(read-write)
 	remote_ip	Remote agent's IP address		(read-write)
-	local_mac	Local interface's MAC address		(read-only)
+	local_mac	Local interface's MAC address		(read-write)
 	remote_mac	Remote agent's MAC address		(read-write)
 	==============  =================================       ============
 
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
@@ -113,7 +113,7 @@ static struct console netconsole_ext;
  *		remote_port	(read-write)
  *		local_ip	(read-write)
  *		remote_ip	(read-write)
- *		local_mac	(read-only)
+ *		local_mac	(read-write)
  *		remote_mac	(read-write)
  */
 struct netconsole_target {
@@ -211,6 +211,8 @@ static struct netconsole_target *alloc_and_init(void)
 
 	nt->np.name = "netconsole";
 	strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+	/* the "don't use" or N/A value for this field */
+	eth_broadcast_addr(nt->np.local_mac);
 	nt->np.local_port = 6665;
 	nt->np.remote_port = 6666;
 	eth_broadcast_addr(nt->np.remote_mac);
@@ -360,10 +362,7 @@ static ssize_t remote_ip_show(struct config_item *item, char *buf)
 
 static ssize_t local_mac_show(struct config_item *item, char *buf)
 {
-	struct net_device *dev = to_target(item)->np.dev;
-	static const u8 bcast[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
-
-	return sysfs_emit(buf, "%pM\n", dev ? dev->dev_addr : bcast);
+	return sysfs_emit(buf, "%pM\n", to_target(item)->np.local_mac);
 }
 
 static ssize_t remote_mac_show(struct config_item *item, char *buf)
@@ -511,11 +510,41 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 
 	strscpy(nt->np.dev_name, buf, IFNAMSIZ);
 	trim_newline(nt->np.dev_name, IFNAMSIZ);
+	/* the "don't use" or N/A value for this field */
+	eth_broadcast_addr(nt->np.local_mac);
 
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return strnlen(buf, count);
 }
 
+static ssize_t local_mac_store(struct config_item *item, const char *buf,
+			       size_t count)
+{
+	struct netconsole_target *nt = to_target(item);
+	u8 local_mac[ETH_ALEN];
+	ssize_t ret = -EINVAL;
+
+	mutex_lock(&dynamic_netconsole_mutex);
+	if (nt->enabled) {
+		pr_err("target (%s) is enabled, disable to update parameters\n",
+		       config_item_name(&nt->group.cg_item));
+		goto out_unlock;
+	}
+
+	if (!mac_pton(buf, local_mac))
+		goto out_unlock;
+	if (buf[3 * ETH_ALEN - 1] && buf[3 * ETH_ALEN - 1] != '\n')
+		goto out_unlock;
+	memcpy(nt->np.local_mac, local_mac, ETH_ALEN);
+	/* force use of local_mac for device lookup */
+	nt->np.dev_name[0] = '\0';
+
+	ret = strnlen(buf, count);
+out_unlock:
+	mutex_unlock(&dynamic_netconsole_mutex);
+	return ret;
+}
+
 static ssize_t local_port_store(struct config_item *item, const char *buf,
 		size_t count)
 {
@@ -839,7 +868,7 @@ CONFIGFS_ATTR(, local_port);
 CONFIGFS_ATTR(, remote_port);
 CONFIGFS_ATTR(, local_ip);
 CONFIGFS_ATTR(, remote_ip);
-CONFIGFS_ATTR_RO(, local_mac);
+CONFIGFS_ATTR(, local_mac);
 CONFIGFS_ATTR(, remote_mac);
 CONFIGFS_ATTR(, release);
 
@@ -1001,8 +1030,9 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	bool stopped = false;
 
-	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
-	      event == NETDEV_RELEASE || event == NETDEV_JOIN))
+	if (!(event == NETDEV_CHANGENAME || event == NETDEV_CHANGEADDR ||
+	      event == NETDEV_UNREGISTER || event == NETDEV_RELEASE ||
+	      event == NETDEV_JOIN))
 		goto done;
 
 	mutex_lock(&target_cleanup_list_lock);
@@ -1014,6 +1044,10 @@ static int netconsole_netdev_event(struct notifier_block *this,
 			case NETDEV_CHANGENAME:
 				strscpy(nt->np.dev_name, dev->name, IFNAMSIZ);
 				break;
+			case NETDEV_CHANGEADDR:
+				memcpy(nt->np.local_mac, dev->dev_addr,
+				       ETH_ALEN);
+				break;
 			case NETDEV_RELEASE:
 			case NETDEV_JOIN:
 			case NETDEV_UNREGISTER:
diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index b34301650c47..e5cdb4d40f13 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -25,7 +25,14 @@ union inet_addr {
 struct netpoll {
 	struct net_device *dev;
 	netdevice_tracker dev_tracker;
+	/*
+	 * Either dev_name or local_mac can be used to specify the local
+	 * interface - dev_name will be used if it is nonempty, else
+	 * local_mac is used. Once netpoll_setup returns successfully,
+	 * both fields are populated.
+	 */
 	char dev_name[IFNAMSIZ];
+	u8 local_mac[ETH_ALEN];
 	const char *name;
 
 	union inet_addr local_ip, remote_ip;
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));
+		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;
 	}
 	cur++;
@@ -660,6 +670,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 
 	np->dev = ndev;
 	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
+	memcpy(np->local_mac, ndev->dev_addr, ETH_ALEN);
 	npinfo->netpoll = np;
 
 	/* last thing to do is link it to the net device structure */
@@ -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;
+	}
+
+	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];
 
 	skb_queue_head_init(&np->skb_pool);
 
 	rtnl_lock();
+	struct net *net = current->nsproxy->net_ns;
 	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;
 	}
@@ -704,7 +732,8 @@ int netpoll_setup(struct netpoll *np)
 	if (!netif_running(ndev)) {
 		unsigned long atmost;
 
-		np_info(np, "device %s not up yet, forcing it\n", np->dev_name);
+		np_info(np, "device %s not up yet, forcing it\n",
+			local_dev(np, buf));
 
 		err = dev_open(ndev, NULL);
 
@@ -738,7 +767,7 @@ int netpoll_setup(struct netpoll *np)
 			if (!ifa) {
 put_noaddr:
 				np_err(np, "no IP address for %s, aborting\n",
-				       np->dev_name);
+				       local_dev(np, buf));
 				err = -EDESTADDRREQ;
 				goto put;
 			}
@@ -769,13 +798,13 @@ int netpoll_setup(struct netpoll *np)
 			}
 			if (err) {
 				np_err(np, "no IPv6 address for %s, aborting\n",
-				       np->dev_name);
+				       local_dev(np, buf));
 				goto put;
 			} else
 				np_info(np, "local IPv6 %pI6c\n", &np->local_ip.in6);
 #else
 			np_err(np, "IPv6 is not supported %s, aborting\n",
-			       np->dev_name);
+			       local_dev(np, buf));
 			err = -EINVAL;
 			goto put;
 #endif
-- 
2.34.1

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

[Simon Horman]

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;
>  	}

...

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

[Uday Shankar]

On Thu, Dec 12, 2024 at 10:11:56AM +0000, Simon Horman wrote:
> 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/

Sure, I can add a test. That patchset does some refactoring that I'd
like to use though. Can it be merged? It looks like it's ready.

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

[Simon Horman]

On Thu, Dec 12, 2024 at 03:31:54PM -0700, Uday Shankar wrote:
> On Thu, Dec 12, 2024 at 10:11:56AM +0000, Simon Horman wrote:
> > 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/
> 
> Sure, I can add a test. That patchset does some refactoring that I'd
> like to use though. Can it be merged? It looks like it's ready.

It is in the queue for the maintainers to decide on.
We will see :)

In any case I agree that it makes sense to base your test
on the refactoring in that series, unless that series gets
derailed for some reason.

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

[Paolo Abeni]

On 12/11/24 03:18, Uday Shankar wrote:
> +static ssize_t local_mac_store(struct config_item *item, const char *buf,
> +			       size_t count)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +	u8 local_mac[ETH_ALEN];
> +	ssize_t ret = -EINVAL;
> +
> +	mutex_lock(&dynamic_netconsole_mutex);
> +	if (nt->enabled) {
> +		pr_err("target (%s) is enabled, disable to update parameters\n",
> +		       config_item_name(&nt->group.cg_item));
> +		goto out_unlock;
> +	}
> +
> +	if (!mac_pton(buf, local_mac))
> +		goto out_unlock;
> +	if (buf[3 * ETH_ALEN - 1] && buf[3 * ETH_ALEN - 1] != '\n')
> +		goto out_unlock;

I think you should instead check 'count >= 3 * ETH_ALEN', and do such
check before calling 'mac_pton'.

Thanks,

Paolo

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

[Uday Shankar]

On Thu, Dec 12, 2024 at 01:34:12PM +0100, Paolo Abeni wrote:
> On 12/11/24 03:18, Uday Shankar wrote:
> > +static ssize_t local_mac_store(struct config_item *item, const char *buf,
> > +			       size_t count)
> > +{
> > +	struct netconsole_target *nt = to_target(item);
> > +	u8 local_mac[ETH_ALEN];
> > +	ssize_t ret = -EINVAL;
> > +
> > +	mutex_lock(&dynamic_netconsole_mutex);
> > +	if (nt->enabled) {
> > +		pr_err("target (%s) is enabled, disable to update parameters\n",
> > +		       config_item_name(&nt->group.cg_item));
> > +		goto out_unlock;
> > +	}
> > +
> > +	if (!mac_pton(buf, local_mac))
> > +		goto out_unlock;
> > +	if (buf[3 * ETH_ALEN - 1] && buf[3 * ETH_ALEN - 1] != '\n')
> > +		goto out_unlock;
> 
> I think you should instead check 'count >= 3 * ETH_ALEN', and do such
> check before calling 'mac_pton'.

Is that needed? mac_pton has an internal check based on strnlen, which
is guaranteed to succeed because the kernfs layer will NUL-terminate buf
for us.

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

[Breno Leitao]

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

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

[Uday Shankar]

On Fri, Jan 03, 2025 at 03:41:17AM -0800, Breno Leitao wrote:
> > 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).

Do you mean creating a new field called dev_mac which replaces
local_mac? I do agree that naming is a bit better but I'd be worried
about breaking programs which expect local_mac to exist. Having the
field go read-only --> read-write via this change feels a lot less
disruptive to preexisting programs than renaming the field.

Or do you mean creating a new field dev_mac which will live alongside
local_mac, and letting local_mac keep its existing semantics? It feels
like that would lead to messier code, since dev_mac's semantics are kind
of a superset of local_mac's semantics (e.g. after selecting and
enabling a netconsole via dev_name, local_mac is populated with the mac
address of the interface and we'd probably want the same for dev_mac as
well).

A third option would be dropping the configfs changes altogether, which
I'd be okay with - as I highlighted in the commit message, I suspect
this interface is far less likely to see real use than the command-line
parameter. A downside of this option though is that automated testing
becomes difficult, as we can't write a variant of netcons_basic.sh
without configfs support. We'd have to have a test which uses the
parameter directly, and I'm not sure if we have a testing framework for
the kernel which would support that.

Let me know which option you think is best, and I'll move forward with
it in v2.

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

I wanted to maintain the invariant that when setting up a netconsole, at
most one of dev_name and local_mac is set to a meaningful value, as
otherwise we'd need to implement and document some sort of priority
system when it comes to selecting the local interface. This invariant
requires having a designated "invalid" value for each field - it's the
empty string for dev_name and the broadcast mac for local_mac (for
backwards compatibility purposes, see below).

> 
> > +	eth_broadcast_addr(nt->np.local_mac);
> 
> Why not just memzeroing the memory?

That could work, but we kind of had an unwritten rule that the broadcast
address was the invalid value for local_mac in the code before. For
example, when creating a brand new netconsole via configfs:

# cd /sys/kernel/config/netconsole/
# mkdir test
# cat test/local_mac
ff:ff:ff:ff:ff:ff

So I stuck with the broadcast mac address for the local_mac "invalid"
value.

ACK on the rest of the comments, I will address them in v2 once we have
clarity on the above issue.

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

[Breno Leitao]

Hello Uday,

On Wed, Jan 08, 2025 at 08:02:44AM -0700, Uday Shankar wrote:
> On Fri, Jan 03, 2025 at 03:41:17AM -0800, Breno Leitao wrote:

> > 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).
> 
> Do you mean creating a new field called dev_mac which replaces
> local_mac? I do agree that naming is a bit better but I'd be worried
> about breaking programs which expect local_mac to exist. Having the
> field go read-only --> read-write via this change feels a lot less
> disruptive to preexisting programs than renaming the field.
> 
> Or do you mean creating a new field dev_mac which will live alongside
> local_mac, and letting local_mac keep its existing semantics? It feels

Right, that is what I meant originally.

> like that would lead to messier code, since dev_mac's semantics are kind
> of a superset of local_mac's semantics (e.g. after selecting and
> enabling a netconsole via dev_name, local_mac is populated with the mac
> address of the interface and we'd probably want the same for dev_mac as
> well).
> 
> A third option would be dropping the configfs changes altogether, which
> I'd be okay with - as I highlighted in the commit message, I suspect
> this interface is far less likely to see real use than the command-line
> parameter. 

I like this option better, in fact. I agree we don't need to expose it via
configfs (at least for now), since configfs configuration solves a
slightly different problem.

> A downside of this option though is that automated testing
> becomes difficult, as we can't write a variant of netcons_basic.sh

True. I _think_ it is better to optimize for simplicity in such case,
and skip the configfs changes (at least for now).

> without configfs support. We'd have to have a test which uses the
> parameter directly, and I'm not sure if we have a testing framework for
> the kernel which would support that.
 
 I am wondering if we can test it by turning netconsole into a module in
 the test .config, and passing the netconsole parameters when loading
 the module? 

--breno

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

[Breno Leitao]

Hello Uday,

On Thu, Jan 09, 2025 at 07:43:44AM -0800, Breno Leitao wrote:
> On Wed, Jan 08, 2025 at 08:02:44AM -0700, Uday Shankar wrote:
> > On Fri, Jan 03, 2025 at 03:41:17AM -0800, Breno Leitao wrote:
> 
> > > 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).
> > 
> > Do you mean creating a new field called dev_mac which replaces
> > local_mac? I do agree that naming is a bit better but I'd be worried
> > about breaking programs which expect local_mac to exist. Having the
> > field go read-only --> read-write via this change feels a lot less
> > disruptive to preexisting programs than renaming the field.
> > 
> > Or do you mean creating a new field dev_mac which will live alongside
> > local_mac, and letting local_mac keep its existing semantics? It feels
> 
> Right, that is what I meant originally.
> 
> > like that would lead to messier code, since dev_mac's semantics are kind
> > of a superset of local_mac's semantics (e.g. after selecting and
> > enabling a netconsole via dev_name, local_mac is populated with the mac
> > address of the interface and we'd probably want the same for dev_mac as
> > well).
> > 
> > A third option would be dropping the configfs changes altogether, which
> > I'd be okay with - as I highlighted in the commit message, I suspect
> > this interface is far less likely to see real use than the command-line
> > parameter. 
> 
> I like this option better, in fact. I agree we don't need to expose it via
> configfs (at least for now), since configfs configuration solves a
> slightly different problem.
> 
> > A downside of this option though is that automated testing
> > becomes difficult, as we can't write a variant of netcons_basic.sh
> 
> True. I _think_ it is better to optimize for simplicity in such case,
> and skip the configfs changes (at least for now).
> 
> > without configfs support. We'd have to have a test which uses the
> > parameter directly, and I'm not sure if we have a testing framework for
> > the kernel which would support that.
>  
>  I am wondering if we can test it by turning netconsole into a module in
>  the test .config, and passing the netconsole parameters when loading
>  the module? 

I wanted to check in on the status of this patchset, as I haven't
received any updates in the past two weeks. I remain enthusiastic about
this feature and believe it would be a valuable addition to the next
major merge window.

If you need any assistance or support, please don't hesitate to reach
out. I'm more than happy to help.

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

[Uday Shankar]

On Mon, Feb 03, 2025 at 05:12:37AM -0800, Breno Leitao wrote:
> I wanted to check in on the status of this patchset, as I haven't
> received any updates in the past two weeks. I remain enthusiastic about
> this feature and believe it would be a valuable addition to the next
> major merge window.
> 
> If you need any assistance or support, please don't hesitate to reach
> out. I'm more than happy to help.

Hey Breno, thanks for checking in. I've had the functional revisions
(removal of configfs interface) done for some time now, but haven't had
the time to figure out testing - last I checked, it would require a fair
bit of refactoring in libnetcons.sh since we'd have to use the module
parameter interface instead of configfs.

Perhaps it would be okay to merge the functional change first, then
follow up with test later? I'll try to clean up what I have, get a round
of manual testing in, and post it soon.