IB/iser: Protect iser state machine with a mutex

The iser connection state lookups and transitions are not fully protected.

Some transitions are protected with a spinlock, and in some cases the
state is accessed unprotected due to specific assumptions of the flow.

Introduce a new mutex to protect the connection state access. We use a
mutex since we need to also include a scheduling operations executed
under the state lock.

Each state transition/condition and its corresponding action will be
protected with the state mutex.

The rdma_cm events handler acquires the mutex when handling connection
events. Since iser connection state can transition to DOWN
concurrently during connection establishment, we bailout from
addr/route resolution events when the state is not PENDING.

This addresses a scenario where ep_poll retries expire during CMA
connection establishment. In this case ep_disconnect is invoked while
CMA events keep coming (address/route resolution, connected, etc...).

Signed-off-by: Ariel Nahum <arieln@mellanox.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>

3 files changed, 65 insertions, 14 deletions

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index d7acd4b62d6e..3dc853c9e4bf 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -632,10 +632,13 @@ iscsi_iser_ep_poll(struct iscsi_endpoint *ep, int timeout_ms)
                              msecs_to_jiffies(timeout_ms));
 
         /* if conn establishment failed, return error code to iscsi */
-        if (!rc &&
-            (ib_conn->state == ISER_CONN_TERMINATING ||
-             ib_conn->state == ISER_CONN_DOWN))
-                rc = -1;
+        if (rc == 0) {
+                mutex_lock(&ib_conn->state_mutex);
+                if (ib_conn->state == ISER_CONN_TERMINATING ||
+                    ib_conn->state == ISER_CONN_DOWN)
+                        rc = -1;
+                mutex_unlock(&ib_conn->state_mutex);
+        }
 
         iser_info("ib conn %p rc = %d\n", ib_conn, rc);
 
@@ -654,6 +657,7 @@ iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
 
         ib_conn = ep->dd_data;
         iser_info("ep %p ib conn %p state %d\n", ep, ib_conn, ib_conn->state);
+        mutex_lock(&ib_conn->state_mutex);
         iser_conn_terminate(ib_conn);
 
         /*
@@ -664,7 +668,10 @@ iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
         if (ib_conn->iscsi_conn) {
                 INIT_WORK(&ib_conn->release_work, iser_release_work);
                 queue_work(release_wq, &ib_conn->release_work);
+                mutex_unlock(&ib_conn->state_mutex);
         } else {
+                ib_conn->state = ISER_CONN_DOWN;
+                mutex_unlock(&ib_conn->state_mutex);
                 iser_conn_release(ib_conn);
         }
         iscsi_destroy_endpoint(ep);
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 37e928477b08..c7efc5a91604 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -335,6 +335,7 @@ struct iser_conn {
         char                         name[ISER_OBJECT_NAME_SIZE];
         struct work_struct           release_work;
         struct completion            stop_completion;
+        struct mutex                 state_mutex;
         struct list_head             conn_list;       /* entry in ig conn list */
 
         char                         *login_buf;
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index a5372c6cf9c6..6e7e54d883ab 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -565,16 +565,17 @@ static void iser_device_try_release(struct iser_device *device)
         mutex_unlock(&ig.device_list_mutex);
 }
 
+/**
+ * Called with state mutex held
+ **/
 static int iser_conn_state_comp_exch(struct iser_conn *ib_conn,
                                      enum iser_ib_conn_state comp,
                                      enum iser_ib_conn_state exch)
 {
         int ret;
 
-        spin_lock_bh(&ib_conn->lock);
         if ((ret = (ib_conn->state == comp)))
                 ib_conn->state = exch;
-        spin_unlock_bh(&ib_conn->lock);
         return ret;
 }
 
@@ -591,6 +592,10 @@ void iser_release_work(struct work_struct *work)
         wait_event_interruptible(ib_conn->wait,
                                  ib_conn->state == ISER_CONN_DOWN);
 
+        mutex_lock(&ib_conn->state_mutex);
+        ib_conn->state = ISER_CONN_DOWN;
+        mutex_unlock(&ib_conn->state_mutex);
+
         iser_conn_release(ib_conn);
 }
 
@@ -601,17 +606,21 @@ void iser_conn_release(struct iser_conn *ib_conn)
 {
         struct iser_device  *device = ib_conn->device;
 
-        BUG_ON(ib_conn->state == ISER_CONN_UP);
-
         mutex_lock(&ig.connlist_mutex);
         list_del(&ib_conn->conn_list);
         mutex_unlock(&ig.connlist_mutex);
+
+        mutex_lock(&ib_conn->state_mutex);
+        BUG_ON(ib_conn->state != ISER_CONN_DOWN);
+
         iser_free_rx_descriptors(ib_conn);
         iser_free_ib_conn_res(ib_conn);
         ib_conn->device = NULL;
         /* on EVENT_ADDR_ERROR there's no device yet for this conn */
         if (device != NULL)
                 iser_device_try_release(device);
+        mutex_unlock(&ib_conn->state_mutex);
+
         /* if cma handler context, the caller actually destroy the id */
         if (ib_conn->cma_id != NULL) {
                 rdma_destroy_id(ib_conn->cma_id);
@@ -639,6 +648,9 @@ void iser_conn_terminate(struct iser_conn *ib_conn)
                          ib_conn,err);
 }
 
