From: Breno Leitao leitao@debian.org

Introduces a enum to track netconsole target state which is going to replace the enabled boolean.

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

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 9cb4dfc242f5..e2ec09f238a0 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -119,6 +119,11 @@ enum sysdata_feature { MAX_SYSDATA_ITEMS = 4, };

+enum target_state { + STATE_DISABLED, + STATE_ENABLED, +}; + /** * struct netconsole_target - Represents a configured netconsole target. * @list: Links this target into the target_list.

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.

Targets are created via cmdline parameters to the module to ensure that we are able to resume targets that were bound by mac and interface name.

Reviewed-by: Breno Leitao leitao@debian.org Signed-off-by: Andre Carvalho asantostc@gmail.com --- tools/testing/selftests/drivers/net/Makefile | 1 + .../selftests/drivers/net/lib/sh/lib_netcons.sh | 35 ++++++-- .../selftests/drivers/net/netcons_resume.sh | 97 ++++++++++++++++++++++ 3 files changed, 128 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile index f5c71d993750..3eba569b3366 100644 --- a/tools/testing/selftests/drivers/net/Makefile +++ b/tools/testing/selftests/drivers/net/Makefile @@ -19,6 +19,7 @@ TEST_PROGS := \ netcons_cmdline.sh \ netcons_fragmented_msg.sh \ netcons_overflow.sh \ + netcons_resume.sh \ netcons_sysdata.sh \ netcons_torture.sh \ netpoll_basic.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 87f89fd92f8c..239f44d4a45d 100644 --- a/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh +++ b/tools/testing/selftests/drivers/net/lib/sh/lib_netcons.sh @@ -203,19 +203,21 @@ function do_cleanup() { function cleanup_netcons() { # delete netconsole dynamic reconfiguration # do not fail if the target is already disabled - if [[ ! -d "${NETCONS_PATH}" ]] + local TARGET_PATH=${1:-${NETCONS_PATH}} + + if [[ ! -d "${TARGET_PATH}" ]] then # in some cases this is called before netcons path is created return fi - if [[ $(cat "${NETCONS_PATH}"/enabled) != 0 ]] + if [[ $(cat "${TARGET_PATH}"/enabled) != 0 ]] then - echo 0 > "${NETCONS_PATH}"/enabled || true + echo 0 > "${TARGET_PATH}"/enabled || true fi # Remove all the keys that got created during the selftest - find "${NETCONS_PATH}/userdata/" -mindepth 1 -type d -delete + find "${TARGET_PATH}/userdata/" -mindepth 1 -type d -delete # Remove the configfs entry - rmdir "${NETCONS_PATH}" + rmdir "${TARGET_PATH}" }

function cleanup() { @@ -377,6 +379,29 @@ function check_netconsole_module() { fi }

+function wait_target_state() { + local TARGET=${1} + local STATE=${2} + local TARGET_PATH="${NETCONS_CONFIGFS}"/"${TARGET}" + local ENABLED=0 + + if [ "${STATE}" == "enabled" ] + then + ENABLED=1 + fi + + if [ ! -d "$TARGET_PATH" ]; then + echo "FAIL: Target does not exist." >&2 + exit "${ksft_fail}" + fi + + local CHECK_CMD="grep "$ENABLED" "$TARGET_PATH/enabled"" + slowwait 2 sh -c "test -n "$($CHECK_CMD)"" || { + 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 000000000000..383ad1149271 --- /dev/null +++ b/tools/testing/selftests/drivers/net/netcons_resume.sh @@ -0,0 +1,97 @@ +#!/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 when the interface is brought +# back up. +# +# The test configures a netconsole target and then removes netdevsim module to +# cause the interface to disappear. Targets are configured via cmdline to ensure +# targets bound by interface name and mac address can be resumed. +# The test verifies 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 +rmmod netconsole 2> /dev/null || true + +check_netconsole_module + +function cleanup() { + cleanup_netcons "${NETCONS_CONFIGFS}/cmdline0" + do_cleanup + rmmod netconsole +} + +trap cleanup EXIT + +# Run the test twice, with different cmdline parameters +for BINDMODE in "ifname" "mac" +do + echo "Running with bind mode: ${BINDMODE}" >&2 + # Set current loglevel to KERN_INFO(6), and default to KERN_NOTICE(5) + echo "6 5" > /proc/sys/kernel/printk + + # Create one namespace and two interfaces + set_network + + # Create the command line for netconsole, with the configuration from + # the function above + CMDLINE=$(create_cmdline_str "${BINDMODE}") + + # The content of kmsg will be save to the following file + OUTPUT_FILE="/tmp/${TARGET}-${BINDMODE}" + + # Load the module, with the cmdline set + modprobe netconsole "${CMDLINE}" + # Expose cmdline target in configfs + mkdir "${NETCONS_CONFIGFS}/cmdline0" + + # Target should be enabled + wait_target_state "cmdline0" "enabled" + + # Remove low level module + rmmod netdevsim + # Target should be disabled + wait_target_state "cmdline0" "disabled" + + # Add back low level module + modprobe netdevsim + # Recreate namespace and two interfaces + set_network + # Target should be enabled again + wait_target_state "cmdline0" "enabled" + + # Listen 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 + # Cleanup & unload the module + cleanup + + echo "${BINDMODE} : Test passed" >&2 +done + +trap - EXIT +exit "${EXIT_STATUS}"

Attempt to resume a previously deactivated target when the associated interface comes back (NETDEV_REGISTER) or when it changes name (NETDEV_CHANGENAME) by calling netpoll_setup on the device.

Depending on how the target was setup (by mac or interface name), the corresponding field is compared with the device being brought up. Targets that match the incoming device, are scheduled for resume on resume_wq, so that netpoll_setup is able to force the device UP.

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 | 98 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 92 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 7a1e5559fc0d..b31762f2ee45 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -39,6 +39,7 @@ #include <linux/u64_stats_sync.h> #include <linux/utsname.h> #include <linux/rtnetlink.h> +#include <linux/workqueue.h>

MODULE_AUTHOR("Matt Mackall mpm@selenic.com"); MODULE_DESCRIPTION("Console driver for network interfaces"); @@ -138,10 +139,14 @@ 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 targets, the latter is + * due to automatic interface state changes and will try + * recover automatically, if the interface comes back + * online. * 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. @@ -155,6 +160,7 @@ enum target_state { * local_mac (read-only) * remote_mac (read-write) * @buf: The buffer used to send the full msg to the network stack + * @resume_wq: Workqueue to resume deactivated target */ struct netconsole_target { struct list_head list; @@ -177,6 +183,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 @@ -242,6 +249,75 @@ static void populate_configfs_item(struct netconsole_target *nt, } #endif /* CONFIG_NETCONSOLE_DYNAMIC */

+/* Check if the target was bound by mac address. */ +static bool bound_by_mac(struct netconsole_target *nt) +{ + return is_valid_ether_addr(nt->np.dev_mac); +} + +/* Attempts to resume logging to a deactivated target. */ +static void resume_target(struct netconsole_target *nt) +{ + int ret; + + /* check if target is still deactivated as it may have been disabled + * while resume was being scheduled. + */ + if (nt->state != STATE_DEACTIVATED) + return; + + if (bound_by_mac(nt)) + /* ensure netpoll_setup will retrieve device by mac */ + memset(&nt->np.dev_name, 0, IFNAMSIZ); + + ret = netpoll_setup(&nt->np); + if (ret) { + /* netpoll fails setup once, do not try again. */ + nt->state = STATE_DISABLED; + return; + } + + nt->state = STATE_ENABLED; + pr_info("network logging resumed on interface %s\n", nt->np.dev_name); +} + +/* Checks if a deactivated target matches a device. */ +static bool deactivated_target_match(struct netconsole_target *nt, + struct net_device *ndev) +{ + if (nt->state != STATE_DEACTIVATED) + return false; + + if (bound_by_mac(nt)) + return !memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN); + return !strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ); +} + +/* Process work scheduled for target resume. */ +static void process_resume_target(struct work_struct *work) +{ + struct netconsole_target *nt = + container_of(work, struct netconsole_target, resume_wq); + unsigned long flags; + + /* resume_target is IRQ unsafe, remove target from + * target_list in order to resume it with IRQ enabled. + */ + spin_lock_irqsave(&target_list_lock, flags); + list_del_init(&nt->list); + spin_unlock_irqrestore(&target_list_lock, flags); + + resume_target(nt); + + /* At this point the target is either enabled or disabled and + * was cleaned up before getting deactivated. Either way, add it + * back to target list. + */ + spin_lock_irqsave(&target_list_lock, flags); + list_add(&nt->list, &target_list); + spin_unlock_irqrestore(&target_list_lock, flags); +} + /* Allocate and initialize with defaults. * Note that these targets get their config_item fields zeroed-out. */ @@ -264,6 +340,7 @@ static struct netconsole_target *alloc_and_init(void) nt->np.remote_port = 6666; eth_broadcast_addr(nt->np.remote_mac); nt->state = STATE_DISABLED; + INIT_WORK(&nt->resume_wq, process_resume_target);

