[PATCH 0/6] iscsi fixes for 5.19 or 5.20

[PATCH 0/6] iscsi fixes for 5.19 or 5.20

[Mike Christie]

The following patches are some fixes for qla4xxx and qedi. They were built
over linus's tree, but apply over Martin's staging and queueing branches.
They do not have conflicts with the other iscsi patches that I've ackd on
the list, so they can be applied before or after those patches.

The first patch is trivial and fixes a bug that can only be triggered with
qla4xxx which should be rare. Patches 2 - 6 are more invassive and fix a
regression in qedi where shutdown hangs when you are using that driver for
iscsi boot. I was not sure if this was too much of an edge case and the
pathes were too invassive for 5.19 so the patches do apply over either
of your 5.19 or 5.20 branches.

[PATCH 1/6] scsi: iscsi: Fix HW conn removal use after free

[Mike Christie]

If qla4xxx doesn't remove the connection before the session, the iSCSI
class tries to remove the connection for it. We were doing a
iscsi_put_conn() in the iter function which is not needed and will result
in a use after free because iscsi_remove_conn() will free the connection.

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

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 2c0dd64159b0..e6084e158cc0 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
 		return 0;
 
 	iscsi_remove_conn(iscsi_dev_to_conn(dev));
-	iscsi_put_conn(iscsi_dev_to_conn(dev));
-
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/6] scsi: iscsi: Allow iscsi_if_stop_conn to be called from kernel

[Mike Christie]

iscsi_if_stop_conn is only called from the userspace interface but in the
next patch we will want to call it from the kernel interface to allow
drivers like qedi to remove sessions from inside the kernel during
shutdown. This removes the iscsi_uevent code from iscsi_if_stop_conn so we
can call it in a new helper in the next patch.

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

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index e6084e158cc0..0d83b6360b8a 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2257,16 +2257,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
 	}
 }
 