+/**
+ * Called with state mutex held
+ **/
 static void iser_connect_error(struct rdma_cm_id *cma_id)
 {
         struct iser_conn *ib_conn;
@@ -649,12 +661,20 @@ static void iser_connect_error(struct rdma_cm_id *cma_id)
         wake_up_interruptible(&ib_conn->wait);
 }
 
+/**
+ * Called with state mutex held
+ **/
 static void iser_addr_handler(struct rdma_cm_id *cma_id)
 {
         struct iser_device *device;
         struct iser_conn   *ib_conn;
         int    ret;
 
+        ib_conn = (struct iser_conn *)cma_id->context;
+        if (ib_conn->state != ISER_CONN_PENDING)
+                /* bailout */
+                return;
+
         device = iser_device_find_by_ib_device(cma_id);
         if (!device) {
                 iser_err("device lookup/creation failed\n");
@@ -662,7 +682,6 @@ static void iser_addr_handler(struct rdma_cm_id *cma_id)
                 return;
         }
 
-        ib_conn = (struct iser_conn *)cma_id->context;
         ib_conn->device = device;
 
         /* connection T10-PI support */
@@ -686,6 +705,9 @@ static void iser_addr_handler(struct rdma_cm_id *cma_id)
         }
 }
 
+/**
+ * Called with state mutex held
+ **/
 static void iser_route_handler(struct rdma_cm_id *cma_id)
 {
         struct rdma_conn_param conn_param;
@@ -694,6 +716,10 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
         struct iser_conn *ib_conn = (struct iser_conn *)cma_id->context;
         struct iser_device *device = ib_conn->device;
 
+        if (ib_conn->state != ISER_CONN_PENDING)
+                /* bailout */
+                return;
+
         ret = iser_create_ib_conn_res((struct iser_conn *)cma_id->context);
         if (ret)
                 goto failure;
@@ -727,6 +753,11 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id)
         struct ib_qp_attr attr;
         struct ib_qp_init_attr init_attr;
 
+        ib_conn = (struct iser_conn *)cma_id->context;
+        if (ib_conn->state != ISER_CONN_PENDING)
+                /* bailout */
+                return;
+
         (void)ib_query_qp(cma_id->qp, &attr, ~0, &init_attr);
         iser_info("remote qpn:%x my qpn:%x\n", attr.dest_qp_num, cma_id->qp->qp_num);
 
@@ -761,9 +792,13 @@ static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
 
 static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 {
+        struct iser_conn *ib_conn;
+
+        ib_conn = (struct iser_conn *)cma_id->context;
         iser_info("event %d status %d conn %p id %p\n",
                   event->event, event->status, cma_id->context, cma_id);
 
+        mutex_lock(&ib_conn->state_mutex);
         switch (event->event) {
         case RDMA_CM_EVENT_ADDR_RESOLVED:
                 iser_addr_handler(cma_id);
@@ -791,6 +826,7 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
                 iser_err("Unexpected RDMA CM event (%d)\n", event->event);
                 break;
         }
+        mutex_unlock(&ib_conn->state_mutex);
         return 0;
 }
 
@@ -803,6 +839,7 @@ void iser_conn_init(struct iser_conn *ib_conn)
         init_completion(&ib_conn->stop_completion);
         INIT_LIST_HEAD(&ib_conn->conn_list);
         spin_lock_init(&ib_conn->lock);
+        mutex_init(&ib_conn->state_mutex);
 }
 
  /**
@@ -816,6 +853,8 @@ int iser_connect(struct iser_conn   *ib_conn,
 {
         int err = 0;
 
+        mutex_lock(&ib_conn->state_mutex);
+
         sprintf(ib_conn->name, "%pISp", dst_addr);
 
         iser_info("connecting to: %s\n", ib_conn->name);
@@ -849,6 +888,7 @@ int iser_connect(struct iser_conn   *ib_conn,
                         goto connect_failure;
                 }
         }
+        mutex_unlock(&ib_conn->state_mutex);
 
         mutex_lock(&ig.connlist_mutex);
         list_add(&ib_conn->conn_list, &ig.connlist);
@@ -860,6 +900,7 @@ id_failure:
 addr_failure:
         ib_conn->state = ISER_CONN_DOWN;
 connect_failure:
+        mutex_unlock(&ib_conn->state_mutex);
         iser_conn_release(ib_conn);
         return err;
 }
@@ -1044,11 +1085,13 @@ static void iser_handle_comp_error(struct iser_tx_desc *desc,
 
         if (ib_conn->post_recv_buf_count == 0 &&
             atomic_read(&ib_conn->post_send_buf_count) == 0) {
-                /* getting here when the state is UP means that the conn is *
-                 * being terminated asynchronously from the iSCSI layer's   *
-                 * perspective.                                             */
-                if (iser_conn_state_comp_exch(ib_conn, ISER_CONN_UP,
-                    ISER_CONN_TERMINATING))
+                /**
+                 * getting here when the state is UP means that the conn is
+                 * being terminated asynchronously from the iSCSI layer's
+                 * perspective. It is safe to peek at the connection state
+                 * since iscsi_conn_failure is allowed to be called twice.
+                 **/
+                if (ib_conn->state == ISER_CONN_UP)
                         iscsi_conn_failure(ib_conn->iscsi_conn,
                                            ISCSI_ERR_CONN_FAILED);