return nt; } @@ -1434,13 +1511,14 @@ static int prepare_sysdata(struct netconsole_target *nt) static int netconsole_netdev_event(struct notifier_block *this, unsigned long event, void *ptr) { - unsigned long flags; - struct netconsole_target *nt, *tmp; struct net_device *dev = netdev_notifier_info_to_dev(ptr); + struct netconsole_target *nt, *tmp; bool stopped = false; + unsigned long flags;

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

mutex_lock(&target_cleanup_list_lock); @@ -1469,6 +1547,13 @@ static int netconsole_netdev_event(struct notifier_block *this, stopped = true; } } + if ((event == NETDEV_REGISTER || event == NETDEV_CHANGENAME) && + deactivated_target_match(nt, dev)) + /* Schedule resume on a workqueue as it will attempt + * to UP the device, which can't be done as part of this + * notifier. + */ + schedule_work(&nt->resume_wq); netconsole_target_put(nt); } spin_unlock_irqrestore(&target_list_lock, flags); @@ -1937,6 +2022,7 @@ static struct netconsole_target *alloc_param_target(char *target_config, /* Cleanup netpoll for given target (from boot/module param) and free it */ static void free_param_target(struct netconsole_target *nt) { + cancel_work_sync(&nt->resume_wq); netpoll_cleanup(&nt->np); #ifdef CONFIG_NETCONSOLE_DYNAMIC kfree(nt->userdata);

Hello Andre,

On Fri, Nov 28, 2025 at 10:08:03PM +0000, Andre Carvalho wrote:

@@ -242,6 +249,75 @@ static void populate_configfs_item(struct netconsole_target *nt, } #endif /* CONFIG_NETCONSOLE_DYNAMIC */ +/* Check if the target was bound by mac address. */ +static bool bound_by_mac(struct netconsole_target *nt) +{

• return is_valid_ether_addr(nt->np.dev_mac);

+}

+/* Attempts to resume logging to a deactivated target. */ +static void resume_target(struct netconsole_target *nt) +{

• int ret;
• /* check if target is still deactivated as it may have been disabled

• * while resume was being scheduled.
  

• */
  

This only happens if this is a dynamic target and someone is toggling the device (or even removing it, which would cause a crash I _think_).

Given you are completely lockless here, so, there is a chance you hit a TOCTOU, also.

I think you want to have dynamic_netconsole_mutex held during the operation of process_resume_target().

* mutex_lock(&dynamic_netconsole_mutex); * remove from the list * resume * re-add to the list * mutex_unlock(&dynamic_netconsole_mutex);

netconsole design has two locks: * target lock list, which protects devices getting disabled by netdev notifications * dynamic_netconsole_mutex, which protects anyone disabling and removing the target from configfs

• if (nt->state != STATE_DEACTIVATED)

• return;
  

• if (bound_by_mac(nt))

• /* ensure netpoll_setup will retrieve device by mac */
  

• memset(&nt->np.dev_name, 0, IFNAMSIZ);
  

This is a clean-up step that was missing whent the target is getting down, and htis is just a work around that doesn't belong in here.

Please move it to netconsole_process_cleanups_core(), in a separate patch.

Something as:

list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list) do_netpoll_cleanup(&nt->np); if (bound_by_mac(nt)) memset(&nt->np.dev_name, 0, IFNAMSIZ);

Ideally this should belong to do_netpoll_cleanup(), but let's keep it in netconsole_process_cleanups_core() for three reasons:

1) Bounding by mac is a netconsole concept 2) do_netpoll_cleanup() is only used by netconsole, and I plan to move it back to netconsole. Some PoC in [1] 3) bound_by_mac() should be in netconsole and we do not want to export it.

[1]: https://lore.kernel.org/all/20250902-netpoll_untangle_v3-v1-3-51a03d6411be@d...

• ret = netpoll_setup(&nt->np);
• if (ret) {

• /* netpoll fails setup once, do not try again. */
  

• nt->state = STATE_DISABLED;
  

• return;
  

• }
• nt->state = STATE_ENABLED;
• pr_info("network logging resumed on interface %s\n", nt->np.dev_name);

+}

+/* Checks if a deactivated target matches a device. */ +static bool deactivated_target_match(struct netconsole_target *nt,

• 		     struct net_device *ndev)
  

+{

• if (nt->state != STATE_DEACTIVATED)

• return false;
  

• if (bound_by_mac(nt))

• return !memcmp(nt->np.dev_mac, ndev->dev_addr, ETH_ALEN);
  

• return !strncmp(nt->np.dev_name, ndev->name, IFNAMSIZ);

+}

+/* Process work scheduled for target resume. */ +static void process_resume_target(struct work_struct *work) +{

• struct netconsole_target *nt =

• container_of(work, struct netconsole_target, resume_wq);
  

• unsigned long flags;

mutex_lock(&dynamic_netconsole_mutex); As discussed above

• /* resume_target is IRQ unsafe, remove target from

• * target_list in order to resume it with IRQ enabled.
  

• */
  

• spin_lock_irqsave(&target_list_lock, flags);
• list_del_init(&nt->list);
• spin_unlock_irqrestore(&target_list_lock, flags);
• resume_target(nt);
• /* At this point the target is either enabled or disabled and

• * was cleaned up before getting deactivated. Either way, add it
  

• * back to target list.
  

• */
  

• spin_lock_irqsave(&target_list_lock, flags);
• list_add(&nt->list, &target_list);
• spin_unlock_irqrestore(&target_list_lock, flags);

mutex_unlock(&dynamic_netconsole_mutex);

+}

/* Allocate and initialize with defaults.

• Note that these targets get their config_item fields zeroed-out.

*/ @@ -264,6 +340,7 @@ static struct netconsole_target *alloc_and_init(void) nt->np.remote_port = 6666; eth_broadcast_addr(nt->np.remote_mac); nt->state = STATE_DISABLED;

• INIT_WORK(&nt->resume_wq, process_resume_target);

It needs to be initialized earlier before the kzalloc, otherwise we might hit a similar problem to the one fixed by e5235eb6cfe0 ("net: netpoll: initialize work queue before error checks")

The code path would be: * alloc_param_target() * alloc_and_init() * kzalloc() fails and return NULL. * resume_wq() is still not initialized fail: * free_param_target() * cancel_work_sync(&nt->resume_wq); and resume_wq is not initialized

Thanks for the patch, --breno

-- pw-bot: cr