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

[Paolo Abeni <pabeni@redhat.com>]

On 7/6/26 1:55 PM, Breno Leitao wrote:
> On Thu, Jul 02, 2026 at 05:19:44AM -0700, Breno Leitao wrote:
>> This work continue to untangle netconsole and netpoll, improving
>> memory usage for netpoll users that are not netconsole.
> 
> Quick follow up on the sashiko findings. most of them are assuming that
> netconsole_write is called locklelly, which is not the case, except
> on panic (CON_NBCON_ATOMIC_UNSAFE). 
> 
> 
> There are in order as they appear in
> https://sashiko.dev/#/patchset/20260702-netconsole_move_more-v2-0-1ebedd921dcb%40debian.org
> 
> Tl;DR: There are two pre-existing issue that I will get fixed, but
> I don't think they are blockers for this series (IMO).
> 
> Details of the findings and analyzes:
> 
> 1) This isn't a bug introduced by this patch, but is it safe to modify
>    target_list using standard list primitives while netconsole_write()
>    iterates over it locklessly?
> 
> netconsole_write() is never called without the lock, unless on
> emergency, which is known to be unsafe (CON_NBCON_ATOMIC_UNSAFE).
> 
> Sashiko doesn't undersatnd that nbcon .device_lock
> (netconsole_device_lock), is called before netconsole_write
> 
> 2) This is a pre-existing issue, but I noticed a potential race
>    condition between netdevice events and configfs rmdir that skips
>    cleanup. If netconsole_netdev_event() processes NETDEV_RELEASE or
>    NETDEV_JOIN, it sets nt->state = STATE_DISABLED and moves the target
>    to target_cleanup_list:
> 
> This is a pre-existing issue, and I am happy to fix in here or as
> a follow-up
> 
> 
> 3) This isn't a bug introduced by this patch, but I found a potential
>    lockless iteration race in netconsole_write() in this file.
> 
> Again, netconsole_write() is not called locklelssly. 
> 
> 
> 4) This is a pre-existing issue, but does lockless iteration of
>    target_list in netconsole_write() race with non-RCU list
>    modifications?
> 
> Same as above.
> 
> 5) This is a pre-existing issue, but is there a race condition between
>    interface disablement and configfs removal that skips cleanup?
> 
> This is a pre-existing issue, and I am happy to fix in here or as
> a follow-up.
> 
> 6) This is a pre-existing issue, but does skb_dequeue() acquire
>    a spinlock_t in NMI contexts?
> 
> netconsole is not used on NMI contexts, unless on emergency
> (CON_NBCON_ATOMIC_UNSAFE).
> 
> 7) When netconsole_write() is called as an atomic console callback, it
>    iterates target_list locklessly. Concurrently, if a privileged user
>    removes the target via configfs:
> 
> netconsole_write() is only called on atomic context on emergencies.
> There is no conflict with configfs.
> 
> 8) This isn't a bug introduced by this patch, but could the lockless
>    iteration of target_list in netconsole_write() race with list
>    removal? In drivers/net/netconsole.c:netconsole_write():
> 
> Same as above, netconsole_write() are not called locklessly except on
> ops.
I think you forgot the following comment, which is BTW the only not
clearly marked as a pre-existing one.

Still looks like a pre-existing problem to me.

---
> @@ -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.
---