On Thu, Jul 09, 2026 at 12:20:24PM +0200, Paolo Abeni wrote:

quoted

@@ -335,17 +340,15 @@ static void refill_skbs_work_handler(struct work_struct *work)
 static void netconsole_skb_pool_init(struct netconsole_target *nt)
 {
-	skb_queue_head_init(&nt->np.skb_pool);
-	INIT_WORK(&nt->np.refill_wq, refill_skbs_work_handler);
-	refill_skbs(&nt->np);
+	skb_queue_head_init(&nt->skb_pool);
+	INIT_WORK(&nt->refill_wq, refill_skbs_work_handler);
+	refill_skbs(nt);
 }

Can this race with target teardown?
If a network device linked to a deactivated target is unregistered, it queues
the target on target_cleanup_list and schedules
netconsole_process_cleanups_core(), which executes netconsole_skb_pool_flush()
under rtnl_lock.

Let me think about it.

Sashiko said:

Can this race with target teardown?
If a network device linked to a deactivated target is unregistered, it
queues the target on target_cleanup_list and schedules
netconsole_process_cleanups_core(), which executes
netconsole_skb_pool_flush() under rtnl_lock.

Correct. This is the summary of the code:



static void netconsole_process_cleanups_core(void) {
	ASSERT_RTNL();
	mutex_lock(&target_cleanup_list_lock);
	list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list) {
		netconsole_skb_pool_flush(nt);
	..
}

Concurrently, if a user enables the target via configfs
enabled_store(), netconsole_skb_pool_init() is called.

Correct. The code is:

static ssize_t enabled_store(struct config_item *item,
	dynamic_netconsole_mutex_lock();
	netconsole_skb_pool_init(nt);
	...
}

Because this initialization happens before acquiring rtnl_lock inside
netpoll_setup(), enabled_store() can execute INIT_WORK() and
skb_queue_head_init() simultaneously with the cleanup thread executing
cancel_work_sync() and skb_queue_purge_reason() on the exact same
fields.

So, it seems they can execute in parallel, given that the device might
be in the cleanup list, and, configfs might be toggling it up.

I don't think this is a big issue, given worst case scenario, the pool
will not be populated, but this seems a clear regression.

Does this initialization need to be moved inside the rtnl_lock
protected region to avoid data corruption?

This refactor genuinely did move the pool init out of netpoll_setup()'s
RTNL coverage — before the series, __netpoll_setup() did the
skb_queue_head_init/INIT_WORK under RTNL, giving blanket mutual
exclusion with the notifier.

I don't think I want to have the initialization under RTNL, given this
is a heavy lift. At the same time, I don't have a clear view on how to
solve it. Maybe getting the target_cleanup_list_lock() at skb pool
initializion (which seems ugly as hell).

Anyway, let me spend some tokens on it, and see if I can figure out
a better plan.

Thanks for raising this up,
--breno

--
pw-bot: cr