This patch refactors the netconsole driver's target enabled state from a simple boolean to an explicit enum (`target_state`).

This allow the states to be expanded to a new state in the upcoming change.

Co-developed-by: Breno Leitao leitao@debian.org Signed-off-by: Breno Leitao leitao@debian.org Signed-off-by: Andre Carvalho asantostc@gmail.com --- drivers/net/netconsole.c | 52 ++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index b5e6a9fdc3155196d1fd7e81def584360ecbc3d2..688ed670b37b56ab4a03d43fb3de94ca0e6a8360 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -132,12 +132,12 @@ enum target_state { * @sysdata_fields: Sysdata features enabled. * @msgcounter: Message sent counter. * @stats: Packet send stats for the target. Used for debugging. - * @enabled: On / off knob to enable / disable target. + * @state: State of the target. * Visible from userspace (read-write). * We maintain a strict 1:1 correspondence between this and * whether the corresponding netpoll is active or inactive. * Also, other parameters of a target may be modified at - * runtime only when it is disabled (enabled == 0). + * runtime only when it is disabled (state == STATE_DISABLED). * @extended: Denotes whether console is extended or not. * @release: Denotes whether kernel release version should be prepended * to the message. Depends on extended console. @@ -165,7 +165,7 @@ struct netconsole_target { u32 msgcounter; #endif struct netconsole_target_stats stats; - bool enabled; + enum target_state state; bool extended; bool release; struct netpoll np; @@ -257,6 +257,7 @@ static struct netconsole_target *alloc_and_init(void) nt->np.local_port = 6665; nt->np.remote_port = 6666; eth_broadcast_addr(nt->np.remote_mac); + nt->state = STATE_DISABLED;

return nt; } @@ -275,7 +276,7 @@ static void netconsole_process_cleanups_core(void) mutex_lock(&target_cleanup_list_lock); list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list) { /* all entries in the cleanup_list needs to be disabled */ - WARN_ON_ONCE(nt->enabled); + WARN_ON_ONCE(nt->state == STATE_ENABLED); do_netpoll_cleanup(&nt->np); /* moved the cleaned target to target_list. Need to hold both * locks @@ -398,7 +399,7 @@ static void trim_newline(char *s, size_t maxlen)

static ssize_t enabled_show(struct config_item *item, char *buf) { - return sysfs_emit(buf, "%d\n", to_target(item)->enabled); + return sysfs_emit(buf, "%d\n", to_target(item)->state == STATE_ENABLED); }

static ssize_t extended_show(struct config_item *item, char *buf) @@ -565,8 +566,8 @@ static ssize_t enabled_store(struct config_item *item, const char *buf, size_t count) { struct netconsole_target *nt = to_target(item); + bool enabled, current_enabled; unsigned long flags; - bool enabled; ssize_t ret;

mutex_lock(&dynamic_netconsole_mutex); @@ -575,9 +576,10 @@ static ssize_t enabled_store(struct config_item *item, goto out_unlock;

ret = -EINVAL; - if (enabled == nt->enabled) { + current_enabled = nt->state == STATE_ENABLED; + if (enabled == current_enabled) { pr_info("network logging has already %s\n", - nt->enabled ? "started" : "stopped"); + current_enabled ? "started" : "stopped"); goto out_unlock; }

@@ -610,16 +612,16 @@ static ssize_t enabled_store(struct config_item *item, if (ret) goto out_unlock;

- nt->enabled = true; + nt->state = STATE_ENABLED; pr_info("network logging started\n"); } else { /* false */ /* We need to disable the netconsole before cleaning it up * otherwise we might end up in write_msg() with - * nt->np.dev == NULL and nt->enabled == true + * nt->np.dev == NULL and nt->state == STATE_ENABLED */ mutex_lock(&target_cleanup_list_lock); spin_lock_irqsave(&target_list_lock, flags); - nt->enabled = false; + nt->state = STATE_DISABLED; /* Remove the target from the list, while holding * target_list_lock */ @@ -648,7 +650,7 @@ static ssize_t release_store(struct config_item *item, const char *buf, ssize_t ret;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); ret = -EINVAL; @@ -675,7 +677,7 @@ static ssize_t extended_store(struct config_item *item, const char *buf, ssize_t ret;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); ret = -EINVAL; @@ -699,7 +701,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf, struct netconsole_target *nt = to_target(item);

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); mutex_unlock(&dynamic_netconsole_mutex); @@ -720,7 +722,7 @@ static ssize_t local_port_store(struct config_item *item, const char *buf, ssize_t ret = -EINVAL;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); goto out_unlock; @@ -742,7 +744,7 @@ static ssize_t remote_port_store(struct config_item *item, ssize_t ret = -EINVAL;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); goto out_unlock; @@ -765,7 +767,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf, int ipv6;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); goto out_unlock; @@ -790,7 +792,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf, int ipv6;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); goto out_unlock; @@ -839,7 +841,7 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf, ssize_t ret = -EINVAL;