-static int iscsi_if_stop_conn(struct iscsi_transport *transport,
-			      struct iscsi_uevent *ev)
+static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 {
-	int flag = ev->u.stop_conn.flag;
-	struct iscsi_cls_conn *conn;
-
-	conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
-	if (!conn)
-		return -EINVAL;
-
 	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
 	/*
 	 * If this is a termination we have to call stop_conn with that flag
@@ -3713,7 +3705,12 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
 	case ISCSI_UEVENT_DESTROY_CONN:
 		return iscsi_if_destroy_conn(transport, ev);
 	case ISCSI_UEVENT_STOP_CONN:
-		return iscsi_if_stop_conn(transport, ev);
+		conn = iscsi_conn_lookup(ev->u.stop_conn.sid,
+					 ev->u.stop_conn.cid);
+		if (!conn)
+			return -EINVAL;
+
+		return iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
 	}
 
 	/*
-- 
2.25.1

[PATCH 3/6] scsi: iscsi: Cleanup bound endpoints during shutdown.

[Mike Christie]

In the next patch we allow drivers to drive session removal during
shutdown. In this case iscsid will not be running, so we need to detect
bound endpoints and disconnect them. This moves the bound ep check so we
now always check.

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

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 0d83b6360b8a..e4614dede7e9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2260,6 +2260,16 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
 static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 {
 	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
+	/*
+	 * For offload, iscsid may not know about the ep like when iscsid is
+	 * restarted or for kernel based session shutdown iscsid is not even
+	 * up. For these cases, we do the disconnect now.
+	 */
+	mutex_lock(&conn->ep_mutex);
+	if (conn->ep)
+		iscsi_if_disconnect_bound_ep(conn, conn->ep, true);
+	mutex_unlock(&conn->ep_mutex);
+
 	/*
 	 * If this is a termination we have to call stop_conn with that flag
 	 * so the correct states get set. If we haven't run the work yet try to
@@ -2269,16 +2279,6 @@ static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
 		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.
 		 */
-- 
2.25.1

[PATCH 4/6] scsi: iscsi: Add helper to remove a session from the kernel

[Mike Christie]

During qedi shutdown we need to stop the iSCSI layer from sending new nops
as pings and from responding to target ones and make sure there is no
running connection cleanups. Commit d1f2ce77638d ("scsi: qedi: Fix host
removal with running sessions") converted the driver to use the libicsi
helper to drive session removal, so the above issues could be handled. The
problem is that during system shutdown iscsid will not be running so when
we try to remove the root session we will hang waiting for userspace to
reply.

This patch adds a helper that will drive the destruction of sessions like
these during system shutdown.

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

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index e4614dede7e9..b67a4a938cd1 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2334,6 +2334,55 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
 	ISCSI_DBG_TRANS_CONN(conn, "cleanup done.\n");
 }
 
+static int iscsi_iter_force_destroy_conn_fn(struct device *dev, void *data)
+{
+	struct iscsi_transport *transport;
+	struct iscsi_cls_conn *conn;
+
+	if (!iscsi_is_conn_dev(dev))
+		return 0;
+
+	conn = iscsi_dev_to_conn(dev);
+	transport = conn->transport;
+
+	if (READ_ONCE(conn->state) != ISCSI_CONN_DOWN)
+		iscsi_if_stop_conn(conn, STOP_CONN_TERM);
+
+	transport->destroy_conn(conn);
+	return 0;
+}
+
+/**
+ * iscsi_force_destroy_session - destroy a session from the kernel
+ * @session: session to destroy
+ *
+ * Force the destruction of a session from the kernel. This should only be
+ * used when userspace is no longer running during system shutdown.
+ */
+void iscsi_force_destroy_session(struct iscsi_cls_session *session)
+{
+	struct iscsi_transport *transport = session->transport;
+	unsigned long flags;
+
+	WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
+
+	spin_lock_irqsave(&sesslock, flags);
+	if (list_empty(&session->sess_list)) {
+		spin_unlock_irqrestore(&sesslock, flags);
+		/*
+		 * Conn/ep is already freed. Session is being torn down via
+		 * async path. For shutdown we don't care about it so return.
+		 */
+		return;
+	}
+	spin_unlock_irqrestore(&sesslock, flags);
+
+	device_for_each_child(&session->dev, NULL,
+			      iscsi_iter_force_destroy_conn_fn);
+	transport->destroy_session(session);
+}
+EXPORT_SYMBOL_GPL(iscsi_force_destroy_session);
+
 void iscsi_free_session(struct iscsi_cls_session *session)
 {
 	ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 9acb8422f680..d6eab7cb221a 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -442,6 +442,7 @@ extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
 						struct iscsi_transport *t,
 						int dd_size,
 						unsigned int target_id);
+extern void iscsi_force_destroy_session(struct iscsi_cls_session *session);
 extern void iscsi_remove_session(struct iscsi_cls_session *session);
 extern void iscsi_free_session(struct iscsi_cls_session *session);
 extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
-- 
2.25.1

[PATCH 5/6] scsi: qedi: Use QEDI_MODE_NORMAL for error handling

[Mike Christie]

When handling errors that lead to host removal use QEDI_MODE_NORMAL. There
is currently no difference in behavior between NORMAL and SHUTDOWN, but in
a subsequent commit we will want to know when we are called from the
pci_driver shutdown callout vs remove/err_handler so we know when userspace
is up.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/qedi/qedi_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 83ffba7f51da..deebe62e2b41 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2491,7 +2491,7 @@ static void qedi_board_disable_work(struct work_struct *work)
 	if (test_and_set_bit(QEDI_IN_SHUTDOWN, &qedi->flags))
 		return;
 
-	__qedi_remove(qedi->pdev, QEDI_MODE_SHUTDOWN);
+	__qedi_remove(qedi->pdev, QEDI_MODE_NORMAL);
 }
 
 static void qedi_shutdown(struct pci_dev *pdev)
-- 
2.25.1

[PATCH 6/6] scsi: iscsi: Fix session removal on shutdown

[Mike Christie]

When the system is shutting down, iscsid is not running so we will not get
a response to the ISCSI_ERR_INVALID_HOST error event. The system shutdown
will then hang waiting on userspace to remove the session.

This has libiscsi force the destruction of the session from the kernel when
iscsi_host_remove() is called from a driver's shutdown callout.

This fixes a regression added in qedi boot with commit d1f2ce77638d ("scsi:
qedi: Fix host removal with running sessions") which made qedi use the
common session removal function that waits on userspace instead of rolling
its own kernel based removal.

Fixes: d1f2ce77638d ("scsi: qedi: Fix host removal with running sessions")
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 4 ++--
 drivers/scsi/be2iscsi/be_main.c          | 2 +-
 drivers/scsi/bnx2i/bnx2i_iscsi.c         | 2 +-
 drivers/scsi/cxgbi/libcxgbi.c            | 2 +-
 drivers/scsi/iscsi_tcp.c                 | 4 ++--
 drivers/scsi/libiscsi.c                  | 9 +++++++--
 drivers/scsi/qedi/qedi_main.c            | 9 ++++++---
 include/scsi/libiscsi.h                  | 2 +-
 8 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 321949a570ed..620ae5b2d80d 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -568,7 +568,7 @@ static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
 	struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
 
 	iscsi_session_teardown(cls_session);
-	iscsi_host_remove(shost);
+	iscsi_host_remove(shost, false);
 	iscsi_host_free(shost);
 }
 
