Re: [PATCH net-next v4 1/6] ibmvnic: Move long delayed work on system_dfl_long_wq

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not match all of the diff.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  edits a call inside the affected critical section.
- [High] This problem wasn't introduced by this patch, but
  ibmvnic_reset() can call ibmvnic_close(), which may sleep, from atomic…
- [Medium] This problem wasn't introduced by this patch, but device
  removal can leak queued reset requests.

--

ibmvnic: Move long delayed work on system_dfl_long_wq

This commit moves the ibmvnic reset work items from the per-cpu
system_long_wq to the unbound system_dfl_long_wq. It converts the two
queue_delayed_work() calls in __ibmvnic_reset() and the queue_work() call
in ibmvnic_reset().

> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 5a510eed335ea..d4c284c8ef437 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c

[ ... ]

> @@ -3267,7 +3267,7 @@ static void __ibmvnic_reset(struct work_struct *work)
>  	spin_lock(&adapter->rwi_lock);
>  	if (!list_empty(&adapter->rwi_list)) {
>  		if (test_and_set_bit_lock(0, &adapter->resetting)) {
> -			queue_delayed_work(system_long_wq,
> +			queue_delayed_work(system_dfl_long_wq,
>  					   &adapter->ibmvnic_delayed_reset,
>  					   IBMVNIC_RESET_DELAY);
>  		} else {

[Severity: High]
This isn't a bug introduced by this patch, but can this plain spin_lock()
on adapter->rwi_lock deadlock against softirq users of the same lock?

__ibmvnic_reset() runs as a work item in process context with BH enabled.
ibmvnic_reset() takes the same lock from softirq context:

ibmvnic_tasklet()
  spin_lock_irqsave(&queue->lock, flags);
  ibmvnic_handle_crq()
    ibmvnic_reset(adapter, VNIC_RESET_MOBILITY);
      spin_lock_irqsave(&adapter->rwi_lock, flags);

The CRQ_INIT and fatal XPORT events in ibmvnic_handle_crq() reach
ibmvnic_reset() the same way. So does the tx watchdog path,
dev_watchdog()->ibmvnic_tx_timeout()->ibmvnic_reset().

Suppose a CRQ interrupt arrives on the worker's CPU while this section
holds rwi_lock. The tasklet can then run on irq_exit and spin forever in
ibmvnic_reset(), with IRQs disabled and crq.lock held.

Would this need spin_lock_bh(), or spin_lock_irqsave() like the other
users? Lockdep should report this as inconsistent {IN-SOFTIRQ-W} ->
{SOFTIRQ-ON-W} usage.

The plain spin_lock() in ibmvnic_remove() looks fine, because it is nested
inside spin_lock_irqsave(&adapter->state_lock, flags).

The plain spin_lock() here appears to date back to commit fd98693cb072
("ibmvnic: Allow queueing resets during probe").

[ ... ]

> @@ -3454,7 +3454,7 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
>  	list_add_tail(&rwi->list, &adapter->rwi_list);

[Severity: Medium]
This is a pre-existing issue, but can the struct ibmvnic_rwi entries added
to adapter->rwi_list here leak when the device is removed?

ibmvnic_remove() sets adapter->state = VNIC_REMOVING and then does:

	flush_work(&adapter->ibmvnic_reset);
	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
	...
	free_netdev(netdev);

__ibmvnic_reset() returns early without touching the list:

	if (adapter->state == VNIC_REMOVING)
		return;

If the worker is already inside its loop, it frees only the current rwi
and breaks out.

ibmvnic_remove() never calls flush_reset_queue(). Are the remaining
entries lost when free_netdev() frees the adapter? The probe failure path
does call flush_reset_queue() before free_netdev().

>  	netdev_dbg(adapter->netdev, "Scheduling reset (reason %s)\n",
>  		   reset_reason_to_string(reason));
> -	queue_work(system_long_wq, &adapter->ibmvnic_reset);
> +	queue_work(system_dfl_long_wq, &adapter->ibmvnic_reset);

[Severity: Low]
This isn't a bug, but the commit message doesn't seem to cover this hunk:

  ibmvnic: Move long delayed work on system_dfl_long_wq

  Currently the code enqueue work items using {queue|mod}_delayed_work(),
  using system_long_wq.

  The function(s) end up calling __queue_delayed_work(), which set a global
  timer that could fire anywhere, enqueuing the work where the timer fired.

ibmvnic doesn't call mod_delayed_work(). Also, this call site is a plain
queue_work() of ibmvnic_reset, not delayed work, so the timer argument
doesn't apply to it.

Every reset request (CRQ events, tx timeout, change-param, sysfs failover)
goes through this call. Before, it ran on the CPU that submitted it. Now
it runs on an unbound pool.

Could the commit message also mention this conversion?

>  
>  	ret = 0;
>  err:

[Severity: High]
This is a pre-existing issue, but can ibmvnic_reset() sleep in atomic
context on the ENOMEM path just below?

	rwi = kzalloc_obj(*rwi, GFP_ATOMIC);
	if (!rwi) {
		ret = ENOMEM;
		goto err;
	}
	...
err:
	/* ibmvnic_close() below can block, so drop the lock first */
	spin_unlock_irqrestore(&adapter->rwi_lock, flags);

	if (ret == ENOMEM)
		ibmvnic_close(netdev);

For most callers, dropping rwi_lock does not leave atomic context.
ibmvnic_tasklet() holds adapter->crq.lock with spin_lock_irqsave() around
ibmvnic_handle_crq(). ibmvnic_tx_timeout() runs from the dev_watchdog
timer.

When failover_pending is false, the close path can block:

ibmvnic_close()
  __ibmvnic_close()
    set_link_state()
      wait_for_completion_timeout(&adapter->init_done, timeout);
  ibmvnic_cleanup()
    synchronize_rcu();

Wouldn't this trigger "scheduling while atomic"? Also, init_done is
completed by CRQ processing in that same tasklet, which is the context
blocked here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095452.135828-1-marco.crivellari%40suse.com