[PATCH 00/10] iscsi fixes

[PATCH 00/10] iscsi fixes

[Mike Christie]

The following patchset was made over Linus's tree and contains fixes for
boot/iscsid restart, more fixes for the in-kernel recovery patch, a data
corruption regression and fix for qedi connection error handling.

[PATCH 01/10] scsi: iscsi: Move iscsi_ep_disconnect

[Mike Christie]

This patch moves iscsi_ep_disconnect so it can be called earlier in the
next patch.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 38 ++++++++++++++---------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 27951ea05dd4..4e10457e3ab9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2217,6 +2217,25 @@ static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
 	ISCSI_DBG_TRANS_CONN(conn, "Stopping conn done.\n");
 }
 
+static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
+{
+	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
+	struct iscsi_endpoint *ep;
+
+	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
+	conn->state = ISCSI_CONN_FAILED;
+
+	if (!conn->ep || !session->transport->ep_disconnect)
+		return;
+
+	ep = conn->ep;
+	conn->ep = NULL;
+
+	session->transport->unbind_conn(conn, is_active);
+	session->transport->ep_disconnect(ep);
+	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
+}
+
 static int iscsi_if_stop_conn(struct iscsi_transport *transport,
 			      struct iscsi_uevent *ev)
 {
@@ -2257,25 +2276,6 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
 	return 0;
 }
 
-static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
-{
-	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
-	struct iscsi_endpoint *ep;
-
-	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
-	conn->state = ISCSI_CONN_FAILED;
-
-	if (!conn->ep || !session->transport->ep_disconnect)
-		return;
-
-	ep = conn->ep;
-	conn->ep = NULL;
-
-	session->transport->unbind_conn(conn, is_active);
-	session->transport->ep_disconnect(ep);
-	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
-}
-
 static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
 {
 	struct iscsi_cls_conn *conn = container_of(work, struct iscsi_cls_conn,
-- 
2.25.1

[PATCH 02/10] scsi: iscsi: Fix offload conn cleanup when iscsid restarts

[Mike Christie]

When userspace restarts during boot or upgrades it won't know about the
offload driver's endpoint and connection mappings. iscsid will start by
cleaning up the old session by doing a stop_conn call. Later if we are
able to create a new connection, we cleanup the old endpoint during the
binding stage. The problem is that if we do stop_conn before doing the
ep_disconnect call offload drivers can still be executing IO. We then
might free tasks from the under the card/driver.

This moves the ep_disconnect call to before we do the stop_conn call for
this case. It will then work and look like a normal recovery/cleanup
procedure from the driver's point of view.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 48 +++++++++++++++++------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 4e10457e3ab9..bf39fb5569b6 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2236,6 +2236,23 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
 	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
 }
 
+static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
+					 struct iscsi_endpoint *ep,
+					 bool is_active)
+{
+	/* Check if this was a conn error and the kernel took ownership */
+	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
+		iscsi_ep_disconnect(conn, is_active);
+	} else {
+		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
+		mutex_unlock(&conn->ep_mutex);
+
+		flush_work(&conn->cleanup_work);
+
+		mutex_lock(&conn->ep_mutex);
+	}
+}
+
 static int iscsi_if_stop_conn(struct iscsi_transport *transport,
 			      struct iscsi_uevent *ev)
 {
@@ -2256,6 +2273,16 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
 		cancel_work_sync(&conn->cleanup_work);
 		iscsi_stop_conn(conn, flag);
 	} else {
+		/*
+		 * For offload, when iscsid is restarted it won't know about
+		 * existing endpoints so it can't do a ep_disconnect. We clean
+		 * it up here for userspace.
+		 */
+		mutex_lock(&conn->ep_mutex);
+		if (conn->ep)
+			iscsi_if_disconnect_bound_ep(conn, conn->ep, true);
+		mutex_unlock(&conn->ep_mutex);
+
 		/*
 		 * Figure out if it was the kernel or userspace initiating this.
 		 */
@@ -2984,16 +3011,7 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
 	}
 
 	mutex_lock(&conn->ep_mutex);
-	/* Check if this was a conn error and the kernel took ownership */
-	if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
-		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
-		mutex_unlock(&conn->ep_mutex);
-
-		flush_work(&conn->cleanup_work);
-		goto put_ep;
-	}
-
-	iscsi_ep_disconnect(conn, false);
+	iscsi_if_disconnect_bound_ep(conn, ep, false);
 	mutex_unlock(&conn->ep_mutex);
 put_ep:
 	iscsi_put_endpoint(ep);
@@ -3704,16 +3722,6 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
 
 	switch (nlh->nlmsg_type) {
 	case ISCSI_UEVENT_BIND_CONN:
-		if (conn->ep) {
-			/*
-			 * For offload boot support where iscsid is restarted
-			 * during the pivot root stage, the ep will be intact
-			 * here when the new iscsid instance starts up and
-			 * reconnects.
-			 */
-			iscsi_ep_disconnect(conn, true);
-		}
-
 		session = iscsi_session_lookup(ev->u.b_conn.sid);
 		if (!session) {
 			err = -EINVAL;
-- 
2.25.1

[PATCH 03/10] scsi: iscsi: Release endpoint ID when its freed.

[Mike Christie]

We can't release the endpoint ID until all references to the endpoint have
been dropped or it could be allocated while in use. This has us use an idr
instead of looping over all conns to find a free ID and then free the ID
when all references have been dropped instead of when the device is only
deleted.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 71 ++++++++++++++---------------
 include/scsi/scsi_transport_iscsi.h |  2 +-
 2 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index bf39fb5569b6..1fc7c6bfbd67 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -86,6 +86,9 @@ struct iscsi_internal {
 	struct transport_container session_cont;
 };
 
+static DEFINE_IDR(iscsi_ep_idr);
+static DEFINE_MUTEX(iscsi_ep_idr_mutex);
+
 static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
 
 static struct workqueue_struct *iscsi_conn_cleanup_workq;
@@ -168,6 +171,11 @@ struct device_attribute dev_attr_##_prefix##_##_name =	\
 static void iscsi_endpoint_release(struct device *dev)
 {
 	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
+
+	mutex_lock(&iscsi_ep_idr_mutex);
+	idr_remove(&iscsi_ep_idr, ep->id);
+	mutex_unlock(&iscsi_ep_idr_mutex);
+
 	kfree(ep);
 }
 
@@ -180,7 +188,7 @@ static ssize_t
 show_ep_handle(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
-	return sysfs_emit(buf, "%llu\n", (unsigned long long) ep->id);
+	return sysfs_emit(buf, "%d\n", ep->id);
 }
 static ISCSI_ATTR(ep, handle, S_IRUGO, show_ep_handle, NULL);
 
@@ -193,48 +201,32 @@ static struct attribute_group iscsi_endpoint_group = {
 	.attrs = iscsi_endpoint_attrs,
 };
 
-#define ISCSI_MAX_EPID -1
-
-static int iscsi_match_epid(struct device *dev, const void *data)
-{
-	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
-	const uint64_t *epid = data;
-
-	return *epid == ep->id;
-}
-
 struct iscsi_endpoint *
 iscsi_create_endpoint(int dd_size)
 {
-	struct device *dev;
 	struct iscsi_endpoint *ep;
-	uint64_t id;
-	int err;
-
-	for (id = 1; id < ISCSI_MAX_EPID; id++) {
-		dev = class_find_device(&iscsi_endpoint_class, NULL, &id,
-					iscsi_match_epid);
-		if (!dev)
-			break;
-		else
-			put_device(dev);
-	}
-	if (id == ISCSI_MAX_EPID) {
-		printk(KERN_ERR "Too many connections. Max supported %u\n",
-		       ISCSI_MAX_EPID - 1);
-		return NULL;
-	}
+	int err, id;
 
 	ep = kzalloc(sizeof(*ep) + dd_size, GFP_KERNEL);
 	if (!ep)
 		return NULL;
 
+	mutex_lock(&iscsi_ep_idr_mutex);
+	id = idr_alloc(&iscsi_ep_idr, ep, 0, -1, GFP_NOIO);
+	if (id < 0) {
+		mutex_unlock(&iscsi_ep_idr_mutex);
+		printk(KERN_ERR "Could not allocate endpoint ID. Error %d.\n",
+		       id);
+		goto free_ep;
+	}
+	mutex_unlock(&iscsi_ep_idr_mutex);
+
 	ep->id = id;
 	ep->dev.class = &iscsi_endpoint_class;
-	dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id);
+	dev_set_name(&ep->dev, "ep-%d", id);
 	err = device_register(&ep->dev);
         if (err)
-                goto free_ep;
+		goto free_id;
 
 	err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group);
 	if (err)
@@ -248,6 +240,10 @@ iscsi_create_endpoint(int dd_size)
 	device_unregister(&ep->dev);
 	return NULL;
 
+free_id:
+	mutex_lock(&iscsi_ep_idr_mutex);
+	idr_remove(&iscsi_ep_idr, id);
+	mutex_unlock(&iscsi_ep_idr_mutex);
 free_ep:
 	kfree(ep);
 	return NULL;
@@ -275,14 +271,17 @@ EXPORT_SYMBOL_GPL(iscsi_put_endpoint);
  */
 struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)
 {
-	struct device *dev;
+	struct iscsi_endpoint *ep;
 
-	dev = class_find_device(&iscsi_endpoint_class, NULL, &handle,
-				iscsi_match_epid);
-	if (!dev)
-		return NULL;
+	mutex_lock(&iscsi_ep_idr_mutex);
+	ep = idr_find(&iscsi_ep_idr, handle);
+	if (!ep)
+		goto unlock;
 
-	return iscsi_dev_to_endpoint(dev);
+	get_device(&ep->dev);
+unlock:
+	mutex_unlock(&iscsi_ep_idr_mutex);
+	return ep;
 }
 EXPORT_SYMBOL_GPL(iscsi_lookup_endpoint);
 
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 38e4a67f5922..fdd486047404 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -295,7 +295,7 @@ extern void iscsi_host_for_each_session(struct Scsi_Host *shost,
 struct iscsi_endpoint {
 	void *dd_data;			/* LLD private data */
 	struct device dev;
-	uint64_t id;
+	int id;
 	struct iscsi_cls_conn *conn;
 };
 
-- 
2.25.1

[PATCH 04/10] scsi: iscsi: Fix endpoint reuse regression

[Mike Christie]

This patch fixes a bug where when using iscsi offload we can free a
endpoint while userspace still thinks it's active. That then causes the
endpoint ID to be reused for a new connection's endpoint while userspace
still thinks the ID is for the original connection. Userspace will then
end up disconnecting a running connection's endpoint or trying to bind
to another connection's endpoint.

This bug is a regression added in:

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

where we added a in kernel ep_disconnect call to fix a bug in:

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

where we would call stop_conn without having done ep_disconnect. This
early ep_disconnect call will then free the endpoint and it's ID while
userspace still thinks the ID is valid.

This patch fixes the early release of the ID by having the in kernel
recovery code keep a reference to the endpoint until userspace has called
into the kernel to finish cleaning up the endpoint/connection. It requires
the previous patch "scsi: iscsi: Release endpoint ID when its freed."
which moved the freeing of the ID until when the endpoint is released.

Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 1fc7c6bfbd67..f200da049f3b 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2247,7 +2247,11 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
 		mutex_unlock(&conn->ep_mutex);
 
 		flush_work(&conn->cleanup_work);
-
+		/*
+		 * Userspace is now done with the EP so we can release the ref
+		 * iscsi_cleanup_conn_work_fn took.
+		 */
+		iscsi_put_endpoint(ep);
 		mutex_lock(&conn->ep_mutex);
 	}
 }
