[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

[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 <din...@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

[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 <din...@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

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

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

[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 <din...@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