Re: [PATCH 05/10] scsi: iscsi: Fix conn cleanup and stop race during iscsid restart

[Lee Duncan <lduncan@suse.com>]

On 4/7/22 17:13, Mike Christie wrote:
> If iscsid is doing a stop_conn at the same time the kernel is starting
> error recovery we can hit a race that allows the cleanup work to run on
> a valid connection. In the race, iscsi_if_stop_conn sees the cleanup bit
> set, but it calls flush_work on the clean_work before
> iscsi_conn_error_event has queued it. The flush then returns before the
> queueing and so the cleanup_work can run later and disconnect/stop a conn
> while it's in a connected state.
> 
> The patch:
> 
> Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> 
> added the late stop_conn call bug originally, and the patch:
> 
> Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> 
> attempted to fix it but only fixed the normal EH case and left the above
> race for the iscsid restart case. For the normal EH case we don't hit the
> race because we only signal userspace to start recovery after we have done
> the queueing, so the flush will always catch the queued work or see it
> completed.
> 
> For iscsid restart cases like boot, we can hit the race because iscsid
> will call down to the kernel before the kernel has signaled any error, so
> both code paths can be running at the same time. This adds a lock around
> the setting of the cleanup bit and queueing so they happen together.
> 
> Fixes: 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 17 +++++++++++++++++
>   include/scsi/scsi_transport_iscsi.h |  2 ++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index f200da049f3b..63a4f0c022fd 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2240,9 +2240,12 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>   					 bool is_active)
>   {
>   	/* Check if this was a conn error and the kernel took ownership */
> +	spin_lock_irq(&conn->lock);
>   	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> +		spin_unlock_irq(&conn->lock);
>   		iscsi_ep_disconnect(conn, is_active);
>   	} else {
> +		spin_unlock_irq(&conn->lock);
>   		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
>   		mutex_unlock(&conn->ep_mutex);
>   
> @@ -2289,9 +2292,12 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>   		/*
>   		 * Figure out if it was the kernel or userspace initiating this.
>   		 */
> +		spin_lock_irq(&conn->lock);
>   		if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> +			spin_unlock_irq(&conn->lock);
>   			iscsi_stop_conn(conn, flag);
>   		} else {
> +			spin_unlock_irq(&conn->lock);
>   			ISCSI_DBG_TRANS_CONN(conn,
>   					     "flush kernel conn cleanup.\n");
>   			flush_work(&conn->cleanup_work);
> @@ -2300,7 +2306,9 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>   		 * Only clear for recovery to avoid extra cleanup runs during
>   		 * termination.
>   		 */
> +		spin_lock_irq(&conn->lock);
>   		clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> +		spin_unlock_irq(&conn->lock);
>   	}
>   	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
>   	return 0;
> @@ -2321,7 +2329,9 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>   	 */
>   	if (conn->state != ISCSI_CONN_BOUND && conn->state != ISCSI_CONN_UP) {
>   		ISCSI_DBG_TRANS_CONN(conn, "Got error while conn is already failed. Ignoring.\n");
> +		spin_lock_irq(&conn->lock);
>   		clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> +		spin_unlock_irq(&conn->lock);
>   		mutex_unlock(&conn->ep_mutex);
>   		return;
>   	}
> @@ -2376,6 +2386,7 @@ iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
>   		conn->dd_data = &conn[1];
>   
>   	mutex_init(&conn->ep_mutex);
> +	spin_lock_init(&conn->lock);
>   	INIT_LIST_HEAD(&conn->conn_list);
>   	INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
>   	conn->transport = transport;
> @@ -2578,9 +2589,12 @@ void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
>   	struct iscsi_uevent *ev;
>   	struct iscsi_internal *priv;
>   	int len = nlmsg_total_size(sizeof(*ev));
> +	unsigned long flags;
>   
> +	spin_lock_irqsave(&conn->lock, flags);
>   	if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags))
>   		queue_work(iscsi_conn_cleanup_workq, &conn->cleanup_work);
> +	spin_unlock_irqrestore(&conn->lock, flags);
>   
>   	priv = iscsi_if_transport_lookup(conn->transport);
>   	if (!priv)
> @@ -3723,11 +3737,14 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
>   		return -EINVAL;
>   
>   	mutex_lock(&conn->ep_mutex);
> +	spin_lock_irq(&conn->lock);
>   	if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> +		spin_unlock_irq(&conn->lock);
>   		mutex_unlock(&conn->ep_mutex);
>   		ev->r.retcode = -ENOTCONN;
>   		return 0;
>   	}
> +	spin_unlock_irq(&conn->lock);
>   
>   	switch (nlh->nlmsg_type) {
>   	case ISCSI_UEVENT_BIND_CONN:
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index fdd486047404..9acb8422f680 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -211,6 +211,8 @@ struct iscsi_cls_conn {
>   	struct mutex ep_mutex;
>   	struct iscsi_endpoint *ep;
>   
> +	/* Used when accessing flags and queueing work. */
> +	spinlock_t lock;
>   	unsigned long flags;
>   	struct work_struct cleanup_work;
>   

Reviewed-by: Lee Duncan <lduncan@suse.com>