@@ -685,7 +685,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	return cls_session;
 
 remove_host:
-	iscsi_host_remove(shost);
+	iscsi_host_remove(shost, false);
 free_host:
 	iscsi_host_free(shost);
 	return NULL;
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 3bb0adefbe06..02026476c39c 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -5745,7 +5745,7 @@ static void beiscsi_remove(struct pci_dev *pcidev)
 	cancel_work_sync(&phba->sess_work);
 
 	beiscsi_iface_destroy_default(phba);
-	iscsi_host_remove(phba->shost);
+	iscsi_host_remove(phba->shost, false);
 	beiscsi_disable_port(phba, 1);
 
 	/* after cancelling boot_work */
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 15fbd09baa94..a3c800e04a2e 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -909,7 +909,7 @@ void bnx2i_free_hba(struct bnx2i_hba *hba)
 {
 	struct Scsi_Host *shost = hba->shost;
 
-	iscsi_host_remove(shost);
+	iscsi_host_remove(shost, false);
 	INIT_LIST_HEAD(&hba->ep_ofld_list);
 	INIT_LIST_HEAD(&hba->ep_active_list);
 	INIT_LIST_HEAD(&hba->ep_destroy_list);
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 4365d52c6430..32abdf0fa9aa 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -328,7 +328,7 @@ void cxgbi_hbas_remove(struct cxgbi_device *cdev)
 		chba = cdev->hbas[i];
 		if (chba) {
 			cdev->hbas[i] = NULL;
-			iscsi_host_remove(chba->shost);
+			iscsi_host_remove(chba->shost, false);
 			pci_dev_put(cdev->pdev);
 			iscsi_host_free(chba->shost);
 		}
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9fee70d6434a..52c6f70d60ec 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -898,7 +898,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
 remove_session:
 	iscsi_session_teardown(cls_session);
 remove_host:
-	iscsi_host_remove(shost);
+	iscsi_host_remove(shost, false);
 free_host:
 	iscsi_host_free(shost);
 	return NULL;
@@ -915,7 +915,7 @@ static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session)
 	iscsi_tcp_r2tpool_free(cls_session->dd_data);
 	iscsi_session_teardown(cls_session);
 
-	iscsi_host_remove(shost);
+	iscsi_host_remove(shost, false);
 	iscsi_host_free(shost);
 }
 
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 797abf4f5399..3ddb701cd29c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2828,11 +2828,12 @@ static void iscsi_notify_host_removed(struct iscsi_cls_session *cls_session)
 /**
  * iscsi_host_remove - remove host and sessions
  * @shost: scsi host
+ * @is_shutdown: true if called from a driver shutdown callout
  *
  * If there are any sessions left, this will initiate the removal and wait
  * for the completion.
  */
-void iscsi_host_remove(struct Scsi_Host *shost)
+void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown)
 {
 	struct iscsi_host *ihost = shost_priv(shost);
 	unsigned long flags;
@@ -2841,7 +2842,11 @@ void iscsi_host_remove(struct Scsi_Host *shost)
 	ihost->state = ISCSI_HOST_REMOVED;
 	spin_unlock_irqrestore(&ihost->lock, flags);
 
-	iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
+	if (!is_shutdown)
+		iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
+	else
+		iscsi_host_for_each_session(shost, iscsi_force_destroy_session);
+
 	wait_event_interruptible(ihost->session_removal_wq,
 				 ihost->num_sessions == 0);
 	if (signal_pending(current))
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index deebe62e2b41..cecfb2cb4c7b 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2414,9 +2414,12 @@ static void __qedi_remove(struct pci_dev *pdev, int mode)
 	int rval;
 	u16 retry = 10;
 
-	if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
-		iscsi_host_remove(qedi->shost);
+	if (mode == QEDI_MODE_NORMAL)
+		iscsi_host_remove(qedi->shost, false);
+	else if (mode == QEDI_MODE_SHUTDOWN)
+		iscsi_host_remove(qedi->shost, true);
 
+	if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
 		if (qedi->tmf_thread) {
 			destroy_workqueue(qedi->tmf_thread);
 			qedi->tmf_thread = NULL;
@@ -2791,7 +2794,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
 #ifdef CONFIG_DEBUG_FS
 	qedi_dbg_host_exit(&qedi->dbg_ctx);
 #endif
-	iscsi_host_remove(qedi->shost);
+	iscsi_host_remove(qedi->shost, false);
 stop_iscsi_func:
 	qedi_ops->stop(qedi->cdev);
 stop_slowpath:
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index c0703cd20a99..9758a4a9923f 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -411,7 +411,7 @@ extern int iscsi_host_add(struct Scsi_Host *shost, struct device *pdev);
 extern struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
 					  int dd_data_size,
 					  bool xmit_can_sleep);
-extern void iscsi_host_remove(struct Scsi_Host *shost);
+extern void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown);
 extern void iscsi_host_free(struct Scsi_Host *shost);
 extern int iscsi_target_alloc(struct scsi_target *starget);
 extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
