| Message ID | 20241211021851.1442842-1-ushankar@purestorage.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netconsole: allow selection of egress interface via MAC address | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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; > } ...
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
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.
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.
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.
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
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(-)