| Message ID | 20230707132911.2033870-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] netconsole: Append kernel version to message | expand |
On Fri, 7 Jul 2023 06:29:11 -0700 Breno Leitao wrote: > Create a new netconsole runtime option that prepends the kernel version in > the netconsole message. This is useful to map kernel messages to kernel > version in a simple way, i.e., without checking somewhere which kernel > version the host that sent the message is using. > > If this option is selected, then the "<release>," is prepended before the > netconsole message. This is an example of a netconsole output, with > release feature enabled: > > 6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test > > Calvin Owens send a RFC about this problem in 2016[1], but his > approach was a bit more intrusive, changing the printk subsystem. This > approach is lighter, and just append the information in the last mile, > just before netconsole push the message to netpoll. > > [1] Link: https://lore.kernel.org/all/51047c0f6e86abcb9ee13f60653b6946f8fcfc99.1463172791.git.calvinowens@fb.com/ > > Cc: Dave Jones <davej@codemonkey.org.uk> > Signed-off-by: Breno Leitao <leitao@debian.org> Looks good! net-next is closed for the duration of the merge window so you'll need to repost next week, and please put [PATCH net-next v3] as the subject prefix while at it. > @@ -332,6 +350,11 @@ static ssize_t enabled_store(struct config_item *item, > } > > if (enabled) { /* true */ > + if (nt->release && !nt->extended) { > + pr_err("release feature requires extended log message\n"); > + goto out_unlock; > + } This is the only bit that gave me pause - when parsing the command line you ignore release if extended is not set (with an error/warning). Does it make sense to be consistent and do the same thing here? Or enabling at runtime is fundamentally different?
On Fri, Jul 07, 2023 at 04:10:50PM -0700, Jakub Kicinski wrote: > On Fri, 7 Jul 2023 06:29:11 -0700 Breno Leitao wrote: > > Create a new netconsole runtime option that prepends the kernel version in > > the netconsole message. This is useful to map kernel messages to kernel > > version in a simple way, i.e., without checking somewhere which kernel > > version the host that sent the message is using. > > > > If this option is selected, then the "<release>," is prepended before the > > netconsole message. This is an example of a netconsole output, with > > release feature enabled: > > > > 6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test > > > > Calvin Owens send a RFC about this problem in 2016[1], but his > > approach was a bit more intrusive, changing the printk subsystem. This > > approach is lighter, and just append the information in the last mile, > > just before netconsole push the message to netpoll. > > > > [1] Link: https://lore.kernel.org/all/51047c0f6e86abcb9ee13f60653b6946f8fcfc99.1463172791.git.calvinowens@fb.com/ > > > > Cc: Dave Jones <davej@codemonkey.org.uk> > > Signed-off-by: Breno Leitao <leitao@debian.org> > > Looks good! net-next is closed for the duration of the merge window > so you'll need to repost next week, and please put [PATCH net-next v3] > as the subject prefix while at it. > > > @@ -332,6 +350,11 @@ static ssize_t enabled_store(struct config_item *item, > > } > > > > if (enabled) { /* true */ > > + if (nt->release && !nt->extended) { > > + pr_err("release feature requires extended log message\n"); > > + goto out_unlock; > > + } > > This is the only bit that gave me pause - when parsing the command line > you ignore release if extended is not set (with an error/warning). > Does it make sense to be consistent and do the same thing here? > Or enabling at runtime is fundamentally different? That is a good point, this patch ignores "release" if extended feature is disabled in the command line, but, fails if "release" is set and extended is not. Looking at the other behaviours (netpoll parsing_ in the code, I think the best approach is to also fail on both cases. I'll fix it in v3. Thanks for the review!
On Fri 2023-07-07 06:29:11, Breno Leitao wrote: > Create a new netconsole runtime option that prepends the kernel version in > the netconsole message. This is useful to map kernel messages to kernel > version in a simple way, i.e., without checking somewhere which kernel > version the host that sent the message is using. > > If this option is selected, then the "<release>," is prepended before the > netconsole message. This is an example of a netconsole output, with > release feature enabled: > > 6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test > > Calvin Owens send a RFC about this problem in 2016[1], but his > approach was a bit more intrusive, changing the printk subsystem. This > approach is lighter, and just append the information in the last mile, > just before netconsole push the message to netpoll. Thanks a lot for solving this on the netconsole level. The extended console format is used also for /dev/kmsg. Which is then used by systemd journal, dmesg, and maybe other userspace tools. Any format changes might break these tools. I have glanced over the patch and have two comments. > @@ -254,6 +267,11 @@ static ssize_t extended_show(struct config_item *item, char *buf) > return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->extended); > } > > +static ssize_t release_show(struct config_item *item, char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->release); I have learned recently that sysfs_emit() was preferred over snprintf() in the _show() callbacks. > +} > + > static ssize_t dev_name_show(struct config_item *item, char *buf) > { > return snprintf(buf, PAGE_SIZE, "%s\n", to_target(item)->np.dev_name); > @@ -366,6 +389,38 @@ static ssize_t enabled_store(struct config_item *item, > return err; > } > > +static ssize_t release_store(struct config_item *item, const char *buf, > + size_t count) > +{ > + struct netconsole_target *nt = to_target(item); > + int release; > + int err; > + > + mutex_lock(&dynamic_netconsole_mutex); > + if (nt->enabled) { > + pr_err("target (%s) is enabled, disable to update parameters\n", > + config_item_name(&nt->item)); > + err = -EINVAL; > + goto out_unlock; > + } > + > + err = kstrtoint(buf, 10, &release); > + if (err < 0) > + goto out_unlock; > + if (release < 0 || release > 1) { > + err = -EINVAL; > + goto out_unlock; > + } You might consider using: bool enabled; err = kstrtobool(buf, &enabled); if (err) goto unlock; It accepts more input values, for example, 1/0, y/n, Y/N, ... Well, I see that kstrtoint() is used also in enabled_store(). It might be confusing when "/enabled" supports only "1/0" and "/release" supports more variants. > + > + nt->release = release; > + > + mutex_unlock(&dynamic_netconsole_mutex); > + return strnlen(buf, count); > +out_unlock: > + mutex_unlock(&dynamic_netconsole_mutex); > + return err; > +} > + Best Regards, Petr
On Fri, Jul 14, 2023 at 01:41:55PM +0200, Petr Mladek wrote: > On Fri 2023-07-07 06:29:11, Breno Leitao wrote: > > @@ -254,6 +267,11 @@ static ssize_t extended_show(struct config_item *item, char *buf) > > return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->extended); > > } > > > > +static ssize_t release_show(struct config_item *item, char *buf) > > +{ > > + return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->release); > > I have learned recently that sysfs_emit() was preferred over snprintf() in the > _show() callbacks. I didn't know either, I just read about it in the thread. Thanks for the heads up. We probably want to change it for the other _show() structs. > > +} > > + > > static ssize_t dev_name_show(struct config_item *item, char *buf) > > { > > return snprintf(buf, PAGE_SIZE, "%s\n", to_target(item)->np.dev_name); > > @@ -366,6 +389,38 @@ static ssize_t enabled_store(struct config_item *item, > > return err; > > } > > > > +static ssize_t release_store(struct config_item *item, const char *buf, > > + size_t count) > > +{ > > + struct netconsole_target *nt = to_target(item); > > + int release; > > + int err; > > + > > + mutex_lock(&dynamic_netconsole_mutex); > > + if (nt->enabled) { > > + pr_err("target (%s) is enabled, disable to update parameters\n", > > + config_item_name(&nt->item)); > > + err = -EINVAL; > > + goto out_unlock; > > + } > > + > > + err = kstrtoint(buf, 10, &release); > > + if (err < 0) > > + goto out_unlock; > > + if (release < 0 || release > 1) { > > + err = -EINVAL; > > + goto out_unlock; > > + } > > You might consider using: > > bool enabled; > > err = kstrtobool(buf, &enabled); > if (err) > goto unlock; > > > It accepts more input values, for example, 1/0, y/n, Y/N, ... > > Well, I see that kstrtoint() is used also in enabled_store(). > It might be confusing when "/enabled" supports only "1/0" > and "/release" supports more variants. Right. we probably want to move a few _stores to kstrtobool(). Here is what I have in mind: * enabled_store() * release_store() * extended_store() That said, there are two ways moving forward: 1) I forward fix it. I've send v3 earlier today[1], I can send a patch on top of it. 2) I fix this in a v4 patch. Probably a patchset of 3 patches: a) Move the current snprintf to emit_sysfs() b) Move kstrtoint() to kstrtobool() c) This new feature using emit_sysfs() and kstrtobool(). What is the best way moving forward? Thanks for the review! [1] Link: https://lore.kernel.org/all/20230714111330.3069605-1-leitao@debian.org/
diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst index dd0518e002f6..7a9de0568e84 100644 --- a/Documentation/networking/netconsole.rst +++ b/Documentation/networking/netconsole.rst @@ -13,6 +13,8 @@ IPv6 support by Cong Wang <xiyou.wangcong@gmail.com>, Jan 1 2013 Extended console support by Tejun Heo <tj@kernel.org>, May 1 2015 +Release prepend support by Breno Leitao <leitao@debian.org>, Jul 7 2023 + Please send bug reports to Matt Mackall <mpm@selenic.com> Satyam Sharma <satyam.sharma@gmail.com>, and Cong Wang <xiyou.wangcong@gmail.com> @@ -34,10 +36,11 @@ Sender and receiver configuration: It takes a string configuration parameter "netconsole" in the following format:: - netconsole=[+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr] + netconsole=[+][r][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr] where + if present, enable extended console support + 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) @@ -125,6 +128,7 @@ The interface exposes these parameters of a netconsole target to userspace: ============== ================================= ============ enabled Is this target currently enabled? (read-write) extended Extended mode enabled (read-write) + release Prepend kernel release to message (read-write) dev_name Local network interface name (read-write) local_port Source UDP port to use (read-write) remote_port Remote agent's UDP port (read-write) @@ -165,6 +169,11 @@ following format which is the same as /dev/kmsg:: <level>,<sequnum>,<timestamp>,<contflag>;<message text> +If 'r' (release) feature is enabled, the kernel release version is +prepended to the start of the message. Example:: + + 6.4.0,6,444,501151268,-;netconsole: network logging started + Non printable characters in <message text> are escaped using "\xff" notation. If the message contains optional dictionary, verbatim newline is used as the delimiter. diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 4f4f79532c6c..a72f471ddf02 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -36,6 +36,7 @@ #include <linux/inet.h> #include <linux/configfs.h> #include <linux/etherdevice.h> +#include <linux/utsname.h> MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>"); MODULE_DESCRIPTION("Console driver for network interfaces"); @@ -84,6 +85,8 @@ static struct console netconsole_ext; * Also, other parameters of a target may be modified at * runtime only when it is disabled (enabled == 0). * @extended: Denotes whether console is extended or not. + * @release: Denotes whether kernel release version should be prepended + * to the message. Depends on extended console. * @np: The netpoll structure for this target. * Contains the other userspace visible parameters: * dev_name (read-write) @@ -101,6 +104,7 @@ struct netconsole_target { #endif bool enabled; bool extended; + bool release; struct netpoll np; }; @@ -188,6 +192,14 @@ static struct netconsole_target *alloc_param_target(char *target_config) target_config++; } + if (*target_config == 'r') { + if (!nt->extended) + pr_err("Not enabling release feature, since extended message is disabled\n"); + else + nt->release = true; + target_config++; + } + /* Parse parameters and setup netpoll */ err = netpoll_parse_options(&nt->np, target_config); if (err) @@ -222,6 +234,7 @@ static void free_param_target(struct netconsole_target *nt) * | * <target>/ * | enabled + * | release * | dev_name * | local_port * | remote_port @@ -254,6 +267,11 @@ static ssize_t extended_show(struct config_item *item, char *buf) return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->extended); } +static ssize_t release_show(struct config_item *item, char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", to_target(item)->release); +} + static ssize_t dev_name_show(struct config_item *item, char *buf) { return snprintf(buf, PAGE_SIZE, "%s\n", to_target(item)->np.dev_name); @@ -332,6 +350,11 @@ static ssize_t enabled_store(struct config_item *item, } if (enabled) { /* true */ + if (nt->release && !nt->extended) { + pr_err("release feature requires extended log message\n"); + goto out_unlock; + } + if (nt->extended && !console_is_registered(&netconsole_ext)) register_console(&netconsole_ext); @@ -366,6 +389,38 @@ static ssize_t enabled_store(struct config_item *item, return err; } +static ssize_t release_store(struct config_item *item, const char *buf, + size_t count) +{ + struct netconsole_target *nt = to_target(item); + int release; + int err; + + mutex_lock(&dynamic_netconsole_mutex); + if (nt->enabled) { + pr_err("target (%s) is enabled, disable to update parameters\n", + config_item_name(&nt->item)); + err = -EINVAL; + goto out_unlock; + } + + err = kstrtoint(buf, 10, &release); + if (err < 0) + goto out_unlock; + if (release < 0 || release > 1) { + err = -EINVAL; + goto out_unlock; + } + + nt->release = release; + + mutex_unlock(&dynamic_netconsole_mutex); + return strnlen(buf, count); +out_unlock: + mutex_unlock(&dynamic_netconsole_mutex); + return err; +} + static ssize_t extended_store(struct config_item *item, const char *buf, size_t count) { @@ -576,10 +631,12 @@ CONFIGFS_ATTR(, local_ip); CONFIGFS_ATTR(, remote_ip); CONFIGFS_ATTR_RO(, local_mac); CONFIGFS_ATTR(, remote_mac); +CONFIGFS_ATTR(, release); static struct configfs_attribute *netconsole_target_attrs[] = { &attr_enabled, &attr_extended, + &attr_release, &attr_dev_name, &attr_local_port, &attr_remote_port, @@ -772,9 +829,23 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg, const char *header, *body; int offset = 0; int header_len, body_len; + const char *msg_ready = msg; + const char *release; + int release_len = 0; + + if (nt->release) { + release = init_utsname()->release; + release_len = strlen(release) + 1; + } - if (msg_len <= MAX_PRINT_CHUNK) { - netpoll_send_udp(&nt->np, msg, msg_len); + if (msg_len + release_len <= MAX_PRINT_CHUNK) { + /* No fragmentation needed */ + if (nt->release) { + scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg); + msg_len += release_len; + msg_ready = buf; + } + netpoll_send_udp(&nt->np, msg_ready, msg_len); return; } @@ -792,7 +863,10 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg, * Transfer multiple chunks with the following extra header. * "ncfrag=<byte-offset>/<total-bytes>" */ - memcpy(buf, header, header_len); + if (nt->release) + scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release); + memcpy(buf + release_len, header, header_len); + header_len += release_len; while (offset < body_len) { int this_header = header_len;
Create a new netconsole runtime option that prepends the kernel version in the netconsole message. This is useful to map kernel messages to kernel version in a simple way, i.e., without checking somewhere which kernel version the host that sent the message is using. If this option is selected, then the "<release>," is prepended before the netconsole message. This is an example of a netconsole output, with release feature enabled: 6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test Calvin Owens send a RFC about this problem in 2016[1], but his approach was a bit more intrusive, changing the printk subsystem. This approach is lighter, and just append the information in the last mile, just before netconsole push the message to netpoll. [1] Link: https://lore.kernel.org/all/51047c0f6e86abcb9ee13f60653b6946f8fcfc99.1463172791.git.calvinowens@fb.com/ Cc: Dave Jones <davej@codemonkey.org.uk> Signed-off-by: Breno Leitao <leitao@debian.org> --- V1 -> V2: * Configured at runtime instead of at compilation time * Reuse send_ext_msg_udp to avoid extra memory allocation --- Documentation/networking/netconsole.rst | 11 +++- drivers/net/netconsole.c | 80 ++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 4 deletions(-)