-- 
2.25.1

RE: [EXT] [PATCH 0/6] iscsi fixes for 5.19 or 5.20

[Nilesh Javali]

> -----Original Message-----
> From: Mike Christie <michael.christie@oracle.com>
> Sent: Friday, June 17, 2022 3:58 AM
> To: 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 0/6] iscsi fixes for 5.19 or 5.20
> 
> External Email
> 
> ----------------------------------------------------------------------
> The following patches are some fixes for qla4xxx and qedi. They were built
> over linus's tree, but apply over Martin's staging and queueing branches.
> They do not have conflicts with the other iscsi patches that I've ackd on
> the list, so they can be applied before or after those patches.
> 
> The first patch is trivial and fixes a bug that can only be triggered with
> qla4xxx which should be rare. Patches 2 - 6 are more invassive and fix a
> regression in qedi where shutdown hangs when you are using that driver for
> iscsi boot. I was not sure if this was too much of an edge case and the
> pathes were too invassive for 5.19 so the patches do apply over either
> of your 5.19 or 5.20 branches.
> 

Thanks for the patches.
The series looks good.

Reviewed-by: Nilesh Javali <njavali@marvell.com>
Tested-by: Nilesh Javali <njavali@marvell.com>

Re: [PATCH 0/6] iscsi fixes for 5.19 or 5.20

[Martin K. Petersen]

Mike,

> The following patches are some fixes for qla4xxx and qedi. They were built
> over linus's tree, but apply over Martin's staging and queueing branches.
> They do not have conflicts with the other iscsi patches that I've ackd on
> the list, so they can be applied before or after those patches.

Applied to 5.20/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/6] iscsi fixes for 5.19 or 5.20

[Martin K. Petersen]

On Thu, 16 Jun 2022 17:27:32 -0500, Mike Christie wrote:

> The following patches are some fixes for qla4xxx and qedi. They were built
> over linus's tree, but apply over Martin's staging and queueing branches.
> They do not have conflicts with the other iscsi patches that I've ackd on
> the list, so they can be applied before or after those patches.
> 
> The first patch is trivial and fixes a bug that can only be triggered with
> qla4xxx which should be rare. Patches 2 - 6 are more invassive and fix a
> regression in qedi where shutdown hangs when you are using that driver for
> iscsi boot. I was not sure if this was too much of an edge case and the
> pathes were too invassive for 5.19 so the patches do apply over either
> of your 5.19 or 5.20 branches.
> 
> [...]

Applied to 5.20/scsi-queue, thanks!

[1/6] scsi: iscsi: Fix HW conn removal use after free
      https://git.kernel.org/mkp/scsi/c/c577ab7ba5f3
[2/6] scsi: iscsi: Allow iscsi_if_stop_conn to be called from kernel
      https://git.kernel.org/mkp/scsi/c/3328333b47f4
[3/6] scsi: iscsi: Cleanup bound endpoints during shutdown.
      https://git.kernel.org/mkp/scsi/c/da2f132d00d9
[4/6] scsi: iscsi: Add helper to remove a session from the kernel
      https://git.kernel.org/mkp/scsi/c/bb42856bfd54
[5/6] scsi: qedi: Use QEDI_MODE_NORMAL for error handling
      https://git.kernel.org/mkp/scsi/c/7bf01eb0d4f9
[6/6] scsi: iscsi: Fix session removal on shutdown
      https://git.kernel.org/mkp/scsi/c/31500e902759

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/6] scsi: iscsi: Fix HW conn removal use after free

[Li Lingfeng]

Hi Mike,

Thanks for this patch addressing the UAF issue. I have some questions when
analyzing this patch.

You mention that iscsi_remove_conn() frees the connection, making the extra
iscsi_put_conn() cause UAF. However, looking at iscsi_remove_conn():

iscsi_remove_conn
  device_del(&conn->dev)
   put_device(parent) // Only parent gets put_device

This doesn't appear to free conn directly - only device_del() + parent
reference drop. Typically, conn is freed via put_device() on &conn->dev when
its refcount reaches zero.

Could you briefly clarify how iscsi_remove_conn() ultimately triggers the
freeing, and in what scenario the subsequent iscsi_put_conn() leads to UAF?

