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

[Chris Leech <cleech@redhat.com>]

On Thu, Apr 07, 2022 at 07:13:09PM -0500, 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(+)
 
Reviewed-by: Chris Leech <cleech@redhat.com>

> 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;
>  
> -- 
> 2.25.1
>