diff mbox series

[1/1] scsi: qedi: Fix host removal with running sessions

Message ID 20210609192709.5094-1-michael.christie@oracle.com
State New
Headers show
Series [1/1] scsi: qedi: Fix host removal with running sessions | expand

Commit Message

Mike Christie June 9, 2021, 7:27 p.m. UTC 
qedi_clear_session_ctx could race with the in-kernel or userspace driven
recovery/removal and we could access a NULL conn or do a double free.

We should be using iscsi_host_remove to start the removal process from the
driver. It will start the in-kernel recovery and notify userspace that the
driver's scsi_hosts are being removed. iscsid will then drive the session
removal like is done when the logout command is run. When the sessions are
removed, iscsi_host_remove will return so qedi can finish knowing there
are no running sessions and no new sessions will be allowed.

This also fixes an issue where we check for a NULL conn after already
accessing it introduced in commit 27e986289e73 ("scsi: iscsi: Drop suspend
calls from ep_disconnect") by just removing the function completely.

Fixes: 27e986289e73 ("scsi: iscsi: Drop suspend calls from ep_disconnect")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/qedi/qedi_gbl.h   |  1 -
 drivers/scsi/qedi/qedi_iscsi.c | 17 -----------------
 drivers/scsi/qedi/qedi_main.c  |  7 ++-----
 3 files changed, 2 insertions(+), 23 deletions(-)

Comments

Martin K. Petersen June 10, 2021, 3:07 a.m. UTC | #1 
Mike,

> qedi_clear_session_ctx could race with the in-kernel or userspace

> driven recovery/removal and we could access a NULL conn or do a double

> free.


Applied to 5.14/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering
Manish Rangankar June 10, 2021, 4:44 a.m. UTC | #2 
> -----Original Message-----

> From: Mike Christie <michael.christie@oracle.com>

> Sent: Thursday, June 10, 2021 12:57 AM

> To: Manish Rangankar <mrangankar@marvell.com>;

> martin.petersen@oracle.com; linux-scsi@vger.kernel.org

> Cc: Mike Christie <michael.christie@oracle.com>

> Subject: [EXT] [PATCH 1/1] scsi: qedi: Fix host removal with running sessions

> 

> External Email

> 

> ----------------------------------------------------------------------

> qedi_clear_session_ctx could race with the in-kernel or userspace driven

> recovery/removal and we could access a NULL conn or do a double free.

> 

> We should be using iscsi_host_remove to start the removal process from the

> driver. It will start the in-kernel recovery and notify userspace that the driver's

> scsi_hosts are being removed. iscsid will then drive the session removal like is

> done when the logout command is run. When the sessions are removed,

> iscsi_host_remove will return so qedi can finish knowing there are no running

> sessions and no new sessions will be allowed.

> 

> This also fixes an issue where we check for a NULL conn after already accessing

> it introduced in commit 27e986289e73 ("scsi: iscsi: Drop suspend calls from

> ep_disconnect") by just removing the function completely.

> 

> Fixes: 27e986289e73 ("scsi: iscsi: Drop suspend calls from ep_disconnect")

> Signed-off-by: Mike Christie <michael.christie@oracle.com>

> ---

>  drivers/scsi/qedi/qedi_gbl.h   |  1 -

>  drivers/scsi/qedi/qedi_iscsi.c | 17 -----------------  drivers/scsi/qedi/qedi_main.c

> |  7 ++-----

>  3 files changed, 2 insertions(+), 23 deletions(-)

> 

> diff --git a/drivers/scsi/qedi/qedi_gbl.h b/drivers/scsi/qedi/qedi_gbl.h index

> fb44a282613e..9f8e8ef405a1 100644

> --- a/drivers/scsi/qedi/qedi_gbl.h

> +++ b/drivers/scsi/qedi/qedi_gbl.h

> @@ -72,6 +72,5 @@ void qedi_remove_sysfs_ctx_attr(struct qedi_ctx *qedi);

> void qedi_clearsq(struct qedi_ctx *qedi,

>  		  struct qedi_conn *qedi_conn,

>  		  struct iscsi_task *task);

> -void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess);

> 

>  #endif

> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c index

> bf581ecea897..97f83760da88 100644

> --- a/drivers/scsi/qedi/qedi_iscsi.c

> +++ b/drivers/scsi/qedi/qedi_iscsi.c

> @@ -1659,23 +1659,6 @@ void qedi_process_iscsi_error(struct qedi_endpoint

> *ep,

>  		qedi_start_conn_recovery(qedi_conn->qedi, qedi_conn);  }

> 

> -void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess) -{

> -	struct iscsi_session *session = cls_sess->dd_data;

> -	struct iscsi_conn *conn = session->leadconn;

> -	struct qedi_conn *qedi_conn = conn->dd_data;

> -

> -	if (iscsi_is_session_online(cls_sess)) {

> -		if (conn)

> -			iscsi_suspend_queue(conn);

> -		qedi_ep_disconnect(qedi_conn->iscsi_ep);

> -	}

> -

> -	qedi_conn_destroy(qedi_conn->cls_conn);

> -

> -	qedi_session_destroy(cls_sess);

> -}