Thanks again for the fix.

Lingfeng

在 2022/6/17 6:27, Mike Christie 写道:
> If qla4xxx doesn't remove the connection before the session, the iSCSI
> class tries to remove the connection for it. We were doing a
> iscsi_put_conn() in the iter function which is not needed and will result
> in a use after free because iscsi_remove_conn() will free the connection.
>
> Reviewed-by: Lee Duncan <lduncan@suse.com>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_transport_iscsi.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 2c0dd64159b0..e6084e158cc0 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
>   		return 0;
>   
>   	iscsi_remove_conn(iscsi_dev_to_conn(dev));
> -	iscsi_put_conn(iscsi_dev_to_conn(dev));
> -
>   	return 0;
>   }
>   

Re: [PATCH 1/6] scsi: iscsi: Fix HW conn removal use after free

[Li Lingfeng]

Friendly ping...

Thanks

在 2025/6/19 20:57, Li Lingfeng 写道:
> Hi Mike,
>
> Thanks for this patch addressing the UAF issue. I have some questions 
> when
> analyzing this patch.
>
> You mention that iscsi_remove_conn() frees the connection, making the 
> extra
> iscsi_put_conn() cause UAF. However, looking at iscsi_remove_conn():
>
> iscsi_remove_conn
>  device_del(&conn->dev)
>   put_device(parent) // Only parent gets put_device
>
> This doesn't appear to free conn directly - only device_del() + parent
> reference drop. Typically, conn is freed via put_device() on 
> &conn->dev when
> its refcount reaches zero.
>
> Could you briefly clarify how iscsi_remove_conn() ultimately triggers the
> freeing, and in what scenario the subsequent iscsi_put_conn() leads to 
> UAF?
>
> Thanks again for the fix.
>
> Lingfeng
>
> 在 2022/6/17 6:27, Mike Christie 写道:
>> If qla4xxx doesn't remove the connection before the session, the iSCSI
>> class tries to remove the connection for it. We were doing a
>> iscsi_put_conn() in the iter function which is not needed and will 
>> result
>> in a use after free because iscsi_remove_conn() will free the 
>> connection.
>>
>> Reviewed-by: Lee Duncan <lduncan@suse.com>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>   drivers/scsi/scsi_transport_iscsi.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 2c0dd64159b0..e6084e158cc0 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct 
>> device *dev, void *data)
>>           return 0;
>>         iscsi_remove_conn(iscsi_dev_to_conn(dev));
>> -    iscsi_put_conn(iscsi_dev_to_conn(dev));
>> -
>>       return 0;
>>   }

Re: [PATCH 1/6] scsi: iscsi: Fix HW conn removal use after free

[Hou Tao]

Hi Mike & Lingfeng:

On 7/3/2025 9:35 AM, Li Lingfeng wrote:
> Friendly ping...
>
> Thanks
>
> 在 2025/6/19 20:57, Li Lingfeng 写道:
>> Hi Mike,
>>
>> Thanks for this patch addressing the UAF issue. I have some questions
>> when
>> analyzing this patch.
>>
>> You mention that iscsi_remove_conn() frees the connection, making the
>> extra
>> iscsi_put_conn() cause UAF. However, looking at iscsi_remove_conn():
>>
>> iscsi_remove_conn
>>  device_del(&conn->dev)
>>   put_device(parent) // Only parent gets put_device
>>
>> This doesn't appear to free conn directly - only device_del() + parent
>> reference drop. Typically, conn is freed via put_device() on
>> &conn->dev when
>> its refcount reaches zero.
>>
>> Could you briefly clarify how iscsi_remove_conn() ultimately triggers
>> the
>> freeing, and in what scenario the subsequent iscsi_put_conn() leads
>> to UAF?
>>
>> Thanks again for the fix.
>>
>> Lingfeng
>>
>> 在 2022/6/17 6:27, Mike Christie 写道:
>>> If qla4xxx doesn't remove the connection before the session, the iSCSI
>>> class tries to remove the connection for it. We were doing a
>>> iscsi_put_conn() in the iter function which is not needed and will
>>> result
>>> in a use after free because iscsi_remove_conn() will free the
>>> connection.
>>>
>>> Reviewed-by: Lee Duncan <lduncan@suse.com>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> ---
>>>   drivers/scsi/scsi_transport_iscsi.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c
>>> b/drivers/scsi/scsi_transport_iscsi.c
>>> index 2c0dd64159b0..e6084e158cc0 100644
>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>> @@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct
>>> device *dev, void *data)
>>>           return 0;
>>>         iscsi_remove_conn(iscsi_dev_to_conn(dev));
>>> -    iscsi_put_conn(iscsi_dev_to_conn(dev));
>>> -
>>>       return 0;
>>>   }
> .

