Re: [PATCH net-next v2 0/8] netconsole: stop charging netpoll users for netconsole-only data

[Breno Leitao <leitao@debian.org>]

On Thu, Jul 09, 2026 at 12:20:24PM +0200, Paolo Abeni wrote:
> > @@ -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