@@ -2322,6 +2326,12 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
 		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.
+	 */
+	if (conn->ep)
+		get_device(&conn->ep->dev);
 	iscsi_ep_disconnect(conn, false);
 
 	if (system_state != SYSTEM_RUNNING) {
-- 
2.25.1

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

[Mike Christie]

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

[PATCH 06/10] scsi: iscsi: Fix unbound endpoint error handling

[Mike Christie]

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.

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 | 65 ++++++++++++++++-------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 63a4f0c022fd..2c0dd64159b0 100644
--- 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;
@@ -2321,21 +2321,6 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
 	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
 
 	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);
 }
-- 
2.25.1

[PATCH 07/10] scsi: iscsi: Merge suspend fields

[Mike Christie]

Move the tx and rx suspend fields into one flags field.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/bnx2i/bnx2i_hwi.c   |  2 +-
 drivers/scsi/bnx2i/bnx2i_iscsi.c |  2 +-
 drivers/scsi/cxgbi/libcxgbi.c    |  6 +++---
 drivers/scsi/libiscsi.c          | 20 ++++++++++----------
 drivers/scsi/libiscsi_tcp.c      |  2 +-
 include/scsi/libiscsi.h          |  9 +++++----
 6 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 5521469ce678..e16327a4b4c9 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -1977,7 +1977,7 @@ static int bnx2i_process_new_cqes(struct bnx2i_conn *bnx2i_conn)
 		if (nopin->cq_req_sn != qp->cqe_exp_seq_sn)
 			break;
 
