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

[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 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

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

[Mike Christie]

On 9/9/21 8:02 PM, Ding Hui wrote:
> 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

A session can only have 1 conn. There is some code that makes it look
like we can do multiple conns or swap the single conn, but it was never
fully implemented/supported upstream.

However, I like the patch. The init should be done in iscsi_session_setup,
so could you fix up the commit, so it's correct?

> 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:
> 

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

[Ding Hui]

On 2021/9/11 12:25 上午, Mike Christie wrote:
> On 9/9/21 8:02 PM, Ding Hui wrote:
>> 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
> 
> A session can only have 1 conn. There is some code that makes it look
> like we can do multiple conns or swap the single conn, but it was never
> fully implemented/supported upstream.
> 
> However, I like the patch. The init should be done in iscsi_session_setup,
> so could you fix up the commit, so it's correct?
> 

Thanks，I'll update the commit log and send v2 1/3.

>> 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:
>>
> 


-- 
Thanks,
-dinghui

[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

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

[Mike Christie]

On 9/9/21 8:02 PM, Ding Hui wrote:
> 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.

This shouldn't happen like you describe above, because when we wake up
we will see the session->state is ISCSI_STATE_TERMINATE. We will then
not reference the conn in that code below.

However, it looks like we could hit an issue where if a user was resetting
the persistent_address or targetname via iscsi_set_param -> iscsi_switch_str_param
then we could be accessing freed memory. I think we need the frwd_lock when swapping
the strings in iscsi_switch_str_param.


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

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

[Ding Hui]

On 2021/9/11 12:38 上午, Mike Christie wrote:
> On 9/9/21 8:02 PM, Ding Hui wrote:
>> 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.
> 
> This shouldn't happen like you describe above, because when we wake up
> we will see the session->state is ISCSI_STATE_TERMINATE. We will then
> not reference the conn in that code below.
> 
> However, it looks like we could hit an issue where if a user was resetting
> the persistent_address or targetname via iscsi_set_param -> iscsi_switch_str_param
> then we could be accessing freed memory. I think we need the frwd_lock when swapping
> the strings in iscsi_switch_str_param.
> 

Thanks for your detailed explanation, I'll drop 2/3 and 3/3 in v2 patch.
I expect that the persistent_address issue be fixed in your future patchset.

Sorry for my ignorance.

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


-- 
Thanks,
-dinghui

[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;
>  }
>