I didn't follow the patch either. If I understand correctly, the
invocation of  iscsi_put_conn() in iscsi_iter_destory_conn_fn() is used
to free the initial reference counter of iscsi_cls_conn. For non-qla4xxx
cases, the ->destroy_conn() callback (e.g., iscsi_conn_teardown) will
call iscsi_remove_conn() and iscsi_put_conn() to remove the connection
from the children list of session and free the connection at last.
However for qla4xxx, it is not the case. The ->destroy_conn() callback
of qla4xxx will keep the connection in the session conn_list and doesn't
use iscsi_put_conn() to free the initial reference counter. Therefore,
it seems necessary to keep the iscsi_put_conn() in the
iscsi_iter_destroy_conn_fn(), otherwise, there will be memory leak problem.

For the use-after-free problem, I think it may be due to the concurrent
invocation of iscsi_add_conn() and iscsi_session_teardown().
iscsi_add_conn() has already invoked device_add(), but fails on
transport_register_device() and is trying to invoke device_del(). At the
same time iscsi_session_teardown() invokes iscsi_remove_session() and is
invoking iscsi_iter_destroy_conn_fn for the failed-to-add connection.
The connection will be put twice: one in iscsi_iter_destroy_conn_fn()
and another one in iscsi_conn_setup() and leads to use-after-free problem.

Re: [PATCH 1/6] scsi: iscsi: Fix HW conn removal use after free

[Li Lingfeng]

Hi all,

在 2025/7/3 17:40, Hou Tao 写道:
> Hi Mike & Lingfeng:
>
> On 7/3/2025 9:35 AM, Li Lingfeng wrote:
>> Friendly ping...
>>
>> Thanks
>>
>> 在 2025/6/19 20:57, Li Lingfeng 写道:
>>> Hi Mike,
>>>
>>> Thanks for this patch addressing the UAF issue. I have some questions
>>> when
>>> analyzing this patch.
>>>
>>> You mention that iscsi_remove_conn() frees the connection, making the
>>> extra
>>> iscsi_put_conn() cause UAF. However, looking at iscsi_remove_conn():
>>>
>>> iscsi_remove_conn
>>>   device_del(&conn->dev)
>>>    put_device(parent) // Only parent gets put_device
>>>
>>> This doesn't appear to free conn directly - only device_del() + parent
>>> reference drop. Typically, conn is freed via put_device() on
>>> &conn->dev when
>>> its refcount reaches zero.
>>>
>>> Could you briefly clarify how iscsi_remove_conn() ultimately triggers
>>> the
>>> freeing, and in what scenario the subsequent iscsi_put_conn() leads
>>> to UAF?
>>>
>>> Thanks again for the fix.
>>>
>>> Lingfeng
>>>
>>> 在 2022/6/17 6:27, Mike Christie 写道:
>>>> If qla4xxx doesn't remove the connection before the session, the iSCSI
>>>> class tries to remove the connection for it. We were doing a
>>>> iscsi_put_conn() in the iter function which is not needed and will
>>>> result
>>>> in a use after free because iscsi_remove_conn() will free the
>>>> connection.
>>>>
>>>> Reviewed-by: Lee Duncan <lduncan@suse.com>
>>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>>> ---
>>>>    drivers/scsi/scsi_transport_iscsi.c | 2 --
>>>>    1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c
>>>> b/drivers/scsi/scsi_transport_iscsi.c
>>>> index 2c0dd64159b0..e6084e158cc0 100644
>>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>>> @@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct
>>>> device *dev, void *data)
>>>>            return 0;
>>>>          iscsi_remove_conn(iscsi_dev_to_conn(dev));
>>>> -    iscsi_put_conn(iscsi_dev_to_conn(dev));
>>>> -
>>>>        return 0;
>>>>    }
>> .
> I didn't follow the patch either. If I understand correctly, the
> invocation of  iscsi_put_conn() in iscsi_iter_destory_conn_fn() is used
> to free the initial reference counter of iscsi_cls_conn. For non-qla4xxx
> cases, the ->destroy_conn() callback (e.g., iscsi_conn_teardown) will
> call iscsi_remove_conn() and iscsi_put_conn() to remove the connection
> from the children list of session and free the connection at last.
> However for qla4xxx, it is not the case. The ->destroy_conn() callback
> of qla4xxx will keep the connection in the session conn_list and doesn't
> use iscsi_put_conn() to free the initial reference counter. Therefore,
> it seems necessary to keep the iscsi_put_conn() in the
> iscsi_iter_destroy_conn_fn(), otherwise, there will be memory leak problem.
This patch indeed caused a memory leak. I reproduced the leak issue and
confirmed that reintroducing the removed iscsi_put_conn() prevents the
leak in the same scenario.