-		if (unlikely(test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx))) {
+		if (unlikely(test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
 			if (nopin->op_code == ISCSI_OP_NOOP_IN &&
 			    nopin->itt == (u16) RESERVED_ITT) {
 				printk(KERN_ALERT "bnx2i: Unsolicited "
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index fe86fd61a995..15fbd09baa94 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1721,7 +1721,7 @@ static int bnx2i_tear_down_conn(struct bnx2i_hba *hba,
 			struct iscsi_conn *conn = ep->conn->cls_conn->dd_data;
 
 			/* Must suspend all rx queue activity for this ep */
-			set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
+			set_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
 		}
 		/* CONN_DISCONNECT timeout may or may not be an issue depending
 		 * on what transcribed in TCP layer, different targets behave
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 8c7d4dda4cf2..4365d52c6430 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -1634,11 +1634,11 @@ void cxgbi_conn_pdu_ready(struct cxgbi_sock *csk)
 	log_debug(1 << CXGBI_DBG_PDU_RX,
 		"csk 0x%p, conn 0x%p.\n", csk, conn);
 
-	if (unlikely(!conn || conn->suspend_rx)) {
+	if (unlikely(!conn || test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
 		log_debug(1 << CXGBI_DBG_PDU_RX,
-			"csk 0x%p, conn 0x%p, id %d, suspend_rx %lu!\n",
+			"csk 0x%p, conn 0x%p, id %d, conn flags 0x%lx!\n",
 			csk, conn, conn ? conn->id : 0xFF,
-			conn ? conn->suspend_rx : 0xFF);
+			conn ? conn->flags : 0xFF);
 		return;
 	}
 
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d09926e6c8a8..5e7bd5a3b430 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1392,8 +1392,8 @@ static bool iscsi_set_conn_failed(struct iscsi_conn *conn)
 	if (conn->stop_stage == 0)
 		session->state = ISCSI_STATE_FAILED;
 
-	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
-	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
+	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
+	set_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
 	return true;
 }
 
@@ -1454,7 +1454,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
 	 * Do this after dropping the extra ref because if this was a requeue
 	 * it's removed from that list and cleanup_queued_task would miss it.
 	 */
-	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
+	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
 		/*
 		 * Save the task and ref in case we weren't cleaning up this
 		 * task and get woken up again.
@@ -1532,7 +1532,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
 	int rc = 0;
 
 	spin_lock_bh(&conn->session->frwd_lock);
-	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
+	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
 		ISCSI_DBG_SESSION(conn->session, "Tx suspended!\n");
 		spin_unlock_bh(&conn->session->frwd_lock);
 		return -ENODATA;
@@ -1746,7 +1746,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
 		goto fault;
 	}
 
-	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
+	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
 		reason = FAILURE_SESSION_IN_RECOVERY;
 		sc->result = DID_REQUEUE << 16;
 		goto fault;
@@ -1935,7 +1935,7 @@ static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
 void iscsi_suspend_queue(struct iscsi_conn *conn)
 {
 	spin_lock_bh(&conn->session->frwd_lock);
-	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
+	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
 	spin_unlock_bh(&conn->session->frwd_lock);
 }
 EXPORT_SYMBOL_GPL(iscsi_suspend_queue);
@@ -1953,7 +1953,7 @@ void iscsi_suspend_tx(struct iscsi_conn *conn)
 	struct Scsi_Host *shost = conn->session->host;
 	struct iscsi_host *ihost = shost_priv(shost);
 
-	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
+	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
 	if (ihost->workq)
 		flush_workqueue(ihost->workq);
 }
@@ -1961,7 +1961,7 @@ EXPORT_SYMBOL_GPL(iscsi_suspend_tx);
 
 static void iscsi_start_tx(struct iscsi_conn *conn)
 {
-	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
+	clear_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
 	iscsi_conn_queue_work(conn);
 }
 
@@ -3330,8 +3330,8 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
 	/*
 	 * Unblock xmitworker(), Login Phase will pass through.
 	 */
-	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
-	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
+	clear_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
+	clear_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_bind);
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 2e9ffe3d1a55..883005757ddb 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -927,7 +927,7 @@ int iscsi_tcp_recv_skb(struct iscsi_conn *conn, struct sk_buff *skb,
 	 */
 	conn->last_recv = jiffies;
 
-	if (unlikely(conn->suspend_rx)) {
+	if (unlikely(test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
 		ISCSI_DBG_TCP(conn, "Rx suspended!\n");
 		*status = ISCSI_TCP_SUSPENDED;
 		return 0;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index e76c94697c1b..84086c240228 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -53,8 +53,10 @@ enum {
 
 #define ISID_SIZE			6
 
-/* Connection suspend "bit" */
-#define ISCSI_SUSPEND_BIT		1
+/* Connection flags */
+#define ISCSI_CONN_FLAG_SUSPEND_TX	BIT(0)
+#define ISCSI_CONN_FLAG_SUSPEND_RX	BIT(1)
+
 
 #define ISCSI_ITT_MASK			0x1fff
 #define ISCSI_TOTAL_CMDS_MAX		4096
@@ -211,8 +213,7 @@ struct iscsi_conn {
 	struct list_head	cmdqueue;	/* data-path cmd queue */
 	struct list_head	requeue;	/* tasks needing another run */
 	struct work_struct	xmitwork;	/* per-conn. xmit workqueue */
-	unsigned long		suspend_tx;	/* suspend Tx */
-	unsigned long		suspend_rx;	/* suspend Rx */
+	unsigned long		flags;		/* ISCSI_CONN_FLAGs */
 
 	/* negotiated params */
 	unsigned		max_recv_dlength; /* initiator_max_recv_dsl*/
-- 
2.25.1

[PATCH 08/10] scsi: iscsi: Fix nop handling during conn recovery

[Mike Christie]

If a offload driver doesn't use the xmit workqueue, then when we are
doing ep_disconnect libiscsi can still inject PDUs to the driver. This
adds a check for if the connection is bound before trying to inject PDUs.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/libiscsi.c | 7 ++++++-
 include/scsi/libiscsi.h | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 5e7bd5a3b430..0bf8cf8585bb 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -678,7 +678,8 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
 	struct iscsi_task *task;
 	itt_t itt;
 
-	if (session->state == ISCSI_STATE_TERMINATE)
+	if (session->state == ISCSI_STATE_TERMINATE ||
+	    !test_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags))
 		return NULL;
 
 	if (opcode == ISCSI_OP_LOGIN || opcode == ISCSI_OP_TEXT) {
@@ -2214,6 +2215,8 @@ void iscsi_conn_unbind(struct iscsi_cls_conn *cls_conn, bool is_active)
 	iscsi_suspend_tx(conn);
 
 	spin_lock_bh(&session->frwd_lock);
+	clear_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags);
+
 	if (!is_active) {
 		/*
 		 * if logout timed out before userspace could even send a PDU
@@ -3318,6 +3321,8 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
 	spin_lock_bh(&session->frwd_lock);
 	if (is_leading)
 		session->leadconn = conn;
+
+	set_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags);
 	spin_unlock_bh(&session->frwd_lock);
 
 	/*
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 84086c240228..d0a24779c52d 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -56,7 +56,7 @@ enum {
 /* Connection flags */
 #define ISCSI_CONN_FLAG_SUSPEND_TX	BIT(0)
 #define ISCSI_CONN_FLAG_SUSPEND_RX	BIT(1)
-
+#define ISCSI_CONN_FLAG_BOUND		BIT(2)
 
 #define ISCSI_ITT_MASK			0x1fff
 #define ISCSI_TOTAL_CMDS_MAX		4096
-- 
2.25.1

[PATCH 09/10] scsi: qedi: Fix failed disconnect handling.

[Mike Christie]

We set the qedi_ep state to EP_STATE_OFLDCONN_START when the ep is
created. Then in qedi_set_path we kick off the offload work. If userspace
times out the connection and calls ep_disconnect, qedi will only flush the
offload work if the qedi_ep state has transitioned away from
EP_STATE_OFLDCONN_START. If we can't connect we will not have transitioned
state and will leave the offload work running, and we will free the
qedi_ep from under it.

This patch just has us init the work when we create the ep, then always
flush it.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/qedi/qedi_iscsi.c | 69 +++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 8196f89f404e..31ec429104e2 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -860,6 +860,37 @@ static int qedi_task_xmit(struct iscsi_task *task)
 	return qedi_iscsi_send_ioreq(task);
 }
 
+static void qedi_offload_work(struct work_struct *work)
+{
+	struct qedi_endpoint *qedi_ep =
+		container_of(work, struct qedi_endpoint, offload_work);
+	struct qedi_ctx *qedi;
+	int wait_delay = 5 * HZ;
+	int ret;
+
+	qedi = qedi_ep->qedi;
+
+	ret = qedi_iscsi_offload_conn(qedi_ep);
+	if (ret) {
+		QEDI_ERR(&qedi->dbg_ctx,
+			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
+			 qedi_ep->iscsi_cid, qedi_ep, ret);
+		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
+		return;
+	}
+
+	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
+					       (qedi_ep->state ==
+					       EP_STATE_OFLDCONN_COMPL),
+					       wait_delay);
+	if (ret <= 0 || qedi_ep->state != EP_STATE_OFLDCONN_COMPL) {
+		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
+		QEDI_ERR(&qedi->dbg_ctx,
+			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
+			 qedi_ep->iscsi_cid, qedi_ep);
+	}
+}
+
 static struct iscsi_endpoint *
 qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
 		int non_blocking)
@@ -908,6 +939,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
 	}
 	qedi_ep = ep->dd_data;
 	memset(qedi_ep, 0, sizeof(struct qedi_endpoint));
+	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
 	qedi_ep->state = EP_STATE_IDLE;
 	qedi_ep->iscsi_cid = (u32)-1;
 	qedi_ep->qedi = qedi;
@@ -1056,12 +1088,11 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
 	qedi_ep = ep->dd_data;
 	qedi = qedi_ep->qedi;
 
+	flush_work(&qedi_ep->offload_work);
+
 	if (qedi_ep->state == EP_STATE_OFLDCONN_START)
 		goto ep_exit_recover;
 
-	if (qedi_ep->state != EP_STATE_OFLDCONN_NONE)
-		flush_work(&qedi_ep->offload_work);
-
 	if (qedi_ep->conn) {
 		qedi_conn = qedi_ep->conn;
 		abrt_conn = qedi_conn->abrt_conn;
@@ -1235,37 +1266,6 @@ static int qedi_data_avail(struct qedi_ctx *qedi, u16 vlanid)
 	return rc;
 }
 
-static void qedi_offload_work(struct work_struct *work)
-{
-	struct qedi_endpoint *qedi_ep =
-		container_of(work, struct qedi_endpoint, offload_work);
-	struct qedi_ctx *qedi;
-	int wait_delay = 5 * HZ;
-	int ret;
-
-	qedi = qedi_ep->qedi;
-
-	ret = qedi_iscsi_offload_conn(qedi_ep);
-	if (ret) {
-		QEDI_ERR(&qedi->dbg_ctx,
-			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
-			 qedi_ep->iscsi_cid, qedi_ep, ret);
-		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
-		return;
-	}
-
-	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
-					       (qedi_ep->state ==
-					       EP_STATE_OFLDCONN_COMPL),
-					       wait_delay);
-	if ((ret <= 0) || (qedi_ep->state != EP_STATE_OFLDCONN_COMPL)) {
-		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
-		QEDI_ERR(&qedi->dbg_ctx,
-			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
-			 qedi_ep->iscsi_cid, qedi_ep);
-	}
-}
-
 static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
 {
 	struct qedi_ctx *qedi;
@@ -1381,7 +1381,6 @@ static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
 			  qedi_ep->dst_addr, qedi_ep->dst_port);
 	}
 
-	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
 	queue_work(qedi->offload_thread, &qedi_ep->offload_work);
 
 	ret = 0;
-- 
2.25.1

[PATCH 10/10] scsi: iscsi: Add Mike Christie as co-maintainer

[Mike Christie]

I've been doing a lot of iscsi patches because Oracle is paying me to work
on iSCSI again. It was supposed to be temp assignment, but my co-worker
that was working on iscsi moved to a new group so it looks like I'm back
on this code again. After talking to Chris and Lee this patch adds me back
as co-maintainer, so I can help them and people remember to cc me on
issues.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fd768d43e048..ca9d56121974 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10369,6 +10369,7 @@ F:	include/linux/isapnp.h
 ISCSI
 M:	Lee Duncan <lduncan@suse.com>
 M:	Chris Leech <cleech@redhat.com>
+M:	Mike Christie <michael.christie@oracle.com>
 L:	open-iscsi@googlegroups.com
 L:	linux-scsi@vger.kernel.org
 S:	Maintained
-- 
2.25.1

RE: [EXT] [PATCH 00/10] iscsi fixes

[Manish Rangankar]

> -----Original Message-----
> From: Mike Christie <michael.christie@oracle.com>
> Sent: Friday, April 8, 2022 5:43 AM
> To: Saurav Kashyap <skashyap@marvell.com>; lduncan@suse.com;
> cleech@redhat.com; Nilesh Javali <njavali@marvell.com>; Manish Rangankar
> <mrangankar@marvell.com>; GR-QLogic-Storage-Upstream <GR-QLogic-
> Storage-Upstream@marvell.com>; martin.petersen@oracle.com; linux-
> scsi@vger.kernel.org; jejb@linux.ibm.com
> Subject: [EXT] [PATCH 00/10] iscsi fixes
> 
> External Email
> 
> ----------------------------------------------------------------------
> The following patchset was made over Linus's tree and contains fixes for
> boot/iscsid restart, more fixes for the in-kernel recovery patch, a data
> corruption regression and fix for qedi connection error handling.
> 
> 
Validated with SW iSCSI and offload iSCSI (bnx2i and qedi) and both the regressions are seen fixed.

Thanks,
Tested-by: Manish Rangankar <mrangankar@marvell.com>
Tested-by: Nilesh Javali <njavali@marvell.com>

RE: [EXT] [PATCH 09/10] scsi: qedi: Fix failed disconnect handling.

[Manish Rangankar]

> -----Original Message-----
> From: Mike Christie <michael.christie@oracle.com>
> Sent: Friday, April 8, 2022 5:43 AM
> To: Saurav Kashyap <skashyap@marvell.com>; lduncan@suse.com;
> cleech@redhat.com; Nilesh Javali <njavali@marvell.com>; Manish Rangankar
> <mrangankar@marvell.com>; GR-QLogic-Storage-Upstream <GR-QLogic-
> Storage-Upstream@marvell.com>; martin.petersen@oracle.com; linux-
> scsi@vger.kernel.org; jejb@linux.ibm.com
> Cc: Mike Christie <michael.christie@oracle.com>
> Subject: [EXT] [PATCH 09/10] scsi: qedi: Fix failed disconnect handling.
> 
> External Email
> 
> ----------------------------------------------------------------------
> We set the qedi_ep state to EP_STATE_OFLDCONN_START when the ep is
> created. Then in qedi_set_path we kick off the offload work. If userspace times
> out the connection and calls ep_disconnect, qedi will only flush the offload work
> if the qedi_ep state has transitioned away from EP_STATE_OFLDCONN_START.
> If we can't connect we will not have transitioned state and will leave the offload
> work running, and we will free the qedi_ep from under it.
> 
> This patch just has us init the work when we create the ep, then always flush it.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/qedi/qedi_iscsi.c | 69 +++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c index
> 8196f89f404e..31ec429104e2 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.c
> +++ b/drivers/scsi/qedi/qedi_iscsi.c
> @@ -860,6 +860,37 @@ static int qedi_task_xmit(struct iscsi_task *task)
>  	return qedi_iscsi_send_ioreq(task);
>  }
> 
> +static void qedi_offload_work(struct work_struct *work) {
> +	struct qedi_endpoint *qedi_ep =
> +		container_of(work, struct qedi_endpoint, offload_work);
> +	struct qedi_ctx *qedi;
> +	int wait_delay = 5 * HZ;
> +	int ret;
> +
> +	qedi = qedi_ep->qedi;
> +
> +	ret = qedi_iscsi_offload_conn(qedi_ep);
> +	if (ret) {
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> +			 qedi_ep->iscsi_cid, qedi_ep, ret);
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		return;
> +	}
> +
> +	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> +					       (qedi_ep->state ==
> +					       EP_STATE_OFLDCONN_COMPL),
> +					       wait_delay);
> +	if (ret <= 0 || qedi_ep->state != EP_STATE_OFLDCONN_COMPL) {
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> +			 qedi_ep->iscsi_cid, qedi_ep);
> +	}
> +}
> +
>  static struct iscsi_endpoint *
>  qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
>  		int non_blocking)
> @@ -908,6 +939,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct
> sockaddr *dst_addr,
>  	}
>  	qedi_ep = ep->dd_data;
>  	memset(qedi_ep, 0, sizeof(struct qedi_endpoint));
> +	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>  	qedi_ep->state = EP_STATE_IDLE;
>  	qedi_ep->iscsi_cid = (u32)-1;
>  	qedi_ep->qedi = qedi;
> @@ -1056,12 +1088,11 @@ static void qedi_ep_disconnect(struct
> iscsi_endpoint *ep)
>  	qedi_ep = ep->dd_data;
>  	qedi = qedi_ep->qedi;
> 
> +	flush_work(&qedi_ep->offload_work);
> +
>  	if (qedi_ep->state == EP_STATE_OFLDCONN_START)
>  		goto ep_exit_recover;
> 
> -	if (qedi_ep->state != EP_STATE_OFLDCONN_NONE)
> -		flush_work(&qedi_ep->offload_work);
> -
>  	if (qedi_ep->conn) {
>  		qedi_conn = qedi_ep->conn;
>  		abrt_conn = qedi_conn->abrt_conn;
> @@ -1235,37 +1266,6 @@ static int qedi_data_avail(struct qedi_ctx *qedi, u16
> vlanid)
>  	return rc;
>  }
> 
> -static void qedi_offload_work(struct work_struct *work) -{
> -	struct qedi_endpoint *qedi_ep =
> -		container_of(work, struct qedi_endpoint, offload_work);
> -	struct qedi_ctx *qedi;
> -	int wait_delay = 5 * HZ;
> -	int ret;
> -
> -	qedi = qedi_ep->qedi;
> -
> -	ret = qedi_iscsi_offload_conn(qedi_ep);
> -	if (ret) {
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> -			 qedi_ep->iscsi_cid, qedi_ep, ret);
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		return;
> -	}
> -
> -	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> -					       (qedi_ep->state ==
> -					       EP_STATE_OFLDCONN_COMPL),
> -					       wait_delay);
> -	if ((ret <= 0) || (qedi_ep->state != EP_STATE_OFLDCONN_COMPL)) {
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> -			 qedi_ep->iscsi_cid, qedi_ep);
> -	}
> -}
> -
>  static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)  {
>  	struct qedi_ctx *qedi;
> @@ -1381,7 +1381,6 @@ static int qedi_set_path(struct Scsi_Host *shost,
> struct iscsi_path *path_data)
>  			  qedi_ep->dst_addr, qedi_ep->dst_port);
>  	}
> 
> -	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>  	queue_work(qedi->offload_thread, &qedi_ep->offload_work);
> 
>  	ret = 0;
> --
> 2.25.1
Thanks,

