[PATCH 1/3] scsi: libiscsi: move init ehwait to iscsi_session_setup()

[PATCH 1/3] scsi: libiscsi: move init ehwait to iscsi_session_setup()

[Ding Hui]

commit ec29d0ac29be ("scsi: iscsi: Fix conn use after free during
resets") move member ehwait from conn to session, but left init ehwait
in iscsi_conn_setup().

Due to one session can be binded by multi conns, the conn after the
first will reinit the session->ehwait, move init ehwait to
iscsi_session_setup() to fix it.

Fixes: ec29d0ac29be ("scsi: iscsi: Fix conn use after free during resets")
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/libiscsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 4683c183e9d4..712a45368385 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2947,6 +2947,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
 	session->tmf_state = TMF_INITIAL;
 	timer_setup(&session->tmf_timer, iscsi_tmf_timedout, 0);
 	mutex_init(&session->eh_mutex);
+	init_waitqueue_head(&session->ehwait);
 
 	spin_lock_init(&session->frwd_lock);
 	spin_lock_init(&session->back_lock);
@@ -3074,8 +3075,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
 		goto login_task_data_alloc_fail;
 	conn->login_task->data = conn->data = data;
 
-	init_waitqueue_head(&session->ehwait);
-
 	return cls_conn;
 
 login_task_data_alloc_fail:
-- 
2.17.1

[PATCH 2/3] scsi: libiscsi: fix invalid pointer dereference in iscsi_eh_session_reset

[Ding Hui]

like commit 5db6dd14b313 ("scsi: libiscsi: Fix NULL pointer dereference in
iscsi_eh_session_reset"), access conn->persistent_address here is not safe
too.

The persistent_address is independent of conn refcount, so maybe
already freed by iscsi_conn_teardown(), also we put the refcount of conn
above, the conn pointer may be invalid.

Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/libiscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 712a45368385..69b3b2148328 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2531,8 +2531,8 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
 	spin_lock_bh(&session->frwd_lock);
 	if (session->state == ISCSI_STATE_LOGGED_IN) {
 		ISCSI_DBG_EH(session,
-			     "session reset succeeded for %s,%s\n",
-			     session->targetname, conn->persistent_address);
+			     "session reset succeeded for %s\n",
+			     session->targetname);
 	} else
 		goto failed;
 	spin_unlock_bh(&session->frwd_lock);
-- 
2.17.1

[PATCH 3/3] scsi: libiscsi: get ref to conn in iscsi_eh_device/target_reset()

[Ding Hui]

like commit fda290c5ae98 ("scsi: iscsi: Get ref to conn during reset
handling"), because in iscsi_exec_task_mgmt_fn(), the eh_mutex and
frwd_lock will be unlock, the conn also can be released if we not
hold ref.

Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/libiscsi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 69b3b2148328..4d3b37c20f8a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2398,7 +2398,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 {
 	struct iscsi_cls_session *cls_session;
 	struct iscsi_session *session;
-	struct iscsi_conn *conn;
+	struct iscsi_conn *conn = NULL;
 	struct iscsi_tm *hdr;
 	int rc = FAILED;
 
@@ -2417,6 +2417,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
 		goto unlock;
 	conn = session->leadconn;
+	iscsi_get_conn(conn->cls_conn);
 
 	/* only have one tmf outstanding at a time */
 	if (session->tmf_state != TMF_INITIAL)
@@ -2463,6 +2464,8 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 done:
 	ISCSI_DBG_EH(session, "dev reset result = %s\n",
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
+	if (conn)
+		iscsi_put_conn(conn->cls_conn);
 	mutex_unlock(&session->eh_mutex);
 	return rc;
 }
@@ -2560,7 +2563,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 {
 	struct iscsi_cls_session *cls_session;
 	struct iscsi_session *session;
-	struct iscsi_conn *conn;
+	struct iscsi_conn *conn = NULL;
 	struct iscsi_tm *hdr;
 	int rc = FAILED;
 
@@ -2579,6 +2582,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
 		goto unlock;
 	conn = session->leadconn;
+	iscsi_get_conn(conn->cls_conn);
 
 	/* only have one tmf outstanding at a time */
 	if (session->tmf_state != TMF_INITIAL)
@@ -2625,6 +2629,8 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 done:
 	ISCSI_DBG_EH(session, "tgt %s reset result = %s\n", session->targetname,
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
+	if (conn)
+		iscsi_put_conn(conn->cls_conn);
 	mutex_unlock(&session->eh_mutex);
 	return rc;
 }
-- 
2.17.1

[RESEND] [PATCH 0/3] fix several bugs about libiscsi

[Ding Hui]

Ding Hui (3):
  scsi: libiscsi: move init ehwait to iscsi_session_setup()
  scsi: libiscsi: fix invalid pointer dereference in
    iscsi_eh_session_reset
  scsi: libiscsi: get ref to conn in iscsi_eh_device/target_reset()

 drivers/scsi/libiscsi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 3/3] scsi: libiscsi: get ref to conn in iscsi_eh_device/target_reset()

[Ding Hui]

like commit fda290c5ae98 ("scsi: iscsi: Get ref to conn during reset
handling"), because in iscsi_exec_task_mgmt_fn(), the eh_mutex and
frwd_lock will be unlock, the conn also can be released if we not
hold ref.

Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/libiscsi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 69b3b2148328..4d3b37c20f8a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2398,7 +2398,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 {
 	struct iscsi_cls_session *cls_session;
 	struct iscsi_session *session;
-	struct iscsi_conn *conn;
+	struct iscsi_conn *conn = NULL;
 	struct iscsi_tm *hdr;
 	int rc = FAILED;
 
@@ -2417,6 +2417,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
 		goto unlock;
 	conn = session->leadconn;
+	iscsi_get_conn(conn->cls_conn);
 
 	/* only have one tmf outstanding at a time */
 	if (session->tmf_state != TMF_INITIAL)
@@ -2463,6 +2464,8 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
 done:
 	ISCSI_DBG_EH(session, "dev reset result = %s\n",
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
+	if (conn)
+		iscsi_put_conn(conn->cls_conn);
 	mutex_unlock(&session->eh_mutex);
 	return rc;
 }
@@ -2560,7 +2563,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 {
 	struct iscsi_cls_session *cls_session;
 	struct iscsi_session *session;
-	struct iscsi_conn *conn;
+	struct iscsi_conn *conn = NULL;
 	struct iscsi_tm *hdr;
 	int rc = FAILED;
 
@@ -2579,6 +2582,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
 		goto unlock;
 	conn = session->leadconn;
+	iscsi_get_conn(conn->cls_conn);
 
 	/* only have one tmf outstanding at a time */
 	if (session->tmf_state != TMF_INITIAL)
@@ -2625,6 +2629,8 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
 done:
 	ISCSI_DBG_EH(session, "tgt %s reset result = %s\n", session->targetname,
 		     rc == SUCCESS ? "SUCCESS" : "FAILED");
+	if (conn)
+		iscsi_put_conn(conn->cls_conn);
 	mutex_unlock(&session->eh_mutex);
 	return rc;
 }
-- 
2.17.1

Re: [PATCH 3/3] scsi: libiscsi: get ref to conn in iscsi_eh_device/target_reset()

[Mike Christie]

On 9/9/21 8:02 PM, Ding Hui wrote:
> like commit fda290c5ae98 ("scsi: iscsi: Get ref to conn during reset
> handling"), because in iscsi_exec_task_mgmt_fn(), the eh_mutex and
> frwd_lock will be unlock, the conn also can be released if we not
> hold ref.
> 

This should not happen because we only access the conn if the session state
is ISCSI_STATE_LOGGED_IN. So on entry of the iscsi_eh_* functions we grab the
lock and mutex and check the session state. If logged in then we access the conn.
We then drop the lock/mutex to do the TMF/IO and do a wait. When we wake up, we
retake the lock/mutex and then check the state again.

But yeah, the code sucks (it's my fault) in that its half refcount half locks
and state checks like that. Ideally we should fully convert to the refcounts
and move the teardown/freeing to the release function. I was going to do this
in a patchset to fix up the EH/TMF code after I get my lock removal patches
merged.



> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> ---
>  drivers/scsi/libiscsi.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 69b3b2148328..4d3b37c20f8a 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2398,7 +2398,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  {
>  	struct iscsi_cls_session *cls_session;
>  	struct iscsi_session *session;
> -	struct iscsi_conn *conn;
> +	struct iscsi_conn *conn = NULL;
>  	struct iscsi_tm *hdr;
>  	int rc = FAILED;
>  
> @@ -2417,6 +2417,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
>  		goto unlock;
>  	conn = session->leadconn;
> +	iscsi_get_conn(conn->cls_conn);
>  
>  	/* only have one tmf outstanding at a time */
>  	if (session->tmf_state != TMF_INITIAL)
> @@ -2463,6 +2464,8 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
>  done:
>  	ISCSI_DBG_EH(session, "dev reset result = %s\n",
>  		     rc == SUCCESS ? "SUCCESS" : "FAILED");
> +	if (conn)
> +		iscsi_put_conn(conn->cls_conn);
>  	mutex_unlock(&session->eh_mutex);
>  	return rc;
>  }
> @@ -2560,7 +2563,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  {
>  	struct iscsi_cls_session *cls_session;
>  	struct iscsi_session *session;
> -	struct iscsi_conn *conn;
> +	struct iscsi_conn *conn = NULL;
>  	struct iscsi_tm *hdr;
>  	int rc = FAILED;
>  
> @@ -2579,6 +2582,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  	if (!session->leadconn || session->state != ISCSI_STATE_LOGGED_IN)
>  		goto unlock;
>  	conn = session->leadconn;
> +	iscsi_get_conn(conn->cls_conn);
>  
>  	/* only have one tmf outstanding at a time */
>  	if (session->tmf_state != TMF_INITIAL)
> @@ -2625,6 +2629,8 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
>  done:
>  	ISCSI_DBG_EH(session, "tgt %s reset result = %s\n", session->targetname,
>  		     rc == SUCCESS ? "SUCCESS" : "FAILED");
> +	if (conn)
> +		iscsi_put_conn(conn->cls_conn);
>  	mutex_unlock(&session->eh_mutex);
>  	return rc;
>  }
>