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

[mingming cao <mmc@linux.ibm.com>]

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@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 */

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