Acked-by: Manish Rangankar <mrangankar@marvell.com>

Re: [PATCH 02/10] scsi: iscsi: Fix offload conn cleanup when iscsid restarts

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> When userspace restarts during boot or upgrades it won't know about the
> offload driver's endpoint and connection mappings. iscsid will start by
> cleaning up the old session by doing a stop_conn call. Later if we are
> able to create a new connection, we cleanup the old endpoint during the
> binding stage. The problem is that if we do stop_conn before doing the
> ep_disconnect call offload drivers can still be executing IO. We then
> might free tasks from the under the card/driver.
> 
> This moves the ep_disconnect call to before we do the stop_conn call for
> this case. It will then work and look like a normal recovery/cleanup
> procedure from the driver's point of view.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 48 +++++++++++++++++------------
>   1 file changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 4e10457e3ab9..bf39fb5569b6 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2236,6 +2236,23 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
>   	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
>   }
>   
> +static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
> +					 struct iscsi_endpoint *ep,
> +					 bool is_active)
> +{
> +	/* Check if this was a conn error and the kernel took ownership */
> +	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> +		iscsi_ep_disconnect(conn, is_active);
> +	} else {
> +		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
> +		mutex_unlock(&conn->ep_mutex);
> +
> +		flush_work(&conn->cleanup_work);
> +
> +		mutex_lock(&conn->ep_mutex);
> +	}
> +}
> +
>   static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>   			      struct iscsi_uevent *ev)
>   {
> @@ -2256,6 +2273,16 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>   		cancel_work_sync(&conn->cleanup_work);
>   		iscsi_stop_conn(conn, flag);
>   	} else {
> +		/*
> +		 * For offload, when iscsid is restarted it won't know about
> +		 * existing endpoints so it can't do a ep_disconnect. We clean
> +		 * it up here for userspace.
> +		 */
> +		mutex_lock(&conn->ep_mutex);
> +		if (conn->ep)
> +			iscsi_if_disconnect_bound_ep(conn, conn->ep, true);
> +		mutex_unlock(&conn->ep_mutex);
> +
>   		/*
>   		 * Figure out if it was the kernel or userspace initiating this.
>   		 */
> @@ -2984,16 +3011,7 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
>   	}
>   
>   	mutex_lock(&conn->ep_mutex);
> -	/* Check if this was a conn error and the kernel took ownership */
> -	if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> -		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
> -		mutex_unlock(&conn->ep_mutex);
> -
> -		flush_work(&conn->cleanup_work);
> -		goto put_ep;
> -	}
> -
> -	iscsi_ep_disconnect(conn, false);
> +	iscsi_if_disconnect_bound_ep(conn, ep, false);
>   	mutex_unlock(&conn->ep_mutex);
>   put_ep:
>   	iscsi_put_endpoint(ep);
> @@ -3704,16 +3722,6 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
>   
>   	switch (nlh->nlmsg_type) {
>   	case ISCSI_UEVENT_BIND_CONN:
> -		if (conn->ep) {
> -			/*
> -			 * For offload boot support where iscsid is restarted
> -			 * during the pivot root stage, the ep will be intact
> -			 * here when the new iscsid instance starts up and
> -			 * reconnects.
> -			 */
> -			iscsi_ep_disconnect(conn, true);
> -		}
> -
>   		session = iscsi_session_lookup(ev->u.b_conn.sid);
>   		if (!session) {
>   			err = -EINVAL;

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

Re: [PATCH 03/10] scsi: iscsi: Release endpoint ID when its freed.

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> We can't release the endpoint ID until all references to the endpoint have
> been dropped or it could be allocated while in use. This has us use an idr
> instead of looping over all conns to find a free ID and then free the ID
> when all references have been dropped instead of when the device is only
> deleted.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 71 ++++++++++++++---------------
>   include/scsi/scsi_transport_iscsi.h |  2 +-
>   2 files changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index bf39fb5569b6..1fc7c6bfbd67 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -86,6 +86,9 @@ struct iscsi_internal {
>   	struct transport_container session_cont;
>   };
>   
> +static DEFINE_IDR(iscsi_ep_idr);
> +static DEFINE_MUTEX(iscsi_ep_idr_mutex);
> +
>   static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
>   
>   static struct workqueue_struct *iscsi_conn_cleanup_workq;
> @@ -168,6 +171,11 @@ struct device_attribute dev_attr_##_prefix##_##_name =	\
>   static void iscsi_endpoint_release(struct device *dev)
>   {
>   	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> +
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, ep->id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>   	kfree(ep);
>   }
>   
> @@ -180,7 +188,7 @@ static ssize_t
>   show_ep_handle(struct device *dev, struct device_attribute *attr, char *buf)
>   {
>   	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	return sysfs_emit(buf, "%llu\n", (unsigned long long) ep->id);
> +	return sysfs_emit(buf, "%d\n", ep->id);
>   }
>   static ISCSI_ATTR(ep, handle, S_IRUGO, show_ep_handle, NULL);
>   
> @@ -193,48 +201,32 @@ static struct attribute_group iscsi_endpoint_group = {
>   	.attrs = iscsi_endpoint_attrs,
>   };
>   
> -#define ISCSI_MAX_EPID -1
> -
> -static int iscsi_match_epid(struct device *dev, const void *data)
> -{
> -	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	const uint64_t *epid = data;
> -
> -	return *epid == ep->id;
> -}
> -
>   struct iscsi_endpoint *
>   iscsi_create_endpoint(int dd_size)
>   {
> -	struct device *dev;
>   	struct iscsi_endpoint *ep;
> -	uint64_t id;
> -	int err;
> -
> -	for (id = 1; id < ISCSI_MAX_EPID; id++) {
> -		dev = class_find_device(&iscsi_endpoint_class, NULL, &id,
> -					iscsi_match_epid);
> -		if (!dev)
> -			break;
> -		else
> -			put_device(dev);
> -	}
> -	if (id == ISCSI_MAX_EPID) {
> -		printk(KERN_ERR "Too many connections. Max supported %u\n",
> -		       ISCSI_MAX_EPID - 1);
> -		return NULL;
> -	}
> +	int err, id;
>   
>   	ep = kzalloc(sizeof(*ep) + dd_size, GFP_KERNEL);
>   	if (!ep)
>   		return NULL;
>   
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	id = idr_alloc(&iscsi_ep_idr, ep, 0, -1, GFP_NOIO);
> +	if (id < 0) {
> +		mutex_unlock(&iscsi_ep_idr_mutex);
> +		printk(KERN_ERR "Could not allocate endpoint ID. Error %d.\n",
> +		       id);
> +		goto free_ep;
> +	}
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>   	ep->id = id;
>   	ep->dev.class = &iscsi_endpoint_class;
> -	dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id);
> +	dev_set_name(&ep->dev, "ep-%d", id);
>   	err = device_register(&ep->dev);
>           if (err)
> -                goto free_ep;
> +		goto free_id;
>   
>   	err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group);
>   	if (err)
> @@ -248,6 +240,10 @@ iscsi_create_endpoint(int dd_size)
>   	device_unregister(&ep->dev);
>   	return NULL;
>   
> +free_id:
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
>   free_ep:
>   	kfree(ep);
>   	return NULL;
> @@ -275,14 +271,17 @@ EXPORT_SYMBOL_GPL(iscsi_put_endpoint);
>    */
>   struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)
>   {
> -	struct device *dev;
> +	struct iscsi_endpoint *ep;
>   
> -	dev = class_find_device(&iscsi_endpoint_class, NULL, &handle,
> -				iscsi_match_epid);
> -	if (!dev)
> -		return NULL;
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	ep = idr_find(&iscsi_ep_idr, handle);
> +	if (!ep)
> +		goto unlock;
>   
> -	return iscsi_dev_to_endpoint(dev);
> +	get_device(&ep->dev);
> +unlock:
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +	return ep;
>   }
>   EXPORT_SYMBOL_GPL(iscsi_lookup_endpoint);
>   
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..fdd486047404 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -295,7 +295,7 @@ extern void iscsi_host_for_each_session(struct Scsi_Host *shost,
>   struct iscsi_endpoint {
>   	void *dd_data;			/* LLD private data */
>   	struct device dev;
> -	uint64_t id;
> +	int id;
>   	struct iscsi_cls_conn *conn;
>   };
>   

Good idea. :)

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

Re: [PATCH 04/10] scsi: iscsi: Fix endpoint reuse regression

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> This patch fixes a bug where when using iscsi offload we can free a
> endpoint while userspace still thinks it's active. That then causes the
> endpoint ID to be reused for a new connection's endpoint while userspace
> still thinks the ID is for the original connection. Userspace will then
> end up disconnecting a running connection's endpoint or trying to bind
> to another connection's endpoint.
> 
> This bug is a regression added in:
> 
> Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> 
> where we added a in kernel ep_disconnect call to fix a bug in:
> 
> Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> 
> where we would call stop_conn without having done ep_disconnect. This
> early ep_disconnect call will then free the endpoint and it's ID while
> userspace still thinks the ID is valid.
> 
> This patch fixes the early release of the ID by having the in kernel
> recovery code keep a reference to the endpoint until userspace has called
> into the kernel to finish cleaning up the endpoint/connection. It requires
> the previous patch "scsi: iscsi: Release endpoint ID when its freed."
> which moved the freeing of the ID until when the endpoint is released.
> 
> Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 1fc7c6bfbd67..f200da049f3b 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2247,7 +2247,11 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>   		mutex_unlock(&conn->ep_mutex);
>   
>   		flush_work(&conn->cleanup_work);
> -
> +		/*
> +		 * Userspace is now done with the EP so we can release the ref
> +		 * iscsi_cleanup_conn_work_fn took.
> +		 */
> +		iscsi_put_endpoint(ep);
>   		mutex_lock(&conn->ep_mutex);
>   	}
>   }
> @@ -2322,6 +2326,12 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>   		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.
> +	 */
> +	if (conn->ep)
> +		get_device(&conn->ep->dev);
>   	iscsi_ep_disconnect(conn, false);
>   
>   	if (system_state != SYSTEM_RUNNING) {

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

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

[Lee Duncan]

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>

Re: [PATCH 06/10] scsi: iscsi: Fix unbound endpoint error handling

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> 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.
> 
> 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 | 65 ++++++++++++++++-------------
>   1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 63a4f0c022fd..2c0dd64159b0 100644
> --- 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;
> @@ -2321,21 +2321,6 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>   	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
>   
>   	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);
>   }

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

