[PATCH net] net: ibmvnic: defer close from reset allocation failure

[PATCH net] net: ibmvnic: defer close from reset allocation failure

[Runyu Xiao]

ibmvnic_reset() can be called by the CRQ tasklet. If allocating the
reset work item fails, it currently calls ibmvnic_close() after
dropping rwi_lock, but ibmvnic_close() reaches napi_disable() and
other sleepable teardown paths.

Defer the close to a work item and keep the work within the adapter
lifetime. Track removal separately from the visible adapter state so
ibmvnic_close() cannot make removal look active again after remove()
has begun. Check that monotonic flag while scheduling reset and
delayed reset work, and drain all work before releasing the adapter.

Use complete_all() for probe_done because the reset and close workers
may wait for probe completion concurrently.

The issue was identified by code inspection. No runtime reproduction or
real hardware testing is available.

Fixes: ed651a10875f ("ibmvnic: Updated reset handling")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 54 ++++++++++++++++++++++++------
 drivers/net/ethernet/ibm/ibmvnic.h |  2 ++
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5a510eed33..4da3fd7ce0 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2152,6 +2152,26 @@ static int ibmvnic_close(struct net_device *netdev)
 	return rc;
 }
 
+static void ibmvnic_close_work(struct work_struct *work)
+{
+	struct ibmvnic_adapter *adapter = container_of(work,
+							struct ibmvnic_adapter,
+							ibmvnic_close_work);
+	unsigned long flags;
+	bool removing;
+
+	/* The close path can sleep, so run it outside the CRQ tasklet. */
+	wait_for_completion(&adapter->probe_done);
+	rtnl_lock();
+	spin_lock_irqsave(&adapter->rwi_lock, flags);
+	removing = adapter->removing;
+	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+
+	if (!removing)
+		ibmvnic_close(adapter->netdev);
+	rtnl_unlock();
+}
+
 /**
  * get_hdr_lens - fills list of L2/L3/L4 hdr lens
  * @hdr_field: bitfield determining needed headers
@@ -3229,9 +3249,12 @@ static void __ibmvnic_reset(struct work_struct *work)
 	if (adapter->state == VNIC_PROBING &&
 	    !wait_for_completion_timeout(&adapter->probe_done, timeout)) {
 		dev_err(dev, "Reset thread timed out on probe");
-		queue_delayed_work(system_long_wq,
-				   &adapter->ibmvnic_delayed_reset,
-				   IBMVNIC_RESET_DELAY);
+		spin_lock_irqsave(&adapter->rwi_lock, flags);
+		if (!adapter->removing)
+			queue_delayed_work(system_long_wq,
+					   &adapter->ibmvnic_delayed_reset,
+					   IBMVNIC_RESET_DELAY);
+		spin_unlock_irqrestore(&adapter->rwi_lock, flags);
 		return;
 	}
 
@@ -3265,7 +3288,7 @@ static void __ibmvnic_reset(struct work_struct *work)
 	 */
 	need_reset = false;
 	spin_lock(&adapter->rwi_lock);