*kernel base:*
master 8c2e52ebbe885c7eeaabd3b7ddcdc1246fc400d2


*kernel diff:*
1) Since I don't have the device that supports qla4xxx, I forcibly
bypassed the connection check during session destruction;
2) I expanded the memory required by conn by 1024 times to more clearly
observe the memory changes.

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 7b4fe0e6afb2..bb354deeb668 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -940,8 +940,12 @@ static void iscsi_sw_tcp_session_destroy(struct 
iscsi_cls_session *cls_session)
         struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
         struct iscsi_session *session = cls_session->dd_data;

-       if (WARN_ON_ONCE(session->leadconn))
-               return;
+       if (WARN_ON_ONCE(session->leadconn)) {
+               printk("%s skip checking leadconn", __func__);
+       //      return;
+       }
+
+       printk("%s %d\n", __func__, __LINE__);

         iscsi_session_remove(cls_session);
         /*
diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index c75a806496d6..eac353ad536f 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2408,7 +2408,7 @@ iscsi_alloc_conn(struct iscsi_cls_session 
*session, int dd_size, uint32_t cid)
         struct iscsi_transport *transport = session->transport;
         struct iscsi_cls_conn *conn;

-       conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
+       conn = kzalloc((sizeof(*conn) + dd_size) * 1024, GFP_KERNEL);
         if (!conn)
                 return NULL;
         if (dd_size)
@@ -2984,6 +2984,7 @@ iscsi_if_destroy_conn(struct iscsi_transport 
*transport, struct iscsi_uevent *ev

         if (transport->destroy_conn)
                 transport->destroy_conn(conn);
+
         return 0;
  }

@@ -3937,13 +3938,20 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh, uint32_t *group)
                 iscsi_put_endpoint(ep);
                 break;
         case ISCSI_UEVENT_DESTROY_SESSION:
+               printk("%s %d skip checking conns\n", __func__, __LINE__);
                 session = iscsi_session_lookup(ev->u.d_session.sid);
-               if (!session)
+               if (!session) {
+                       printk("%s %d\n", __func__, __LINE__);
                         err = -EINVAL;
-               else if (iscsi_session_has_conns(ev->u.d_session.sid))
+/*
+               } else if (iscsi_session_has_conns(ev->u.d_session.sid)) {
+                       printk("%s %d\n", __func__, __LINE__);
                         err = -EBUSY;
-               else
+*/
+               } else {
+                       printk("%s %d\n", __func__, __LINE__);
                         transport->destroy_session(session);
+               }
                 break;
         case ISCSI_UEVENT_DESTROY_SESSION_ASYNC:
                 session = iscsi_session_lookup(ev->u.d_session.sid);


*open-iscsi base:*
master 5b49cf2a86b1bfce13082f8ddc90b2282feb563d


*open-iscsi diff:*
I forcibly bypassed the standard connection destruction procedure, so
that during the session destruction process, the connection would be
destroyed to trigger iscsi_iter_destroy_conn_fn().