Re: [PATCH 09/10] scsi: qedi: Fix failed disconnect handling.

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> We set the qedi_ep state to EP_STATE_OFLDCONN_START when the ep is
> created. Then in qedi_set_path we kick off the offload work. If userspace
> times out the connection and calls ep_disconnect, qedi will only flush the
> offload work if the qedi_ep state has transitioned away from
> EP_STATE_OFLDCONN_START. If we can't connect we will not have transitioned
> state and will leave the offload work running, and we will free the
> qedi_ep from under it.
> 
> This patch just has us init the work when we create the ep, then always
> flush it.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/qedi/qedi_iscsi.c | 69 +++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
> index 8196f89f404e..31ec429104e2 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.c
> +++ b/drivers/scsi/qedi/qedi_iscsi.c
> @@ -860,6 +860,37 @@ static int qedi_task_xmit(struct iscsi_task *task)
>   	return qedi_iscsi_send_ioreq(task);
>   }
>   
> +static void qedi_offload_work(struct work_struct *work)
> +{
> +	struct qedi_endpoint *qedi_ep =
> +		container_of(work, struct qedi_endpoint, offload_work);
> +	struct qedi_ctx *qedi;
> +	int wait_delay = 5 * HZ;
> +	int ret;
> +
> +	qedi = qedi_ep->qedi;
> +
> +	ret = qedi_iscsi_offload_conn(qedi_ep);
> +	if (ret) {
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> +			 qedi_ep->iscsi_cid, qedi_ep, ret);
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		return;
> +	}
> +
> +	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> +					       (qedi_ep->state ==
> +					       EP_STATE_OFLDCONN_COMPL),
> +					       wait_delay);
> +	if (ret <= 0 || qedi_ep->state != EP_STATE_OFLDCONN_COMPL) {
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> +			 qedi_ep->iscsi_cid, qedi_ep);
> +	}
> +}
> +
>   static struct iscsi_endpoint *
>   qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
>   		int non_blocking)
> @@ -908,6 +939,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
>   	}
>   	qedi_ep = ep->dd_data;
>   	memset(qedi_ep, 0, sizeof(struct qedi_endpoint));
> +	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>   	qedi_ep->state = EP_STATE_IDLE;
>   	qedi_ep->iscsi_cid = (u32)-1;
>   	qedi_ep->qedi = qedi;
> @@ -1056,12 +1088,11 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
>   	qedi_ep = ep->dd_data;
>   	qedi = qedi_ep->qedi;
>   
> +	flush_work(&qedi_ep->offload_work);
> +
>   	if (qedi_ep->state == EP_STATE_OFLDCONN_START)
>   		goto ep_exit_recover;
>   
> -	if (qedi_ep->state != EP_STATE_OFLDCONN_NONE)
> -		flush_work(&qedi_ep->offload_work);
> -
>   	if (qedi_ep->conn) {
>   		qedi_conn = qedi_ep->conn;
>   		abrt_conn = qedi_conn->abrt_conn;
> @@ -1235,37 +1266,6 @@ static int qedi_data_avail(struct qedi_ctx *qedi, u16 vlanid)
>   	return rc;
>   }
>   
> -static void qedi_offload_work(struct work_struct *work)
> -{
> -	struct qedi_endpoint *qedi_ep =
> -		container_of(work, struct qedi_endpoint, offload_work);
> -	struct qedi_ctx *qedi;
> -	int wait_delay = 5 * HZ;
> -	int ret;
> -
> -	qedi = qedi_ep->qedi;
> -
> -	ret = qedi_iscsi_offload_conn(qedi_ep);
> -	if (ret) {
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> -			 qedi_ep->iscsi_cid, qedi_ep, ret);
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		return;
> -	}
> -
> -	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> -					       (qedi_ep->state ==
> -					       EP_STATE_OFLDCONN_COMPL),
> -					       wait_delay);
> -	if ((ret <= 0) || (qedi_ep->state != EP_STATE_OFLDCONN_COMPL)) {
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> -			 qedi_ep->iscsi_cid, qedi_ep);
> -	}
> -}
> -
>   static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
>   {
>   	struct qedi_ctx *qedi;
> @@ -1381,7 +1381,6 @@ static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
>   			  qedi_ep->dst_addr, qedi_ep->dst_port);
>   	}
>   
> -	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>   	queue_work(qedi->offload_thread, &qedi_ep->offload_work);
>   
>   	ret = 0;

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

Re: [PATCH 10/10] scsi: iscsi: Add Mike Christie as co-maintainer

[Lee Duncan]

On 4/7/22 17:13, Mike Christie wrote:
> I've been doing a lot of iscsi patches because Oracle is paying me to work
> on iSCSI again. It was supposed to be temp assignment, but my co-worker
> that was working on iscsi moved to a new group so it looks like I'm back
> on this code again. After talking to Chris and Lee this patch adds me back
> as co-maintainer, so I can help them and people remember to cc me on
> issues.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fd768d43e048..ca9d56121974 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10369,6 +10369,7 @@ F:	include/linux/isapnp.h
>   ISCSI
>   M:	Lee Duncan <lduncan@suse.com>
>   M:	Chris Leech <cleech@redhat.com>
> +M:	Mike Christie <michael.christie@oracle.com>
>   L:	open-iscsi@googlegroups.com
>   L:	linux-scsi@vger.kernel.org
>   S:	Maintained

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

Re: [PATCH 01/10] scsi: iscsi: Move iscsi_ep_disconnect

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:05PM -0500, Mike Christie wrote:
> This patch moves iscsi_ep_disconnect so it can be called earlier in the
> next patch.

Reviewed-by: Chris Leech <cleech@redhat.com>
 
