Re: [PATCH net] net: ibmvnic: defer close from reset allocation failure
From: mingming cao
Date: Fri Oct 16 2026 - 17:56:46 EST
On 10/4/26 3:31 AM, Runyu Xiao wrote:
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@xxxxxxxxxxxxxxx
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
---
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 */
Thanks for looking at this. The bug is real — deferring close out of
the CRQ tasklet is the right direction.
I walked ibmvnic_close_work() against the probe success path. Can a
probe-time ENOMEM leave the adapter in VNIC_CLOSED with no
init_resources()?
ibmvnic_reset() does not reject VNIC_PROBING, and the CRQ tasklet can
reach it during probe (e.g. CRQ_INIT -> VNIC_RESET_FAILOVER when state
is not VNIC_DOWN, or an XPORT_EVENT -> FATAL/MOBILITY). On GFP_ATOMIC
failure your worker waits on probe_done, then:
probe sets VNIC_PROBED / VNIC_DOWN, register_netdev()
-> complete_all() + flush_work(close_work)
-> ibmvnic_close() sets VNIC_CLOSED
next open sees state == VNIC_CLOSED, skips login/init_resources()
-> ibmvnic_napi_enable() uses adapter->napi[i] while napi is
still NULL (allocated only in init_napi() via init_resources())
Previously the inline close ran under VNIC_PROBING and probe overwrote
the final state afterwards. flush_work() after complete_all() reverses
that.
Should the worker only close when the iface was actually running
(netif_running() / state == VNIC_OPEN)?
While you are here: would dev_close() be better than calling
ibmvnic_close() directly? A bare ndo_stop leaves IFF_UP set when the
device was up (same as the old inline ENOMEM path). Keep rtnl_lock() in
the worker — dev_close() only takes the ops lock; __dev_close_many()
still expects rtnl. And netif_close() is a no-op when !(flags & IFF_UP),
which also avoids this post-probe VNIC_CLOSED case.
Minor on adapter->removing: the commit message is a bit stronger than
what a snapshot under rwi_lock can guarantee vs ibmvnic_remove(). Fine
to simplify in v2 if you prefer guarding VNIC_REMOVING in
__ibmvnic_close() instead of a new bool.
Happy to look at v2.
Thanks,
Mingming