> -

>  void qedi_process_tcp_error(struct qedi_endpoint *ep,

>  			    struct iscsi_eqe_data *data)

>  {

> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c index

> edf915432704..0b0acb827071 100644

> --- a/drivers/scsi/qedi/qedi_main.c

> +++ b/drivers/scsi/qedi/qedi_main.c

> @@ -2417,11 +2417,9 @@ static void __qedi_remove(struct pci_dev *pdev, int

> mode)

>  	int rval;

>  	u16 retry = 10;

> 

> -	if (mode == QEDI_MODE_SHUTDOWN)

> -		iscsi_host_for_each_session(qedi->shost,

> -					    qedi_clear_session_ctx);

> -

>  	if (mode == QEDI_MODE_NORMAL || mode ==

> QEDI_MODE_SHUTDOWN) {

> +		iscsi_host_remove(qedi->shost);

> +

>  		if (qedi->tmf_thread) {

>  			flush_workqueue(qedi->tmf_thread);

>  			destroy_workqueue(qedi->tmf_thread);

> @@ -2482,7 +2480,6 @@ static void __qedi_remove(struct pci_dev *pdev, int

> mode)

>  		if (qedi->boot_kset)

>  			iscsi_boot_destroy_kset(qedi->boot_kset);

> 

> -		iscsi_host_remove(qedi->shost);

>  		iscsi_host_free(qedi->shost);

>  	}

>  }

> --

> 2.25.1


Thanks,
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Martin K. Petersen June 16, 2021, 3:49 a.m. UTC | #3 
On Wed, 9 Jun 2021 14:27:09 -0500, Mike Christie wrote:

> qedi_clear_session_ctx could race with the in-kernel or userspace driven

> recovery/removal and we could access a NULL conn or do a double free.

> 

> We should be using iscsi_host_remove to start the removal process from the

> driver. It will start the in-kernel recovery and notify userspace that the

> driver's scsi_hosts are being removed. iscsid will then drive the session

> removal like is done when the logout command is run. When the sessions are

> removed, iscsi_host_remove will return so qedi can finish knowing there

> are no running sessions and no new sessions will be allowed.

> 

> [...]


Applied to 5.14/scsi-queue, thanks!

[1/1] scsi: qedi: Fix host removal with running sessions
      https://git.kernel.org/mkp/scsi/c/d1f2ce77638d

-- 
Martin K. Petersen	Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/qedi/qedi_gbl.h b/drivers/scsi/qedi/qedi_gbl.h
index fb44a282613e..9f8e8ef405a1 100644
--- a/drivers/scsi/qedi/qedi_gbl.h
+++ b/drivers/scsi/qedi/qedi_gbl.h
@@ -72,6 +72,5 @@  void qedi_remove_sysfs_ctx_attr(struct qedi_ctx *qedi);
 void qedi_clearsq(struct qedi_ctx *qedi,
 		  struct qedi_conn *qedi_conn,
 		  struct iscsi_task *task);
-void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess);
 
 #endif
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index bf581ecea897..97f83760da88 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -1659,23 +1659,6 @@  void qedi_process_iscsi_error(struct qedi_endpoint *ep,
 		qedi_start_conn_recovery(qedi_conn->qedi, qedi_conn);
 }
 
-void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess)
-{
-	struct iscsi_session *session = cls_sess->dd_data;
-	struct iscsi_conn *conn = session->leadconn;
-	struct qedi_conn *qedi_conn = conn->dd_data;
-
-	if (iscsi_is_session_online(cls_sess)) {
-		if (conn)
-			iscsi_suspend_queue(conn);
-		qedi_ep_disconnect(qedi_conn->iscsi_ep);
-	}
-
-	qedi_conn_destroy(qedi_conn->cls_conn);
-
-	qedi_session_destroy(cls_sess);
-}
-
 void qedi_process_tcp_error(struct qedi_endpoint *ep,
 			    struct iscsi_eqe_data *data)
 {
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index edf915432704..0b0acb827071 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2417,11 +2417,9 @@  static void __qedi_remove(struct pci_dev *pdev, int mode)
 	int rval;
 	u16 retry = 10;
 
-	if (mode == QEDI_MODE_SHUTDOWN)
-		iscsi_host_for_each_session(qedi->shost,
-					    qedi_clear_session_ctx);
-
 	if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
+		iscsi_host_remove(qedi->shost);
+
 		if (qedi->tmf_thread) {
 			flush_workqueue(qedi->tmf_thread);
 			destroy_workqueue(qedi->tmf_thread);
@@ -2482,7 +2480,6 @@  static void __qedi_remove(struct pci_dev *pdev, int mode)
 		if (qedi->boot_kset)
 			iscsi_boot_destroy_kset(qedi->boot_kset);
 
-		iscsi_host_remove(qedi->shost);
 		iscsi_host_free(qedi->shost);
 	}
 }