> Reviewed-by: Lee Duncan <lduncan@suse.com>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 38 ++++++++++++++---------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 27951ea05dd4..4e10457e3ab9 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2217,6 +2217,25 @@ static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
>  	ISCSI_DBG_TRANS_CONN(conn, "Stopping conn done.\n");
>  }
>  
> +static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
> +{
> +	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
> +	struct iscsi_endpoint *ep;
> +
> +	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
> +	conn->state = ISCSI_CONN_FAILED;
> +
> +	if (!conn->ep || !session->transport->ep_disconnect)
> +		return;
> +
> +	ep = conn->ep;
> +	conn->ep = NULL;
> +
> +	session->transport->unbind_conn(conn, is_active);
> +	session->transport->ep_disconnect(ep);
> +	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
> +}
> +
>  static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>  			      struct iscsi_uevent *ev)
>  {
> @@ -2257,25 +2276,6 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>  	return 0;
>  }
>  
> -static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
> -{
> -	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
> -	struct iscsi_endpoint *ep;
> -
> -	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
> -	conn->state = ISCSI_CONN_FAILED;
> -
> -	if (!conn->ep || !session->transport->ep_disconnect)
> -		return;
> -
> -	ep = conn->ep;
> -	conn->ep = NULL;
> -
> -	session->transport->unbind_conn(conn, is_active);
> -	session->transport->ep_disconnect(ep);
> -	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
> -}
> -
>  static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>  {
>  	struct iscsi_cls_conn *conn = container_of(work, struct iscsi_cls_conn,
> -- 
> 2.25.1
> 

Re: [PATCH 02/10] scsi: iscsi: Fix offload conn cleanup when iscsid restarts

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:06PM -0500, Mike Christie wrote:
> When userspace restarts during boot or upgrades it won't know about the
> offload driver's endpoint and connection mappings. iscsid will start by
> cleaning up the old session by doing a stop_conn call. Later if we are
> able to create a new connection, we cleanup the old endpoint during the
> binding stage. The problem is that if we do stop_conn before doing the
> ep_disconnect call offload drivers can still be executing IO. We then
> might free tasks from the under the card/driver.
> 
> This moves the ep_disconnect call to before we do the stop_conn call for
> this case. It will then work and look like a normal recovery/cleanup
> procedure from the driver's point of view.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 48 +++++++++++++++++------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Reviewed-by: Chris Leech <cleech@redhat.com>
 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 4e10457e3ab9..bf39fb5569b6 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2236,6 +2236,23 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
>  	ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
>  }
>  
> +static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
> +					 struct iscsi_endpoint *ep,
> +					 bool is_active)
> +{
> +	/* Check if this was a conn error and the kernel took ownership */
> +	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> +		iscsi_ep_disconnect(conn, is_active);
> +	} else {
> +		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
> +		mutex_unlock(&conn->ep_mutex);
> +
> +		flush_work(&conn->cleanup_work);
> +
> +		mutex_lock(&conn->ep_mutex);
> +	}
> +}
> +
>  static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>  			      struct iscsi_uevent *ev)
>  {
> @@ -2256,6 +2273,16 @@ static int iscsi_if_stop_conn(struct iscsi_transport *transport,
>  		cancel_work_sync(&conn->cleanup_work);
>  		iscsi_stop_conn(conn, flag);
>  	} else {
> +		/*
> +		 * For offload, when iscsid is restarted it won't know about
> +		 * existing endpoints so it can't do a ep_disconnect. We clean
> +		 * it up here for userspace.
> +		 */
> +		mutex_lock(&conn->ep_mutex);
> +		if (conn->ep)
> +			iscsi_if_disconnect_bound_ep(conn, conn->ep, true);
> +		mutex_unlock(&conn->ep_mutex);
> +
>  		/*
>  		 * Figure out if it was the kernel or userspace initiating this.
>  		 */
> @@ -2984,16 +3011,7 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
>  	}
>  
>  	mutex_lock(&conn->ep_mutex);
> -	/* Check if this was a conn error and the kernel took ownership */
> -	if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> -		ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
> -		mutex_unlock(&conn->ep_mutex);
> -
> -		flush_work(&conn->cleanup_work);
> -		goto put_ep;
> -	}
> -
> -	iscsi_ep_disconnect(conn, false);
> +	iscsi_if_disconnect_bound_ep(conn, ep, false);
>  	mutex_unlock(&conn->ep_mutex);
>  put_ep:
>  	iscsi_put_endpoint(ep);
> @@ -3704,16 +3722,6 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
>  
>  	switch (nlh->nlmsg_type) {
>  	case ISCSI_UEVENT_BIND_CONN:
> -		if (conn->ep) {
> -			/*
> -			 * For offload boot support where iscsid is restarted
> -			 * during the pivot root stage, the ep will be intact
> -			 * here when the new iscsid instance starts up and
> -			 * reconnects.
> -			 */
> -			iscsi_ep_disconnect(conn, true);
> -		}
> -
>  		session = iscsi_session_lookup(ev->u.b_conn.sid);
>  		if (!session) {
>  			err = -EINVAL;
> -- 
> 2.25.1
> 

Re: [PATCH 03/10] scsi: iscsi: Release endpoint ID when its freed.

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:07PM -0500, Mike Christie wrote:
> We can't release the endpoint ID until all references to the endpoint have
> been dropped or it could be allocated while in use. This has us use an idr
> instead of looping over all conns to find a free ID and then free the ID
> when all references have been dropped instead of when the device is only
> deleted.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 71 ++++++++++++++---------------
>  include/scsi/scsi_transport_iscsi.h |  2 +-
>  2 files changed, 36 insertions(+), 37 deletions(-)
 
Reviewed-by: Chris Leech <cleech@redhat.com>

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index bf39fb5569b6..1fc7c6bfbd67 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -86,6 +86,9 @@ struct iscsi_internal {
>  	struct transport_container session_cont;
>  };
>  
> +static DEFINE_IDR(iscsi_ep_idr);
> +static DEFINE_MUTEX(iscsi_ep_idr_mutex);
> +
>  static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
>  
>  static struct workqueue_struct *iscsi_conn_cleanup_workq;
> @@ -168,6 +171,11 @@ struct device_attribute dev_attr_##_prefix##_##_name =	\
>  static void iscsi_endpoint_release(struct device *dev)
>  {
>  	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> +
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, ep->id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>  	kfree(ep);
>  }
>  
> @@ -180,7 +188,7 @@ static ssize_t
>  show_ep_handle(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	return sysfs_emit(buf, "%llu\n", (unsigned long long) ep->id);
> +	return sysfs_emit(buf, "%d\n", ep->id);
>  }
>  static ISCSI_ATTR(ep, handle, S_IRUGO, show_ep_handle, NULL);
>  
> @@ -193,48 +201,32 @@ static struct attribute_group iscsi_endpoint_group = {
>  	.attrs = iscsi_endpoint_attrs,
>  };
>  
> -#define ISCSI_MAX_EPID -1
> -
> -static int iscsi_match_epid(struct device *dev, const void *data)
> -{
> -	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	const uint64_t *epid = data;
> -
> -	return *epid == ep->id;
> -}
> -
>  struct iscsi_endpoint *
>  iscsi_create_endpoint(int dd_size)
>  {
> -	struct device *dev;
>  	struct iscsi_endpoint *ep;
> -	uint64_t id;
> -	int err;
> -
> -	for (id = 1; id < ISCSI_MAX_EPID; id++) {
> -		dev = class_find_device(&iscsi_endpoint_class, NULL, &id,
> -					iscsi_match_epid);
> -		if (!dev)
> -			break;
> -		else
> -			put_device(dev);
> -	}
> -	if (id == ISCSI_MAX_EPID) {
> -		printk(KERN_ERR "Too many connections. Max supported %u\n",
> -		       ISCSI_MAX_EPID - 1);
> -		return NULL;
> -	}
> +	int err, id;
>  
>  	ep = kzalloc(sizeof(*ep) + dd_size, GFP_KERNEL);
>  	if (!ep)
>  		return NULL;
>  
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	id = idr_alloc(&iscsi_ep_idr, ep, 0, -1, GFP_NOIO);
> +	if (id < 0) {
> +		mutex_unlock(&iscsi_ep_idr_mutex);
> +		printk(KERN_ERR "Could not allocate endpoint ID. Error %d.\n",
> +		       id);
> +		goto free_ep;
> +	}
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>  	ep->id = id;
>  	ep->dev.class = &iscsi_endpoint_class;
> -	dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id);
> +	dev_set_name(&ep->dev, "ep-%d", id);
>  	err = device_register(&ep->dev);
>          if (err)
> -                goto free_ep;
> +		goto free_id;
>  
>  	err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group);
>  	if (err)
> @@ -248,6 +240,10 @@ iscsi_create_endpoint(int dd_size)
>  	device_unregister(&ep->dev);
>  	return NULL;
>  
> +free_id:
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
>  free_ep:
>  	kfree(ep);
>  	return NULL;
> @@ -275,14 +271,17 @@ EXPORT_SYMBOL_GPL(iscsi_put_endpoint);
>   */
>  struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)
>  {
> -	struct device *dev;
> +	struct iscsi_endpoint *ep;
>  
> -	dev = class_find_device(&iscsi_endpoint_class, NULL, &handle,
> -				iscsi_match_epid);
> -	if (!dev)
> -		return NULL;
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	ep = idr_find(&iscsi_ep_idr, handle);
> +	if (!ep)
> +		goto unlock;
>  
> -	return iscsi_dev_to_endpoint(dev);
> +	get_device(&ep->dev);
> +unlock:
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +	return ep;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_lookup_endpoint);
>  
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..fdd486047404 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -295,7 +295,7 @@ extern void iscsi_host_for_each_session(struct Scsi_Host *shost,
>  struct iscsi_endpoint {
>  	void *dd_data;			/* LLD private data */
>  	struct device dev;
> -	uint64_t id;
> +	int id;
>  	struct iscsi_cls_conn *conn;
>  };
>  
> -- 
> 2.25.1
> 

Re: [PATCH 04/10] scsi: iscsi: Fix endpoint reuse regression

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:08PM -0500, Mike Christie wrote:
> This patch fixes a bug where when using iscsi offload we can free a
> endpoint while userspace still thinks it's active. That then causes the
> endpoint ID to be reused for a new connection's endpoint while userspace
> still thinks the ID is for the original connection. Userspace will then
> end up disconnecting a running connection's endpoint or trying to bind
> to another connection's endpoint.
> 
> This bug is a regression added in:
> 
> Commit 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> 
> where we added a in kernel ep_disconnect call to fix a bug in:
> 
> Commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> 
> where we would call stop_conn without having done ep_disconnect. This
> early ep_disconnect call will then free the endpoint and it's ID while
> userspace still thinks the ID is valid.
> 
> This patch fixes the early release of the ID by having the in kernel
> recovery code keep a reference to the endpoint until userspace has called
> into the kernel to finish cleaning up the endpoint/connection. It requires
> the previous patch "scsi: iscsi: Release endpoint ID when its freed."
> which moved the freeing of the ID until when the endpoint is released.
> 
> Fixes: 23d6fefbb3f6 ("scsi: iscsi: Fix in-kernel conn failure handling")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
 
Reviewed-by: Chris Leech <cleech@redhat.com>

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 1fc7c6bfbd67..f200da049f3b 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2247,7 +2247,11 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>  		mutex_unlock(&conn->ep_mutex);
>  
>  		flush_work(&conn->cleanup_work);
> -
> +		/*
> +		 * Userspace is now done with the EP so we can release the ref
> +		 * iscsi_cleanup_conn_work_fn took.
> +		 */
> +		iscsi_put_endpoint(ep);
>  		mutex_lock(&conn->ep_mutex);
>  	}
>  }
> @@ -2322,6 +2326,12 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>  		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.
> +	 */
> +	if (conn->ep)
> +		get_device(&conn->ep->dev);
>  	iscsi_ep_disconnect(conn, false);
>  
>  	if (system_state != SYSTEM_RUNNING) {
> -- 
> 2.25.1
> 

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

[Chris Leech]

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
> 

Re: [PATCH 06/10] scsi: iscsi: Fix unbound endpoint error handling

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:10PM -0500, Mike Christie wrote:
> 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.
> 
> 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 | 65 ++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 29 deletions(-)
 
Reviewed-by: Chris Leech <cleech@redhat.com>

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 63a4f0c022fd..2c0dd64159b0 100644
> --- 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;
> @@ -2321,21 +2321,6 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
>  	struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
>  
>  	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);
>  }
> -- 
> 2.25.1
> 

Re: [PATCH 07/10] scsi: iscsi: Merge suspend fields

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:11PM -0500, Mike Christie wrote:
> Move the tx and rx suspend fields into one flags field.
> 
> Reviewed-by: Lee Duncan <lduncan@suse.com>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/bnx2i/bnx2i_hwi.c   |  2 +-
>  drivers/scsi/bnx2i/bnx2i_iscsi.c |  2 +-
>  drivers/scsi/cxgbi/libcxgbi.c    |  6 +++---
>  drivers/scsi/libiscsi.c          | 20 ++++++++++----------
>  drivers/scsi/libiscsi_tcp.c      |  2 +-
>  include/scsi/libiscsi.h          |  9 +++++----
>  6 files changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Chris Leech <cleech@redhat.com>
 
> diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
> index 5521469ce678..e16327a4b4c9 100644
> --- a/drivers/scsi/bnx2i/bnx2i_hwi.c
> +++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
> @@ -1977,7 +1977,7 @@ static int bnx2i_process_new_cqes(struct bnx2i_conn *bnx2i_conn)
>  		if (nopin->cq_req_sn != qp->cqe_exp_seq_sn)
>  			break;
>  
> -		if (unlikely(test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx))) {
> +		if (unlikely(test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
>  			if (nopin->op_code == ISCSI_OP_NOOP_IN &&
>  			    nopin->itt == (u16) RESERVED_ITT) {
>  				printk(KERN_ALERT "bnx2i: Unsolicited "
> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> index fe86fd61a995..15fbd09baa94 100644
> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> @@ -1721,7 +1721,7 @@ static int bnx2i_tear_down_conn(struct bnx2i_hba *hba,
>  			struct iscsi_conn *conn = ep->conn->cls_conn->dd_data;
>  
>  			/* Must suspend all rx queue activity for this ep */
> -			set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
> +			set_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
>  		}
>  		/* CONN_DISCONNECT timeout may or may not be an issue depending
>  		 * on what transcribed in TCP layer, different targets behave
> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index 8c7d4dda4cf2..4365d52c6430 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
> @@ -1634,11 +1634,11 @@ void cxgbi_conn_pdu_ready(struct cxgbi_sock *csk)
>  	log_debug(1 << CXGBI_DBG_PDU_RX,
>  		"csk 0x%p, conn 0x%p.\n", csk, conn);
>  
> -	if (unlikely(!conn || conn->suspend_rx)) {
> +	if (unlikely(!conn || test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
>  		log_debug(1 << CXGBI_DBG_PDU_RX,
> -			"csk 0x%p, conn 0x%p, id %d, suspend_rx %lu!\n",
> +			"csk 0x%p, conn 0x%p, id %d, conn flags 0x%lx!\n",
>  			csk, conn, conn ? conn->id : 0xFF,
> -			conn ? conn->suspend_rx : 0xFF);
> +			conn ? conn->flags : 0xFF);
>  		return;
>  	}
>  
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index d09926e6c8a8..5e7bd5a3b430 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1392,8 +1392,8 @@ static bool iscsi_set_conn_failed(struct iscsi_conn *conn)
>  	if (conn->stop_stage == 0)
>  		session->state = ISCSI_STATE_FAILED;
>  
> -	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
> -	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
> +	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
> +	set_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
>  	return true;
>  }
>  
> @@ -1454,7 +1454,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
>  	 * Do this after dropping the extra ref because if this was a requeue
>  	 * it's removed from that list and cleanup_queued_task would miss it.
>  	 */
> -	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
> +	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
>  		/*
>  		 * Save the task and ref in case we weren't cleaning up this
>  		 * task and get woken up again.
> @@ -1532,7 +1532,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
>  	int rc = 0;
>  
>  	spin_lock_bh(&conn->session->frwd_lock);
> -	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
> +	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
>  		ISCSI_DBG_SESSION(conn->session, "Tx suspended!\n");
>  		spin_unlock_bh(&conn->session->frwd_lock);
>  		return -ENODATA;
> @@ -1746,7 +1746,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
>  		goto fault;
>  	}
>  
> -	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
> +	if (test_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags)) {
>  		reason = FAILURE_SESSION_IN_RECOVERY;
>  		sc->result = DID_REQUEUE << 16;
>  		goto fault;
> @@ -1935,7 +1935,7 @@ static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
>  void iscsi_suspend_queue(struct iscsi_conn *conn)
>  {
>  	spin_lock_bh(&conn->session->frwd_lock);
> -	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
> +	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
>  	spin_unlock_bh(&conn->session->frwd_lock);
>  }
>  EXPORT_SYMBOL_GPL(iscsi_suspend_queue);
> @@ -1953,7 +1953,7 @@ void iscsi_suspend_tx(struct iscsi_conn *conn)
>  	struct Scsi_Host *shost = conn->session->host;
>  	struct iscsi_host *ihost = shost_priv(shost);
>  
> -	set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
> +	set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
>  	if (ihost->workq)
>  		flush_workqueue(ihost->workq);
>  }
> @@ -1961,7 +1961,7 @@ EXPORT_SYMBOL_GPL(iscsi_suspend_tx);
>  
>  static void iscsi_start_tx(struct iscsi_conn *conn)
>  {
> -	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
> +	clear_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
>  	iscsi_conn_queue_work(conn);
>  }
>  
> @@ -3330,8 +3330,8 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
>  	/*
>  	 * Unblock xmitworker(), Login Phase will pass through.
>  	 */
> -	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
> -	clear_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
> +	clear_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
> +	clear_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_conn_bind);
> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index 2e9ffe3d1a55..883005757ddb 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -927,7 +927,7 @@ int iscsi_tcp_recv_skb(struct iscsi_conn *conn, struct sk_buff *skb,
>  	 */
>  	conn->last_recv = jiffies;
>  
> -	if (unlikely(conn->suspend_rx)) {
> +	if (unlikely(test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))) {
>  		ISCSI_DBG_TCP(conn, "Rx suspended!\n");
>  		*status = ISCSI_TCP_SUSPENDED;
>  		return 0;
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index e76c94697c1b..84086c240228 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -53,8 +53,10 @@ enum {
>  
>  #define ISID_SIZE			6
>  
> -/* Connection suspend "bit" */
> -#define ISCSI_SUSPEND_BIT		1
> +/* Connection flags */
> +#define ISCSI_CONN_FLAG_SUSPEND_TX	BIT(0)
> +#define ISCSI_CONN_FLAG_SUSPEND_RX	BIT(1)
> +
>  
>  #define ISCSI_ITT_MASK			0x1fff
>  #define ISCSI_TOTAL_CMDS_MAX		4096
> @@ -211,8 +213,7 @@ struct iscsi_conn {
>  	struct list_head	cmdqueue;	/* data-path cmd queue */
>  	struct list_head	requeue;	/* tasks needing another run */
>  	struct work_struct	xmitwork;	/* per-conn. xmit workqueue */
> -	unsigned long		suspend_tx;	/* suspend Tx */
> -	unsigned long		suspend_rx;	/* suspend Rx */
> +	unsigned long		flags;		/* ISCSI_CONN_FLAGs */
>  
>  	/* negotiated params */
>  	unsigned		max_recv_dlength; /* initiator_max_recv_dsl*/
> -- 
> 2.25.1
> 

Re: [PATCH 10/10] scsi: iscsi: Add Mike Christie as co-maintainer

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:14PM -0500, Mike Christie wrote:
> I've been doing a lot of iscsi patches because Oracle is paying me to work
> on iSCSI again. It was supposed to be temp assignment, but my co-worker
> that was working on iscsi moved to a new group so it looks like I'm back
> on this code again. After talking to Chris and Lee this patch adds me back
> as co-maintainer, so I can help them and people remember to cc me on
> issues.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
 
Acked-by: Chris Leech <cleech@redhat.com>

> diff --git a/MAINTAINERS b/MAINTAINERS
> index fd768d43e048..ca9d56121974 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10369,6 +10369,7 @@ F:	include/linux/isapnp.h
>  ISCSI
>  M:	Lee Duncan <lduncan@suse.com>
>  M:	Chris Leech <cleech@redhat.com>
> +M:	Mike Christie <michael.christie@oracle.com>
>  L:	open-iscsi@googlegroups.com
>  L:	linux-scsi@vger.kernel.org
>  S:	Maintained
> -- 
> 2.25.1
> 

Re: [PATCH 08/10] scsi: iscsi: Fix nop handling during conn recovery

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:12PM -0500, Mike Christie wrote:
> If a offload driver doesn't use the xmit workqueue, then when we are
> doing ep_disconnect libiscsi can still inject PDUs to the driver. This
> adds a check for if the connection is bound before trying to inject PDUs.
> 
> Reviewed-by: Lee Duncan <lduncan@suse.com>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/libiscsi.c | 7 ++++++-
>  include/scsi/libiscsi.h | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Chris Leech <cleech@redhat.com>
 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 5e7bd5a3b430..0bf8cf8585bb 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -678,7 +678,8 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  	struct iscsi_task *task;
>  	itt_t itt;
>  
> -	if (session->state == ISCSI_STATE_TERMINATE)
> +	if (session->state == ISCSI_STATE_TERMINATE ||
> +	    !test_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags))
>  		return NULL;
>  
>  	if (opcode == ISCSI_OP_LOGIN || opcode == ISCSI_OP_TEXT) {
> @@ -2214,6 +2215,8 @@ void iscsi_conn_unbind(struct iscsi_cls_conn *cls_conn, bool is_active)
>  	iscsi_suspend_tx(conn);
>  
>  	spin_lock_bh(&session->frwd_lock);
> +	clear_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags);
> +
>  	if (!is_active) {
>  		/*
>  		 * if logout timed out before userspace could even send a PDU
> @@ -3318,6 +3321,8 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session,
>  	spin_lock_bh(&session->frwd_lock);
>  	if (is_leading)
>  		session->leadconn = conn;
> +
> +	set_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags);
>  	spin_unlock_bh(&session->frwd_lock);
>  
>  	/*
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index 84086c240228..d0a24779c52d 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -56,7 +56,7 @@ enum {
>  /* Connection flags */
>  #define ISCSI_CONN_FLAG_SUSPEND_TX	BIT(0)
>  #define ISCSI_CONN_FLAG_SUSPEND_RX	BIT(1)
> -
> +#define ISCSI_CONN_FLAG_BOUND		BIT(2)
>  
>  #define ISCSI_ITT_MASK			0x1fff
>  #define ISCSI_TOTAL_CMDS_MAX		4096
> -- 
> 2.25.1
> 

Re: [PATCH 09/10] scsi: qedi: Fix failed disconnect handling.

[Chris Leech]

On Thu, Apr 07, 2022 at 07:13:13PM -0500, Mike Christie wrote:
> We set the qedi_ep state to EP_STATE_OFLDCONN_START when the ep is
> created. Then in qedi_set_path we kick off the offload work. If userspace
> times out the connection and calls ep_disconnect, qedi will only flush the
> offload work if the qedi_ep state has transitioned away from
> EP_STATE_OFLDCONN_START. If we can't connect we will not have transitioned
> state and will leave the offload work running, and we will free the
> qedi_ep from under it.
> 
> This patch just has us init the work when we create the ep, then always
> flush it.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/qedi/qedi_iscsi.c | 69 +++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 35 deletions(-)

Reviewed-by: Chris Leech <cleech@redhat.com>
 
> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
> index 8196f89f404e..31ec429104e2 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.c
> +++ b/drivers/scsi/qedi/qedi_iscsi.c
> @@ -860,6 +860,37 @@ static int qedi_task_xmit(struct iscsi_task *task)
>  	return qedi_iscsi_send_ioreq(task);
>  }
>  
> +static void qedi_offload_work(struct work_struct *work)
> +{
> +	struct qedi_endpoint *qedi_ep =
> +		container_of(work, struct qedi_endpoint, offload_work);
> +	struct qedi_ctx *qedi;
> +	int wait_delay = 5 * HZ;
> +	int ret;
> +
> +	qedi = qedi_ep->qedi;
> +
> +	ret = qedi_iscsi_offload_conn(qedi_ep);
> +	if (ret) {
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> +			 qedi_ep->iscsi_cid, qedi_ep, ret);
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		return;
> +	}
> +
> +	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> +					       (qedi_ep->state ==
> +					       EP_STATE_OFLDCONN_COMPL),
> +					       wait_delay);
> +	if (ret <= 0 || qedi_ep->state != EP_STATE_OFLDCONN_COMPL) {
> +		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> +		QEDI_ERR(&qedi->dbg_ctx,
> +			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> +			 qedi_ep->iscsi_cid, qedi_ep);
> +	}
> +}
> +
>  static struct iscsi_endpoint *
>  qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
>  		int non_blocking)
> @@ -908,6 +939,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
>  	}
>  	qedi_ep = ep->dd_data;
>  	memset(qedi_ep, 0, sizeof(struct qedi_endpoint));
> +	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>  	qedi_ep->state = EP_STATE_IDLE;
>  	qedi_ep->iscsi_cid = (u32)-1;
>  	qedi_ep->qedi = qedi;
> @@ -1056,12 +1088,11 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
>  	qedi_ep = ep->dd_data;
>  	qedi = qedi_ep->qedi;
>  
> +	flush_work(&qedi_ep->offload_work);
> +
>  	if (qedi_ep->state == EP_STATE_OFLDCONN_START)
>  		goto ep_exit_recover;
>  
> -	if (qedi_ep->state != EP_STATE_OFLDCONN_NONE)
> -		flush_work(&qedi_ep->offload_work);
> -
>  	if (qedi_ep->conn) {
>  		qedi_conn = qedi_ep->conn;
>  		abrt_conn = qedi_conn->abrt_conn;
> @@ -1235,37 +1266,6 @@ static int qedi_data_avail(struct qedi_ctx *qedi, u16 vlanid)
>  	return rc;
>  }
>  
> -static void qedi_offload_work(struct work_struct *work)
> -{
> -	struct qedi_endpoint *qedi_ep =
> -		container_of(work, struct qedi_endpoint, offload_work);
> -	struct qedi_ctx *qedi;
> -	int wait_delay = 5 * HZ;
> -	int ret;
> -
> -	qedi = qedi_ep->qedi;
> -
> -	ret = qedi_iscsi_offload_conn(qedi_ep);
> -	if (ret) {
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "offload error: iscsi_cid=%u, qedi_ep=%p, ret=%d\n",
> -			 qedi_ep->iscsi_cid, qedi_ep, ret);
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		return;
> -	}
> -
> -	ret = wait_event_interruptible_timeout(qedi_ep->tcp_ofld_wait,
> -					       (qedi_ep->state ==
> -					       EP_STATE_OFLDCONN_COMPL),
> -					       wait_delay);
> -	if ((ret <= 0) || (qedi_ep->state != EP_STATE_OFLDCONN_COMPL)) {
> -		qedi_ep->state = EP_STATE_OFLDCONN_FAILED;
> -		QEDI_ERR(&qedi->dbg_ctx,
> -			 "Offload conn TIMEOUT iscsi_cid=%u, qedi_ep=%p\n",
> -			 qedi_ep->iscsi_cid, qedi_ep);
> -	}
> -}
> -
>  static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
>  {
>  	struct qedi_ctx *qedi;
> @@ -1381,7 +1381,6 @@ static int qedi_set_path(struct Scsi_Host *shost, struct iscsi_path *path_data)
>  			  qedi_ep->dst_addr, qedi_ep->dst_port);
>  	}
>  
> -	INIT_WORK(&qedi_ep->offload_work, qedi_offload_work);
>  	queue_work(qedi->offload_thread, &qedi_ep->offload_work);
>  
>  	ret = 0;
> -- 
> 2.25.1
> 

RE: [PATCH 03/10] scsi: iscsi: Release endpoint ID when its freed.

[wubo (T)]

> -----Original Message-----
> From: Mike Christie [mailto:michael.christie@oracle.com]
> Sent: Friday, April 8, 2022 8:13 AM
> To: skashyap@marvell.com; lduncan@suse.com; cleech@redhat.com;
> njavali@marvell.com; mrangankar@marvell.com;
> GR-QLogic-Storage-Upstream@marvell.com; martin.petersen@oracle.com;
> linux-scsi@vger.kernel.org; jejb@linux.ibm.com
> Cc: Mike Christie <michael.christie@oracle.com>
> Subject: [PATCH 03/10] scsi: iscsi: Release endpoint ID when its freed.
> 
> We can't release the endpoint ID until all references to the endpoint have been
> dropped or it could be allocated while in use. This has us use an idr instead of
> looping over all conns to find a free ID and then free the ID when all references
> have been dropped instead of when the device is only deleted.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 71 ++++++++++++++---------------
> include/scsi/scsi_transport_iscsi.h |  2 +-
>  2 files changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index bf39fb5569b6..1fc7c6bfbd67 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -86,6 +86,9 @@ struct iscsi_internal {
>  	struct transport_container session_cont;  };
> 
> +static DEFINE_IDR(iscsi_ep_idr);
> +static DEFINE_MUTEX(iscsi_ep_idr_mutex);
> +
>  static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
> 
>  static struct workqueue_struct *iscsi_conn_cleanup_workq;
> @@ -168,6 +171,11 @@ struct device_attribute dev_attr_##_prefix##_##_name
> =	\
>  static void iscsi_endpoint_release(struct device *dev)  {
>  	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> +
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, ep->id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>  	kfree(ep);
>  }
> 
> @@ -180,7 +188,7 @@ static ssize_t
>  show_ep_handle(struct device *dev, struct device_attribute *attr, char *buf)
> {
>  	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	return sysfs_emit(buf, "%llu\n", (unsigned long long) ep->id);
> +	return sysfs_emit(buf, "%d\n", ep->id);
>  }
>  static ISCSI_ATTR(ep, handle, S_IRUGO, show_ep_handle, NULL);
> 
> @@ -193,48 +201,32 @@ static struct attribute_group iscsi_endpoint_group = {
>  	.attrs = iscsi_endpoint_attrs,
>  };
> 
> -#define ISCSI_MAX_EPID -1
> -
> -static int iscsi_match_epid(struct device *dev, const void *data) -{
> -	struct iscsi_endpoint *ep = iscsi_dev_to_endpoint(dev);
> -	const uint64_t *epid = data;
> -
> -	return *epid == ep->id;
> -}
> -
>  struct iscsi_endpoint *
>  iscsi_create_endpoint(int dd_size)
>  {
> -	struct device *dev;
>  	struct iscsi_endpoint *ep;
> -	uint64_t id;
> -	int err;
> -
> -	for (id = 1; id < ISCSI_MAX_EPID; id++) {
> -		dev = class_find_device(&iscsi_endpoint_class, NULL, &id,
> -					iscsi_match_epid);
> -		if (!dev)
> -			break;
> -		else
> -			put_device(dev);
> -	}
> -	if (id == ISCSI_MAX_EPID) {
> -		printk(KERN_ERR "Too many connections. Max supported %u\n",
> -		       ISCSI_MAX_EPID - 1);
> -		return NULL;
> -	}
> +	int err, id;
> 
>  	ep = kzalloc(sizeof(*ep) + dd_size, GFP_KERNEL);
>  	if (!ep)
>  		return NULL;
> 
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	id = idr_alloc(&iscsi_ep_idr, ep, 0, -1, GFP_NOIO);
> +	if (id < 0) {
> +		mutex_unlock(&iscsi_ep_idr_mutex);
> +		printk(KERN_ERR "Could not allocate endpoint ID. Error %d.\n",
> +		       id);
> +		goto free_ep;
> +	}
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +
>  	ep->id = id;
>  	ep->dev.class = &iscsi_endpoint_class;
> -	dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id);
> +	dev_set_name(&ep->dev, "ep-%d", id);
>  	err = device_register(&ep->dev);
>          if (err)
> -                goto free_ep;
> +		goto free_id;
> 
>  	err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group);
>  	if (err)
> @@ -248,6 +240,10 @@ iscsi_create_endpoint(int dd_size)
>  	device_unregister(&ep->dev);
>  	return NULL;
> 
> +free_id:
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	idr_remove(&iscsi_ep_idr, id);
> +	mutex_unlock(&iscsi_ep_idr_mutex);
>  free_ep:
>  	kfree(ep);
>  	return NULL;
> @@ -275,14 +271,17 @@ EXPORT_SYMBOL_GPL(iscsi_put_endpoint);
>   */
>  struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)  {
> -	struct device *dev;
> +	struct iscsi_endpoint *ep;
> 
> -	dev = class_find_device(&iscsi_endpoint_class, NULL, &handle,
> -				iscsi_match_epid);
> -	if (!dev)
> -		return NULL;
> +	mutex_lock(&iscsi_ep_idr_mutex);
> +	ep = idr_find(&iscsi_ep_idr, handle);
> +	if (!ep)
> +		goto unlock;
> 
> -	return iscsi_dev_to_endpoint(dev);
> +	get_device(&ep->dev);
> +unlock:
> +	mutex_unlock(&iscsi_ep_idr_mutex);
> +	return ep;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_lookup_endpoint);
> 
> diff --git a/include/scsi/scsi_transport_iscsi.h
> b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..fdd486047404 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -295,7 +295,7 @@ extern void iscsi_host_for_each_session(struct Scsi_Host
> *shost,  struct iscsi_endpoint {
>  	void *dd_data;			/* LLD private data */
>  	struct device dev;
> -	uint64_t id;
> +	int id;
>  	struct iscsi_cls_conn *conn;
>  };
> 
> --


Reviewed-by: Wu Bo <wubo40@huawei.com>

Re: [PATCH 00/10] iscsi fixes

[Martin K. Petersen]

On Thu, 7 Apr 2022 19:13:04 -0500, Mike Christie wrote:

> The following patchset was made over Linus's tree and contains fixes for
> boot/iscsid restart, more fixes for the in-kernel recovery patch, a data
> corruption regression and fix for qedi connection error handling.
> 

Applied to 5.18/scsi-fixes, thanks!

[01/10] scsi: iscsi: Move iscsi_ep_disconnect
        https://git.kernel.org/mkp/scsi/c/c34f95e98d8f
[02/10] scsi: iscsi: Fix offload conn cleanup when iscsid restarts
        https://git.kernel.org/mkp/scsi/c/cbd2283aaf47
[03/10] scsi: iscsi: Release endpoint ID when its freed.
        https://git.kernel.org/mkp/scsi/c/3c6ae371b8a1
[04/10] scsi: iscsi: Fix endpoint reuse regression
        https://git.kernel.org/mkp/scsi/c/0aadafb5c344
[05/10] scsi: iscsi: Fix conn cleanup and stop race during iscsid restart
        https://git.kernel.org/mkp/scsi/c/7c6e99c18167
[06/10] scsi: iscsi: Fix unbound endpoint error handling
        https://git.kernel.org/mkp/scsi/c/03690d819745
[07/10] scsi: iscsi: Merge suspend fields
        https://git.kernel.org/mkp/scsi/c/5bd856256f8c
[08/10] scsi: iscsi: Fix nop handling during conn recovery
        https://git.kernel.org/mkp/scsi/c/44ac97109e42
[09/10] scsi: qedi: Fix failed disconnect handling.
        https://git.kernel.org/mkp/scsi/c/857b06527f70
[10/10] scsi: iscsi: Add Mike Christie as co-maintainer
        https://git.kernel.org/mkp/scsi/c/70a3baeec4e8

-- 
Martin K. Petersen	Oracle Linux Engineering