scsi: iscsi: Fix unbound endpoint error handling

If a driver raises a connection error before the connection is bound, we
can leave a cleanup_work queued that can later run and disconnect/stop a
connection that is logged in. The problem is that drivers can call
iscsi_conn_error_event for endpoints that are connected but not yet bound
when something like the network port they are using is brought down.
iscsi_cleanup_conn_work_fn will check for this and exit early, but if the
cleanup_work is stuck behind other works, it might not get run until after
userspace has done ep_disconnect. Because the endpoint is not yet bound
there was no way for ep_disconnect to flush the work.

The bug of leaving stop_conns queued was added in:

Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")

and:

Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
kernel space")

was supposed to fix it, but left this case.

This patch moves the conn state check to before we even queue the work so
we can avoid queueing.

Link: https://lore.kernel.org/r/20220408001314.5014-7-michael.christie@oracle.com
Fixes: 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in kernel space")
Tested-by: Manish Rangankar <mrangankar@marvell.com>
Reviewed-by: Lee Duncan <lduncan@@suse.com>
Reviewed-by: Chris Leech <cleech@redhat.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 63a4f0c..2c0dd64 100644 (file)
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2201,10 +2201,10 @@ static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
 
        switch (flag) {
        case STOP_CONN_RECOVER:
-               conn->state = ISCSI_CONN_FAILED;
+               WRITE_ONCE(conn->state, ISCSI_CONN_FAILED);
                break;
        case STOP_CONN_TERM:
-               conn->state = ISCSI_CONN_DOWN;
+               WRITE_ONCE(conn->state, ISCSI_CONN_DOWN);
                break;
        default:
                iscsi_cls_conn_printk(KERN_ERR, conn, "invalid stop flag %d\n",
@@ -2222,7 +2222,7 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
        struct iscsi_endpoint *ep;
 
        ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
-       conn->state = ISCSI_CONN_FAILED;
+       WRITE_ONCE(conn->state, ISCSI_CONN_FAILED);
 
        if (!conn->ep || !session->transport->ep_disconnect)
                return;
@@ -2322,21 +2322,6 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
 
        mutex_lock(&conn->ep_mutex);
        /*
-        * If we are not at least bound there is nothing for us to do. Userspace
-        * will do a ep_disconnect call if offload is used, but will not be
-        * doing a stop since there is nothing to clean up, so we have to clear
-        * the cleanup bit here.
-        */
-       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;
-       }
-
-       /*
         * Get a ref to the ep, so we don't release its ID until after
         * userspace is done referencing it in iscsi_if_disconnect_bound_ep.
         */
@@ -2391,7 +2376,7 @@ iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
        INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
        conn->transport = transport;
        conn->cid = cid;
-       conn->state = ISCSI_CONN_DOWN;
+       WRITE_ONCE(conn->state, ISCSI_CONN_DOWN);
 
        /* this is released in the dev's release function */
        if (!get_device(&session->dev))
@@ -2590,10 +2575,30 @@ void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
        struct iscsi_internal *priv;
        int len = nlmsg_total_size(sizeof(*ev));
        unsigned long flags;
+       int state;
 
        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);
+       /*
+        * Userspace will only do a stop call if we are at least bound. And, we
+        * only need to do the in kernel cleanup if in the UP state so cmds can
+        * be released to upper layers. If in other states just wait for
+        * userspace to avoid races that can leave the cleanup_work queued.
+        */
+       state = READ_ONCE(conn->state);
+       switch (state) {
+       case ISCSI_CONN_BOUND:
+       case ISCSI_CONN_UP:
+               if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP,
+                                     &conn->flags)) {
+                       queue_work(iscsi_conn_cleanup_workq,
+                                  &conn->cleanup_work);
+               }
+               break;
+       default:
+               ISCSI_DBG_TRANS_CONN(conn, "Got conn error in state %d\n",
+                                    state);
+               break;
+       }
        spin_unlock_irqrestore(&conn->lock, flags);
 
        priv = iscsi_if_transport_lookup(conn->transport);
@@ -2944,7 +2949,7 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
        char *data = (char*)ev + sizeof(*ev);
        struct iscsi_cls_conn *conn;
        struct iscsi_cls_session *session;
-       int err = 0, value = 0;
+       int err = 0, value = 0, state;
 
        if (ev->u.set_param.len > PAGE_SIZE)
                return -EINVAL;
@@ -2961,8 +2966,8 @@ iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
                        session->recovery_tmo = value;
                break;
        default:
-               if ((conn->state == ISCSI_CONN_BOUND) ||
-                       (conn->state == ISCSI_CONN_UP)) {
+               state = READ_ONCE(conn->state);
+               if (state == ISCSI_CONN_BOUND || state == ISCSI_CONN_UP) {
                        err = transport->set_param(conn, ev->u.set_param.param,
                                        data, ev->u.set_param.len);
                } else {
@@ -3758,7 +3763,7 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
                                                ev->u.b_conn.transport_eph,
                                                ev->u.b_conn.is_leading);
                if (!ev->r.retcode)
-                       conn->state = ISCSI_CONN_BOUND;
+                       WRITE_ONCE(conn->state, ISCSI_CONN_BOUND);
 
                if (ev->r.retcode || !transport->ep_connect)
                        break;
@@ -3777,7 +3782,8 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
        case ISCSI_UEVENT_START_CONN:
                ev->r.retcode = transport->start_conn(conn);
                if (!ev->r.retcode)
-                       conn->state = ISCSI_CONN_UP;
+                       WRITE_ONCE(conn->state, ISCSI_CONN_UP);
+
                break;
        case ISCSI_UEVENT_SEND_PDU:
                pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
@@ -4084,10 +4090,11 @@ static ssize_t show_conn_state(struct device *dev,
 {
        struct iscsi_cls_conn *conn = iscsi_dev_to_conn(dev->parent);
        const char *state = "unknown";
+       int conn_state = READ_ONCE(conn->state);
 
-       if (conn->state >= 0 &&
-           conn->state < ARRAY_SIZE(connection_state_names))
-               state = connection_state_names[conn->state];
+       if (conn_state >= 0 &&
+           conn_state < ARRAY_SIZE(connection_state_names))
+               state = connection_state_names[conn_state];
 
        return sysfs_emit(buf, "%s\n", state);
 }