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@xxxxxxxxxx>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 17 +++++++++++++++++
> include/scsi/scsi_transport_iscsi.h | 2 ++
> 2 files changed, 19 insertions(+)
Reviewed-by: Chris Leech <cleech@xxxxxxxxxx>
> 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
>
