From: Simon Horman <horms@kernel•org>
To: Uday Shankar <ushankar@purestorage•com>
Cc: Breno Leitao <leitao@debian•org>,
"David S. Miller" <davem@davemloft•net>,
Eric Dumazet <edumazet@google•com>,
Jakub Kicinski <kuba@kernel•org>, Paolo Abeni <pabeni@redhat•com>,
netdev@vger•kernel.org
Subject: Re: [PATCH] netconsole: allow selection of egress interface via MAC address
Date: Thu, 12 Dec 2024 10:11:56 +0000 [thread overview]
Message-ID: <20241212101156.GF2806@kernel.org> (raw)
In-Reply-To: <20241211021851.1442842-1-ushankar@purestorage.com>
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;
> }
...
next prev parent reply other threads:[~2024-12-12 10:12 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-11 2:18 [PATCH] netconsole: allow selection of egress interface via MAC address Uday Shankar
2024-12-12 10:11 ` Simon Horman [this message]
2024-12-12 22:31 ` Uday Shankar
2024-12-13 10:34 ` Simon Horman
2024-12-12 12:34 ` Paolo Abeni
2024-12-12 21:59 ` Uday Shankar
2025-01-03 11:41 ` Breno Leitao
2025-01-08 15:02 ` Uday Shankar
2025-01-09 15:43 ` Breno Leitao
2025-02-03 13:12 ` Breno Leitao
2025-02-03 20:29 ` Uday Shankar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241212101156.GF2806@kernel.org \
--to=horms@kernel$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=leitao@debian$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=ushankar@purestorage$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox