Hello Andre,

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

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@debian.org/ (local)

+
+	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);

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