-	if (!list_empty(&adapter->rwi_list)) {
+	if (!adapter->removing && !list_empty(&adapter->rwi_list)) {
 		if (test_and_set_bit_lock(0, &adapter->resetting)) {
 			queue_delayed_work(system_long_wq,
 					   &adapter->ibmvnic_delayed_reset,
@@ -3422,7 +3445,8 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 	 * a failover reset scheduled, we will detect and drop the
 	 * duplicate reset when walking the ->rwi_list below.
 	 */
-	if (adapter->state == VNIC_REMOVING ||
+	if (adapter->removing ||
+	    adapter->state == VNIC_REMOVING ||
 	    adapter->state == VNIC_REMOVED ||
 	    (adapter->failover_pending && reason != VNIC_RESET_FAILOVER)) {
 		ret = EBUSY;
@@ -3458,11 +3482,11 @@ static int ibmvnic_reset(struct ibmvnic_adapter *adapter,
 
 	ret = 0;
 err:
-	/* ibmvnic_close() below can block, so drop the lock first */
-	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
-
 	if (ret == ENOMEM)
-		ibmvnic_close(netdev);
+		queue_work(system_long_wq, &adapter->ibmvnic_close_work);
+
+	/* ibmvnic_close() can block, so defer it out of atomic context. */
+	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
 
 	return -ret;
 }
@@ -6466,6 +6490,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	SET_NETDEV_DEV(netdev, &dev->dev);
 
 	INIT_WORK(&adapter->ibmvnic_reset, __ibmvnic_reset);
+	INIT_WORK(&adapter->ibmvnic_close_work, ibmvnic_close_work);
 	INIT_DELAYED_WORK(&adapter->ibmvnic_delayed_reset,
 			  __ibmvnic_delayed_reset);
 	INIT_LIST_HEAD(&adapter->rwi_list);
@@ -6568,7 +6593,8 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 		goto cpu_notif_add_failed;
 	}
 
-	complete(&adapter->probe_done);
+	complete_all(&adapter->probe_done);
+	flush_work(&adapter->ibmvnic_close_work);
 
 	return 0;
 
@@ -6591,10 +6617,14 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	/* cleanup worker thread after releasing CRQ so we don't get
 	 * transport events (i.e new work items for the worker thread).
 	 */
+	spin_lock_irqsave(&adapter->rwi_lock, flags);
+	adapter->removing = true;
 	adapter->state = VNIC_REMOVING;
-	complete(&adapter->probe_done);
+	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+	complete_all(&adapter->probe_done);
 	flush_work(&adapter->ibmvnic_reset);
 	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
+	flush_work(&adapter->ibmvnic_close_work);
 
 	flush_reset_queue(adapter);
 
@@ -6620,6 +6650,7 @@ static void ibmvnic_remove(struct vio_dev *dev)
 	 * from the flush_work() below, can make progress.
 	 */
 	spin_lock(&adapter->rwi_lock);
+	adapter->removing = true;
 	adapter->state = VNIC_REMOVING;
 	spin_unlock(&adapter->rwi_lock);
 
@@ -6629,6 +6660,7 @@ static void ibmvnic_remove(struct vio_dev *dev)
 
 	flush_work(&adapter->ibmvnic_reset);
 	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
+	flush_work(&adapter->ibmvnic_close_work);
 
 	rtnl_lock();
 	unregister_netdevice(netdev);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 480dc58707..d8c8a6d230 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -1045,6 +1045,7 @@ struct ibmvnic_adapter {
 
 	struct tasklet_struct tasklet;
 	enum vnic_state state;
+	bool removing;
 	/* Used for serialization of state field. When taking both state
 	 * and rwi locks, take state lock first.
 	 */
@@ -1056,6 +1057,7 @@ struct ibmvnic_adapter {
 	 */
 	spinlock_t rwi_lock;
 	struct work_struct ibmvnic_reset;
+	struct work_struct ibmvnic_close_work;
 	struct delayed_work ibmvnic_delayed_reset;
 	unsigned long resetting;
 	/* last device reset time */
-- 
2.34.1

Re: [PATCH net] net: ibmvnic: defer close from reset allocation failure

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 (4) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A close requested while the adapter is still probing now runs
  after probe has finished, and it can leave the device in a state that…
- [Medium] The new `removing` flag does not deliver what the commit
  message claims, namely that "ibmvnic_close() cannot make removal look…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  __ibmvnic_reset() takes adapter->rwi_lock with plain spin_lock() from…
- [Medium] This problem wasn't introduced by this patch, but
  ibmvnic_close_work() calls the ndo_stop implementation ibmvnic_close()…
- [Medium] This problem wasn't introduced by this patch, but
  ibmvnic_remove() never calls flush_reset_queue().
- [Medium] This problem wasn't introduced by this patch, but rtnl does
  not serialize the link-state transaction in ibmvnic_close() against…
- [Medium] This problem wasn't introduced by this patch, but
  ibmvnic_close() can run twice for one open, which unbalances the sub-…

--

net: ibmvnic: defer close from reset allocation failure

ibmvnic_reset() can run from the CRQ tasklet. This patch moves the
ibmvnic_close() call it makes when the rwi allocation fails into a new
work item. It also adds an adapter->removing flag that is checked when
reset work is scheduled, flushes the new work in probe and remove, and
switches probe_done to complete_all().

> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 5a510eed335ea..4da3fd7ce0012 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2152,6 +2152,26 @@ static int ibmvnic_close(struct net_device *netdev)
>  	return rc;
>  }
>  
> +static void ibmvnic_close_work(struct work_struct *work)
> +{

[ ... ]

> +	/* The close path can sleep, so run it outside the CRQ tasklet. */
> +	wait_for_completion(&adapter->probe_done);
> +	rtnl_lock();
> +	spin_lock_irqsave(&adapter->rwi_lock, flags);
> +	removing = adapter->removing;
> +	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> +
> +	if (!removing)
> +		ibmvnic_close(adapter->netdev);

[Severity: High]
Can this close run on an adapter that was never opened and leave it in
VNIC_CLOSED?

ibmvnic_reset() does not reject VNIC_PROBING. During probe, the CRQ
tasklet can reach it through handle_crq_init_rsp() queuing
VNIC_RESET_FAILOVER, or through an XPORT_EVENT queuing VNIC_RESET_FATAL.
If the GFP_ATOMIC rwi allocation fails there, the close work is queued
and blocks in wait_for_completion(&adapter->probe_done).

ibmvnic_probe() then writes the final state and runs the close:

ibmvnic_probe()
    adapter->state = VNIC_PROBED;   /* or VNIC_DOWN */
    register_netdev(netdev);
    complete_all(&adapter->probe_done);
    flush_work(&adapter->ibmvnic_close_work);
        ibmvnic_close_work()
            ibmvnic_close()
                __ibmvnic_close()
                    adapter->state = VNIC_CLOSED;

The failover_pending shortcut in ibmvnic_close() also sets VNIC_CLOSED.

Probe never calls init_resources(), so adapter->napi is still NULL. On
the next open, ibmvnic_open() treats VNIC_CLOSED as already set up:

    if (adapter->state != VNIC_CLOSED) {
        rc = ibmvnic_login(netdev);
        ...
        rc = init_resources(adapter);

Then __ibmvnic_open()->ibmvnic_napi_enable() does:

    for (i = 0; i < adapter->req_rx_queues; i++)
        napi_enable(&adapter->napi[i]);

Would that dereference the NULL adapter->napi array?

Before this patch, the inline close ran while the state was still
VNIC_PROBING, and probe overwrote it with VNIC_PROBED or VNIC_DOWN
afterwards. The new flush_work() in probe reverses that order.

If probe ended in VNIC_DOWN, the VNIC_CLOSED state also makes
handle_crq_init_rsp() queue VNIC_RESET_FAILOVER instead of
VNIC_RESET_PASSIVE_INIT.

Should the worker only close an adapter that is actually open (for
example state == VNIC_OPEN)? Or should close requests queued during
probing be dropped?

[Severity: Medium]
The commit message says the new flag ensures "ibmvnic_close() cannot make
removal look active again after remove() has begun". Does this snapshot
of adapter->removing guarantee that?

ibmvnic_remove() sets removing and VNIC_REMOVING without holding rtnl, so
it does not serialize against this worker:

CPU1 (__ibmvnic_reset)
  passes the !adapter->removing gate
  do_reset() waits in rtnl_lock()

CPU2 (ibmvnic_close_work)
  holds rtnl, sees removing == false
  __ibmvnic_close()->set_link_state() sleeps

CPU3 (ibmvnic_remove)
  adapter->removing = true;
  adapter->state = VNIC_REMOVING;
  flush_work(&adapter->ibmvnic_reset);

CPU2
  __ibmvnic_close()
      adapter->state = VNIC_CLOSED;
  rtnl_unlock();

CPU1
  do_reset() takes rtnl and re-checks only the state:
      if (reset_state == VNIC_REMOVING || reset_state == VNIC_REMOVED) {

Several checks on the in-progress reset path still look only at
adapter->state:

  - the early VNIC_REMOVING return in __ibmvnic_reset()
  - the per-rwi REMOVING/REMOVED check in its loop
  - the re-checks in do_reset() and do_hard_reset()

Would the reset then go ahead on a device that is being removed?

Failed resets are retried:

    if (!rwi && rc)
        rwi = tmprwi;

There is also a 60 second sleep after three failures. Can
ibmvnic_remove() then block in flush_work() indefinitely?

[Severity: Medium]
This is a pre-existing issue, but can this close run twice for one open
and leave the sub-CRQ IRQ disable depth unbalanced?

Each close calls disable_irq() on every rx/tx sub-CRQ IRQ in
ibmvnic_disable_irqs(). __ibmvnic_open() enables each queue only once:

    if (prev_state == VNIC_CLOSED)
        enable_irq(adapter->rx_scrq[i]->irq);

The deferred close can run when the adapter is already VNIC_CLOSED. For
example, ndo_stop runs while the worker waits on rtnl, or the ENOMEM hits
an interface that was already down. The depth then reaches 2, and the
next open leaves it at 1.

Would RX/TX completion interrupts then stay disabled while the driver
reports VNIC_OPEN?

The old inline ibmvnic_close() on ENOMEM had the same behavior. A state
check in the new worker would also cover this.

[Severity: Medium]
This isn't a bug introduced by this patch, but ibmvnic_close() is the
ndo_stop callback, and calling it directly bypasses dev_close().

IFF_UP and the qdisc state are not updated, so the core still sees the
interface as up. It will call ndo_stop again on a later link down or in
unregister_netdevice(). The close also runs whether or not the core ever
opened the device.

Is calling ndo_stop outside dev_close() intended here? The old code made
the same direct call from ibmvnic_reset(), and this patch only moves it
into a worker.

[Severity: Medium]
This is a pre-existing issue, but does the rtnl_lock() taken in this
worker serialize ibmvnic_close() against a reset? do_reset() drops rtnl
around its own link-state change:

    rtnl_unlock();
    rc = set_link_state(adapter, IBMVNIC_LOGICAL_LNK_DN);
    rtnl_lock();

This worker, or any ndo_stop, can take rtnl in that window and start a
second set_link_state(). Both callers do
reinit_completion(&adapter->init_done), wait on that same completion and
read the shared init_done_rc. The CRQ response handler uses a plain
complete().

Can one caller's reinit_completion() wipe out the response meant for the
other? One waiter would then time out after 20 seconds, or read the other
transaction's result.

The same race was already reachable through ndo_stop and through the old
inline close.

> +	rtnl_unlock();
> +}

[ ... ]

> @@ -3265,7 +3288,7 @@ static void __ibmvnic_reset(struct work_struct *work)
>  	 */
>  	need_reset = false;
>  	spin_lock(&adapter->rwi_lock);
> -	if (!list_empty(&adapter->rwi_list)) {
> +	if (!adapter->removing && !list_empty(&adapter->rwi_list)) {

[Severity: High]
This isn't a bug introduced by this patch, but since this critical
section is being modified: is a plain spin_lock() safe here?

__ibmvnic_reset() runs from a workqueue with BH enabled, but rwi_lock is
also taken in softirq context:

ibmvnic_interrupt()
  tasklet_schedule()
    ibmvnic_tasklet()
      ibmvnic_handle_crq()
        ibmvnic_reset()
          spin_lock_irqsave(&adapter->rwi_lock, flags);

ibmvnic_tx_timeout() also reaches ibmvnic_reset(), from the netdev
watchdog timer.

Suppose a CRQ interrupt arrives on the CPU running the reset worker while
the worker holds rwi_lock. The tasklet can run on irq exit and spin on
the lock held by the task it interrupted. Would that deadlock the CPU?

The probe timeout block added above uses spin_lock_irqsave() on the same
lock. Should this one use spin_lock_irqsave() or spin_lock_bh() as well?

[ ... ]

> @@ -6629,6 +6660,7 @@ static void ibmvnic_remove(struct vio_dev *dev)
>  
>  	flush_work(&adapter->ibmvnic_reset);
>  	flush_delayed_work(&adapter->ibmvnic_delayed_reset);
> +	flush_work(&adapter->ibmvnic_close_work);

[Severity: Medium]
This is a pre-existing issue, but does ibmvnic_remove() leak the
ibmvnic_rwi entries still on adapter->rwi_list?

ibmvnic_reset() can add an entry and queue the reset work just before
remove() sets VNIC_REMOVING. The flushed __ibmvnic_reset() then returns
early in one of three places:

  - the VNIC_REMOVING check
  - the new !adapter->removing gate
  - the in-loop REMOVING check, which frees only the current rwi

release_resources() does not touch rwi_list, and free_netdev() frees the
adapter.

The probe failure path calls flush_reset_queue() after its flushes.
Should ibmvnic_remove() do the same?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004103131.1205618-1-runyu.xiao%40seu.edu.cn