netdev - Re: [PATCH net 1/1] ibmvnic: serialize access to work queue on remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1bd774566df5d634d58d49a064e5ca34@imap.linux.ibm.com>
Date:   Sat, 13 Feb 2021 08:17:05 -0800
From:   Dany Madden <drt@...ux.ibm.com>
To:     Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
Cc:     netdev@...r.kernel.org, Lijun Pan <ljp@...ux.ibm.com>,
        Rick Lindsley <ricklind@...ux.ibm.com>, uwe@...ine-koenig.org,
        saeed@...nel.org
Subject: Re: [PATCH net 1/1] ibmvnic: serialize access to work queue on remove

On 2021-02-12 20:42, Sukadev Bhattiprolu wrote:
> The work queue is used to queue reset requests like CHANGE-PARAM or
> FAILOVER resets for the worker thread. When the adapter is being 
> removed
> the adapter state is set to VNIC_REMOVING and the work queue is flushed
> so no new work is added. However the check for adapter being removed is
> racy in that the adapter can go into REMOVING state just after we check
> and we might end up adding work just as it is being flushed (or after).
> 
> The ->rwi_lock is already being used to serialize queue/dequeue work.
> Extend its usage ensure there is no race when scheduling/flushing work.
> 
> Fixes: 6954a9e4192b ("ibmvnic: Flush existing work items before device 
> removal")
> Signed-off-by: Sukadev Bhattiprolu <sukadev@...ux.ibm.com>

Reviewed-by: Dany Madden <drt@...ux.ibm.com>

> Cc:Uwe Kleine-König <uwe@...ine-koenig.org>
> Cc:Saeed Mahameed <saeed@...nel.org>
> ---
> Changelog
> 	An earlier version was reviewed by Saeed Mahmeed. But I have deferred
> 	some earlier patches in that set. Also, now extend the use of 
> ->rwi_lock
> 	rather than defining a new lock.
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 27 ++++++++++++++++++++-------
>  drivers/net/ethernet/ibm/ibmvnic.h |  5 ++++-
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index ce6b1cb0b0f9..004565b18a03 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2442,6 +2442,8 @@ static int ibmvnic_reset(struct ibmvnic_adapter 
> *adapter,
>  	unsigned long flags;
>  	int ret;
> 
> +	spin_lock_irqsave(&adapter->rwi_lock, flags);
> +
>  	/*
>  	 * If failover is pending don't schedule any other reset.
>  	 * Instead let the failover complete. If there is already a
> @@ -2462,14 +2464,11 @@ static int ibmvnic_reset(struct
> ibmvnic_adapter *adapter,
>  		goto err;
>  	}
> 
> -	spin_lock_irqsave(&adapter->rwi_lock, flags);
> -
>  	list_for_each(entry, &adapter->rwi_list) {
>  		tmp = list_entry(entry, struct ibmvnic_rwi, list);
>  		if (tmp->reset_reason == reason) {
>  			netdev_dbg(netdev, "Skipping matching reset, reason=%d\n",
>  				   reason);
> -			spin_unlock_irqrestore(&adapter->rwi_lock, flags);
>  			ret = EBUSY;
>  			goto err;
>  		}
> @@ -2477,8 +2476,6 @@ static int ibmvnic_reset(struct ibmvnic_adapter 
> *adapter,
> 
>  	rwi = kzalloc(sizeof(*rwi), GFP_ATOMIC);
>  	if (!rwi) {
> -		spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> -		ibmvnic_close(netdev);
>  		ret = ENOMEM;
>  		goto err;
>  	}
> @@ -2491,12 +2488,17 @@ static int ibmvnic_reset(struct
> ibmvnic_adapter *adapter,
>  	}
>  	rwi->reset_reason = reason;
>  	list_add_tail(&rwi->list, &adapter->rwi_list);
> -	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
>  	netdev_dbg(adapter->netdev, "Scheduling reset (reason %d)\n", 
> reason);
>  	schedule_work(&adapter->ibmvnic_reset);
> 
> -	return 0;
> +	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);
> +
>  	return -ret;
>  }
> 
> @@ -5512,7 +5514,18 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&adapter->state_lock, flags);
> +
> +	/* If ibmvnic_reset() is scheduling a reset, wait for it to
> +	 * finish. Then, set the state to REMOVING to prevent it from
> +	 * scheduling any more work and to have reset functions ignore
> +	 * any resets that have already been scheduled. Drop the lock
> +	 * after setting state, so __ibmvnic_reset() which is called
> +	 * from the flush_work() below, can make progress.
> +	 */
> +	spin_lock_irqsave(&adapter->rwi_lock, flags);
>  	adapter->state = VNIC_REMOVING;
> +	spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> +
>  	spin_unlock_irqrestore(&adapter->state_lock, flags);
> 
>  	flush_work(&adapter->ibmvnic_reset);
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.h
> b/drivers/net/ethernet/ibm/ibmvnic.h
> index c09c3f6bba9f..3cccbba70365 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.h
> +++ b/drivers/net/ethernet/ibm/ibmvnic.h
> @@ -1081,6 +1081,7 @@ struct ibmvnic_adapter {
>  	struct tasklet_struct tasklet;
>  	enum vnic_state state;
>  	enum ibmvnic_reset_reason reset_reason;
> +	/* when taking both state and rwi locks, take state lock first */
>  	spinlock_t rwi_lock;
>  	struct list_head rwi_list;
>  	struct work_struct ibmvnic_reset;
> @@ -1097,6 +1098,8 @@ struct ibmvnic_adapter {
>  	struct ibmvnic_tunables desired;
>  	struct ibmvnic_tunables fallback;
> 
> -	/* Used for serializatin of state field */
> +	/* Used for serialization of state field. When taking both state
> +	 * and rwi locks, take state lock first.
> +	 */
>  	spinlock_t state_lock;
>  };