diff --git a/usr/netlink.c b/usr/netlink.c
index 4bcaf8b..970d782 100644
--- a/usr/netlink.c
+++ b/usr/netlink.c
@@ -516,12 +516,15 @@ kcreate_conn(uint64_t transport_handle, uint32_t sid,
  static int
  kdestroy_conn(uint64_t transport_handle, uint32_t sid, uint32_t cid)
  {
-       int rc;
-       struct iscsi_uevent ev;
-       struct iovec iov[2];
-
+//     int rc;
+//     struct iscsi_uevent ev;
+//     struct iovec iov[2];
+       (void)transport_handle;
+       (void)sid;
+       (void)cid;
         log_debug(7, "in %s", __FUNCTION__);
-
+       printf("skip destroy conn\n");
+/*
         memset(&ev, 0, sizeof(struct iscsi_uevent));

         ev.type = ISCSI_UEVENT_DESTROY_CONN;
@@ -534,7 +537,7 @@ kdestroy_conn(uint64_t transport_handle, uint32_t 
sid, uint32_t cid)
         rc = __kipc_call(iov, 2);
         if (rc < 0)
                 return rc;
-
+*/
         return 0;
  }


*test script:*
#!/bin/bash
LOOP_TIMES=10000
LOG_FILE="iscsi_opt.log"
DEVICE_TYPE="VIRTUAL-DISK"

for ((count=1; count<=LOOP_TIMES; count++)); do
     iscsiadm -m node -l &>/dev/null &
     login_pid=$!
     login_start=$(date +%s)
     device_found=0

     while kill -0 $login_pid 2>/dev/null; do
         sleep 0.5
         elapsed=$(( $(date +%s) - login_start ))

         if [ $elapsed -ge 30 ]; then
             if lsscsi | grep -q "$DEVICE_TYPE"; then
                 kill $login_pid 2>/dev/null
                 status="DEVICE_READY (KILLED_TIMEOUT)"
                 device_found=1
                 break
             fi
         fi
     done

     if [ $device_found -eq 0 ]; then
         wait $login_pid
         status="ATTACHED"
     fi

     mem_login=$(grep MemAvailable /proc/meminfo | awk '{print $2}')
     printf "%-6d %-12d %-8s %s\n" $count $mem_login "LOGIN" "$status" | 
tee -a $LOG_FILE

     iscsiadm -m node -u &>/dev/null
     while iscsiadm -m session &>/dev/null; do sleep 0.1; done
     mem_logout=$(grep MemAvailable /proc/meminfo | awk '{print $2}')
     printf "%-6d %-12d %-8s %s\n" $count $mem_logout "LOGOUT" "SUCCESS" 
| tee -a $LOG_FILE
done


*result without iscsi_put_conn():*
1) After a certain number of cycles, the available memory significantly
decreases.
1      53738028     LOGIN    ATTACHED
1      53735616     LOGOUT   SUCCESS
...
100    52453148     LOGIN    ATTACHED
100    52453376     LOGOUT   SUCCESS
...
200    51122008     LOGIN    ATTACHED
200    51122644     LOGOUT   SUCCESS
...
2108   25984180     LOGIN    ATTACHED
2108   25995580     LOGOUT   SUCCESS

2) After removing iSCSI devices, stopping related services, and unloading
kernel modules, the available memory has not returned to the expected
level.
[root@fedora test_ko]# lsscsi
[0:0:0:0]    disk    QEMU     QEMU HARDDISK    2.5+  /dev/sda
[0:0:1:0]    disk    QEMU     QEMU HARDDISK    2.5+  /dev/sdb
[0:0:2:0]    disk    QEMU     QEMU HARDDISK    2.5+  /dev/sdc
[0:0:3:0]    disk    QEMU     QEMU HARDDISK    2.5+  /dev/sdd
[0:0:4:0]    disk    QEMU     QEMU HARDDISK    2.5+  /dev/sde
[root@fedora test_ko]# service iscsid stop
Redirecting to /bin/systemctl stop iscsid.service
Warning: Stopping iscsid.service, but it can still be activated by:
   iscsid.socket
[root@fedora test_ko]# service tgtd stop
Redirecting to /bin/systemctl stop tgtd.service
[root@fedora test_ko]# rmmod iscsi_tcp
[root@fedora test_ko]# rmmod iscsi_boot_sysfs
[root@fedora test_ko]# rmmod libiscsi_tcp
[root@fedora test_ko]# rmmod libiscsi
[root@fedora test_ko]# rmmod scsi_transport_iscsi
[root@fedora test_ko]# cat /proc/meminfo | grep MemAvailable
MemAvailable:   26063488 kB
[root@fedora test_ko]# cat /proc/meminfo | grep MemAvailable
MemAvailable:   26065000 kB
[root@fedora test_ko]#


*result with iscsi_put_conn():*
After a certain number of cycles, the available memory does not show
significant changes.
1      53823120     LOGIN    ATTACHED
1      53828420     LOGOUT   SUCCESS
...
100    53738576     LOGIN    ATTACHED
100    53747436     LOGOUT   SUCCESS
...
200    53723400     LOGIN    ATTACHED
200    53728696     LOGOUT   SUCCESS
...

I haven't found more details about the UAF issue described in this patch,
but I think the patch's description of the root cause 'iscsi_remove_conn()
will free the connection' is incorrect. Moreover, this fix approach is
inappropriate.
Until the original issue is fully understood, I recommend reverting this
patch.

Thanks,

Lingfeng


> For the use-after-free problem, I think it may be due to the concurrent
> invocation of iscsi_add_conn() and iscsi_session_teardown().
> iscsi_add_conn() has already invoked device_add(), but fails on
> transport_register_device() and is trying to invoke device_del(). At the
> same time iscsi_session_teardown() invokes iscsi_remove_session() and is
> invoking iscsi_iter_destroy_conn_fn for the failed-to-add connection.
> The connection will be put twice: one in iscsi_iter_destroy_conn_fn()
> and another one in iscsi_conn_setup() and leads to use-after-free problem.
>