mutex_lock(&dynamic_netconsole_mutex); - if (nt->enabled) { + if (nt->state == STATE_ENABLED) { pr_err("target (%s) is enabled, disable to update parameters\n", config_item_name(&nt->group.cg_item)); goto out_unlock; @@ -1315,7 +1317,7 @@ static void drop_netconsole_target(struct config_group *group, * The target may have never been enabled, or was manually disabled * before being removed so netpoll may have already been cleaned up. */ - if (nt->enabled) + if (nt->state == STATE_ENABLED) netpoll_cleanup(&nt->np);

config_item_put(&nt->group.cg_item); @@ -1444,7 +1446,7 @@ static int netconsole_netdev_event(struct notifier_block *this, case NETDEV_RELEASE: case NETDEV_JOIN: case NETDEV_UNREGISTER: - nt->enabled = false; + nt->state = STATE_DISABLED; list_move(&nt->list, &target_cleanup_list); stopped = true; } @@ -1711,7 +1713,8 @@ static void write_ext_msg(struct console *con, const char *msg,

spin_lock_irqsave(&target_list_lock, flags); list_for_each_entry(nt, &target_list, list) - if (nt->extended && nt->enabled && netif_running(nt->np.dev)) + if (nt->extended && nt->state == STATE_ENABLED && + netif_running(nt->np.dev)) send_ext_msg_udp(nt, msg, len); spin_unlock_irqrestore(&target_list_lock, flags); } @@ -1731,7 +1734,8 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)

spin_lock_irqsave(&target_list_lock, flags); list_for_each_entry(nt, &target_list, list) { - if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) { + if (!nt->extended && nt->state == STATE_ENABLED && + netif_running(nt->np.dev)) { /* * We nest this inside the for-each-target loop above * so that we're able to get as much logging out to @@ -1887,7 +1891,7 @@ static struct netconsole_target *alloc_param_target(char *target_config, */ goto fail; } else { - nt->enabled = true; + nt->state = STATE_ENABLED; } populate_configfs_item(nt, cmdline_count);

Attempt to resume a previously deactivated target when the associated interface comes back (NETDEV_UP event is received).

Target transitions to STATE_DISABLED in case of failures resuming it to avoid retrying the same target indefinitely.

Signed-off-by: Andre Carvalho asantostc@gmail.com --- drivers/net/netconsole.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 59d770bb4baa5f9616b10c0dfb39ed45a4eb7710..397e6543b3d9aeda6da450823adee09cb3e9ae70 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -135,10 +135,12 @@ enum target_state { * @stats: Packet send stats for the target. Used for debugging. * @state: State of the target. * Visible from userspace (read-write). - * We maintain a strict 1:1 correspondence between this and - * whether the corresponding netpoll is active or inactive. + * From a userspace perspective, the target is either enabled or + * disabled. Internally, although both STATE_DISABLED and + * STATE_DEACTIVATED correspond to inactive netpoll the latter is + * due to interface state changes and may recover automatically. * Also, other parameters of a target may be modified at - * runtime only when it is disabled (state == STATE_DISABLED). + * runtime only when it is disabled (state != STATE_ENABLED). * @extended: Denotes whether console is extended or not. * @release: Denotes whether kernel release version should be prepended * to the message. Depends on extended console. @@ -172,6 +174,7 @@ struct netconsole_target { struct netpoll np; /* protected by target_list_lock */ char buf[MAX_PRINT_CHUNK]; + struct work_struct resume_wq; };

#ifdef CONFIG_NETCONSOLE_DYNAMIC @@ -237,6 +240,33 @@ static void populate_configfs_item(struct netconsole_target *nt, } #endif /* CONFIG_NETCONSOLE_DYNAMIC */

+/* Attempts to resume logging to a deactivated target. */ +static void resume_target(struct netconsole_target *nt) +{ + int ret; + + if (nt->state != STATE_DEACTIVATED) + return; + + ret = netpoll_setup(&nt->np); + if (ret) { + /* netpoll fails to register once, do not try again. */ + nt->state = STATE_DISABLED; + } else { + pr_info("network logging resumed on interface %s\n", + nt->np.dev_name); + nt->state = STATE_ENABLED; + } +} + +static void resume_work_handler(struct work_struct *work) +{ + struct netconsole_target *nt = + container_of(work, struct netconsole_target, resume_wq); + + resume_target(nt); +} + /* Allocate and initialize with defaults. * Note that these targets get their config_item fields zeroed-out. */ @@ -260,6 +290,8 @@ static struct netconsole_target *alloc_and_init(void) eth_broadcast_addr(nt->np.remote_mac); nt->state = STATE_DISABLED;

+ INIT_WORK(&nt->resume_wq, resume_work_handler); + return nt; }

@@ -1440,7 +1472,8 @@ static int netconsole_netdev_event(struct notifier_block *this, bool stopped = false;

if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER || - event == NETDEV_RELEASE || event == NETDEV_JOIN)) + event == NETDEV_RELEASE || event == NETDEV_JOIN || + event == NETDEV_UP)) goto done;

mutex_lock(&target_cleanup_list_lock); @@ -1460,6 +1493,10 @@ static int netconsole_netdev_event(struct notifier_block *this, stopped = true; } } + if (nt->state == STATE_DEACTIVATED && event == NETDEV_UP) { + if (!strncmp(nt->np.dev_name, dev->name, IFNAMSIZ)) + schedule_work(&nt->resume_wq); + } netconsole_target_put(nt); } spin_unlock_irqrestore(&target_list_lock, flags); @@ -1915,6 +1952,7 @@ static struct netconsole_target *alloc_param_target(char *target_config, static void free_param_target(struct netconsole_target *nt) { netpoll_cleanup(&nt->np); + cancel_work_sync(&nt->resume_wq); kfree(nt); }

Introduce a new netconsole selftest to validate that netconsole is able to resume a deactivated target when the low level interface comes back.

The test setups the network using netdevsim, creates a netconsole target and then remove/add netdevsim in order to bring the same interfaces back. Afterwards, the test validates that the target works as expected.

Signed-off-by: Andre Carvalho asantostc@gmail.com --- tools/testing/selftests/drivers/net/Makefile | 1 + .../selftests/drivers/net/lib/sh/lib_netcons.sh | 23 ++++++++ .../selftests/drivers/net/netcons_resume.sh | 68 ++++++++++++++++++++++ 3 files changed, 92 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile index 984ece05f7f92e836592107ba4c692da6d8ce1b3..f47c4d57f7b4ce82b0b59bee4c87a9660819675e 100644 --- a/tools/testing/selftests/drivers/net/Makefile +++ b/tools/testing/selftests/drivers/net/Makefile @@ -17,6 +17,7 @@ TEST_PROGS := \ netcons_fragmented_msg.sh \ netcons_overflow.sh \ netcons_sysdata.sh \ + netcons_resume.sh \ netpoll_basic.py \ ping.py \ queues.py \ diff --git a/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh b/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh index 8e1085e896472d5c87ec8b236240878a5b2d00d2..ba7c865b1be3b60f53ea548aba269059ca74aee6 100644 --- a/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh +++ b/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh @@ -350,6 +350,29 @@ function check_netconsole_module() { fi }

+function wait_target_state() { + local TARGET=${1} + local STATE=${2} + local FILENAME="${NETCONS_CONFIGFS}"/"${TARGET}"/"enabled" + + if [ "${STATE}" == "enabled" ] + then + ENABLED=1 + else + ENABLED=0 + fi + + if [ ! -f "$FILENAME" ]; then + echo "FAIL: Target does not exist." >&2 + exit "${ksft_fail}" + fi + + slowwait 2 sh -c 'test -n "$(grep '"'${ENABLED}'"' '"'${FILENAME}'"')"' || { + echo "FAIL: ${TARGET} is not ${STATE}." >&2 + exit "${ksft_fail}" + } +} + # A wrapper to translate protocol version to udp version function wait_for_port() { local NAMESPACE=${1} diff --git a/tools/testing/selftests/drivers/net/netcons_resume.sh b/tools/testing/selftests/drivers/net/netcons_resume.sh new file mode 100755 index 0000000000000000000000000000000000000000..7e8ea74821fffdac8be0c3db2f1aa7953b4d5bd5 --- /dev/null +++ b/tools/testing/selftests/drivers/net/netcons_resume.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: GPL-2.0 + +# This test validates that netconsole is able to resume a target that was +# deactivated when its interface was removed. +# +# The test configures a netconsole dynamic target and then removes netdevsim +# module to cause the interface to disappear. The test veries that the target +# moved to disabled state before adding netdevsim and the interface back. +# +# Finally, the test verifies that the target is re-enabled automatically and +# the message is received on the destination interface. +# +# Author: Andre Carvalho asantostc@gmail.com + +set -euo pipefail + +SCRIPTDIR=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")") + +source "${SCRIPTDIR}"/lib/sh/lib_netcons.sh + +modprobe netdevsim 2> /dev/null || true +modprobe netconsole 2> /dev/null || true + +# The content of kmsg will be save to the following file +OUTPUT_FILE="/tmp/${TARGET}" + +# Check for basic system dependency and exit if not found +check_for_dependencies + +# Set current loglevel to KERN_INFO(6), and default to KERN_NOTICE(5) +echo "6 5" > /proc/sys/kernel/printk +# Remove the namespace, interfaces and netconsole target on exit +trap cleanup EXIT + +# Create one namespace and two interfaces +set_network +# Create a dynamic target for netconsole +create_dynamic_target + +# Remove low level module +rmmod netdevsim +# Target should be disabled +wait_target_state "${TARGET}" "disabled" + +# Add back low level module +modprobe netdevsim +# Recreate namespace and two interfaces +set_network +# Target should be enabled again +wait_target_state "${TARGET}" "enabled" + +# Listed for netconsole port inside the namespace and destination +# interface +listen_port_and_save_to "${OUTPUT_FILE}" & +# Wait for socat to start and listen to the port. +wait_local_port_listen "${NAMESPACE}" "${PORT}" udp +# Send the message +echo "${MSG}: ${TARGET}" > /dev/kmsg +# Wait until socat saves the file to disk +busywait "${BUSYWAIT_TIMEOUT}" test -s "${OUTPUT_FILE}" +# Make sure the message was received in the dst part +# and exit +validate_msg "${OUTPUT_FILE}" +# kill socat in case it is still running +pkill_socat + +exit "${ksft_pass}"