Re: [PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

Re: [PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 10/21/22 6:57 PM, Wenchao Hao wrote:
> +
>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>  {
>  	int err;
> @@ -1899,6 +1922,7 @@ static void __iscsi_unblock_session(struct work_struct *work)
>  	cancel_delayed_work_sync(&session->recovery_work);
>  	spin_lock_irqsave(&session->lock, flags);
>  	session->state = ISCSI_SESSION_LOGGED_IN;
> +	session->target_state = ISCSI_SESSION_TARGET_BOUND;
>  	spin_unlock_irqrestore(&session->lock, flags);
>  	/* start IO */

Hey,

Sorry for the late reply.

For the initial login we have not scanned the session above, so there
is no target yet. If iscsid is restarted at this time, then iscsid wants
to sync the session and also do the initial scan.

To handle that case and also better match the state names with the
session's target state we can:

1. Move the above line to iscsi_user_scan_session after we have scanned
the target.
2. Add a new state ISCSI_SESSION_TARGET_ALLOCATED to reflect we have
allocated the target_id, but not yet scanned.

[PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Wenchao Hao]

I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
for multiple times which should be fixed.

This patch introduce target_state in iscsi_cls_session to make
sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.

But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
Report unbind session event when the target has been removed"). The issue
is iscsid died for any reason after it send unbind session to kernel, once
iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.

Now kernel think iscsi_cls_session has already sent an
ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
would cause userspace unable to logout. Actually the session is in
invalid state(it's target_id is INVALID), iscsid should not sync this
session in it's restart.

So we need to check session's target state during iscsid restart,
if session is in unbound state, do not sync this session and perform
session teardown. It's reasonable because once a session is unbound, we
can not recover it any more(mainly because it's target id is INVALID)

V4:
- Move debug print out of spinlock critical section

V3:
- Make target bind state to a state kind rather than a bool.

V2:
- Using target_unbound rather than state to indicate session has been
  unbound

Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 49 +++++++++++++++++++++++++++++
 include/scsi/scsi_transport_iscsi.h |  7 +++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..1ed279831a78 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,29 @@ static const char *iscsi_session_state_name(int state)
 	return name;
 }
 
+static struct {
+	int value;
+	char *name;
+} iscsi_session_target_state_names[] = {
+	{ ISCSI_SESSION_TARGET_UNBOUND,		"UNBOUND" },
+	{ ISCSI_SESSION_TARGET_BOUND,		"BOUND" },
+	{ ISCSI_SESSION_TARGET_UNBINDING,	"UNBINDING" },
+};
+
+static const char *iscsi_session_target_state_name(int state)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(iscsi_session_target_state_names); i++) {
+		if (iscsi_session_target_state_names[i].value == state) {
+			name = iscsi_session_target_state_names[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
 	int err;
@@ -1899,6 +1922,7 @@ static void __iscsi_unblock_session(struct work_struct *work)
 	cancel_delayed_work_sync(&session->recovery_work);
 	spin_lock_irqsave(&session->lock, flags);
 	session->state = ISCSI_SESSION_LOGGED_IN;
+	session->target_state = ISCSI_SESSION_TARGET_BOUND;
 	spin_unlock_irqrestore(&session->lock, flags);
 	/* start IO */
 	scsi_target_unblock(&session->dev, SDEV_RUNNING);
@@ -1961,6 +1985,15 @@ static void __iscsi_unbind_session(struct work_struct *work)
 	unsigned long flags;
 	unsigned int target_id;
 
+	spin_lock_irqsave(&session->lock, flags);
+	if (session->target_state != ISCSI_SESSION_TARGET_BOUND) {
+		spin_unlock_irqrestore(&session->lock, flags);
+		ISCSI_DBG_TRANS_SESSION(session, "Abort unbind sesison\n");
+		return;
+	}
+	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
+	spin_unlock_irqrestore(&session->lock, flags);
+
 	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
 	/* Prevent new scans and make sure scanning is not in progress */
@@ -1984,6 +2017,9 @@ static void __iscsi_unbind_session(struct work_struct *work)
 
 unbind_session_exit:
 	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
+	spin_lock_irqsave(&session->lock, flags);
+	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+	spin_unlock_irqrestore(&session->lock, flags);
 	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
 }
 
@@ -4368,6 +4404,16 @@ iscsi_session_attr(def_taskmgmt_tmo, ISCSI_PARAM_DEF_TASKMGMT_TMO, 0);
 iscsi_session_attr(discovery_parent_idx, ISCSI_PARAM_DISCOVERY_PARENT_IDX, 0);
 iscsi_session_attr(discovery_parent_type, ISCSI_PARAM_DISCOVERY_PARENT_TYPE, 0);
 
+static ssize_t
+show_priv_session_target_state(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct iscsi_cls_session *session = iscsi_dev_to_session(dev->parent);
+	return sysfs_emit(buf, "%s\n",
+			iscsi_session_target_state_name(session->target_state));
+}
+static ISCSI_CLASS_ATTR(priv_sess, target_state, S_IRUGO,
+			show_priv_session_target_state, NULL);
 static ssize_t
 show_priv_session_state(struct device *dev, struct device_attribute *attr,
 			char *buf)
@@ -4470,6 +4516,7 @@ static struct attribute *iscsi_session_attrs[] = {
 	&dev_attr_sess_boot_target.attr,
 	&dev_attr_priv_sess_recovery_tmo.attr,
 	&dev_attr_priv_sess_state.attr,
+	&dev_attr_priv_sess_target_state.attr,
 	&dev_attr_priv_sess_creator.attr,
 	&dev_attr_sess_chap_out_idx.attr,
 	&dev_attr_sess_chap_in_idx.attr,
@@ -4583,6 +4630,8 @@ static umode_t iscsi_session_attr_is_visible(struct kobject *kobj,
 		return S_IRUGO | S_IWUSR;
 	else if (attr == &dev_attr_priv_sess_state.attr)
 		return S_IRUGO;
+	else if (attr == &dev_attr_priv_sess_target_state.attr)
+		return S_IRUGO;
 	else if (attr == &dev_attr_priv_sess_creator.attr)
 		return S_IRUGO;
 	else if (attr == &dev_attr_priv_sess_target_id.attr)
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index cab52b0f11d0..d59e31ebbfdf 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -236,6 +236,12 @@ enum {
 	ISCSI_SESSION_FREE,
 };
 
+enum {
+	ISCSI_SESSION_TARGET_UNBOUND,
+	ISCSI_SESSION_TARGET_BOUND,
+	ISCSI_SESSION_TARGET_UNBINDING,
+};
+
 #define ISCSI_MAX_TARGET -1
 
 struct iscsi_cls_session {
@@ -264,6 +270,7 @@ struct iscsi_cls_session {
 	 */
 	pid_t creator;
 	int state;
+	int target_state;			/* session target bind state */
 	int sid;				/* session id */
 	void *dd_data;				/* LLD private data */
 	struct device dev;	/* sysfs transport/container device */
-- 
2.35.3

Re: [PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Wenchao Hao]

On 2022/10/22 1:24, Mike Christie wrote:
> On 10/21/22 6:57 PM, Wenchao Hao wrote:
>> +
>>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>>  {
>>  	int err;
>> @@ -1899,6 +1922,7 @@ static void __iscsi_unblock_session(struct work_struct *work)
>>  	cancel_delayed_work_sync(&session->recovery_work);
>>  	spin_lock_irqsave(&session->lock, flags);
>>  	session->state = ISCSI_SESSION_LOGGED_IN;
>> +	session->target_state = ISCSI_SESSION_TARGET_BOUND;
>>  	spin_unlock_irqrestore(&session->lock, flags);
>>  	/* start IO */
> 
> Hey,
> 
> Sorry for the late reply.
> 

It doesn't matter.

> For the initial login we have not scanned the session above, so there
> is no target yet. If iscsid is restarted at this time, then iscsid wants
> to sync the session and also do the initial scan.
> 
> To handle that case and also better match the state names with the
> session's target state we can:
> 
> 1. Move the above line to iscsi_user_scan_session after we have scanned
> the target.
> 2. Add a new state ISCSI_SESSION_TARGET_ALLOCATED to reflect we have
> allocated the target_id, but not yet scanned.
> .

I have some wonder about the target_id like be2iscsi which allocated from 
iscsi_sess_ida. Should not we get the target_id from iSCSI target?
If they allocate target_id with an random value, how to handle the
iscsi_user_scan_session which would check the session's target_id.

I have no environment which deployed these iSCSI drivers like be2iscsi, so I
can not valid my guess.

Re: [PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Mike Christie]

On 10/22/22 4:40 AM, Wenchao Hao wrote:
> 
> On 2022/10/22 1:24, Mike Christie wrote:
>> On 10/21/22 6:57 PM, Wenchao Hao wrote:
>>> +
>>>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>>>  {
>>>  	int err;
>>> @@ -1899,6 +1922,7 @@ static void __iscsi_unblock_session(struct work_struct *work)
>>>  	cancel_delayed_work_sync(&session->recovery_work);
>>>  	spin_lock_irqsave(&session->lock, flags);
>>>  	session->state = ISCSI_SESSION_LOGGED_IN;
>>> +	session->target_state = ISCSI_SESSION_TARGET_BOUND;
>>>  	spin_unlock_irqrestore(&session->lock, flags);
>>>  	/* start IO */
>>
>> Hey,
>>
>> Sorry for the late reply.
>>
> 
> It doesn't matter.
> 
>> For the initial login we have not scanned the session above, so there
>> is no target yet. If iscsid is restarted at this time, then iscsid wants
>> to sync the session and also do the initial scan.
>>
>> To handle that case and also better match the state names with the
>> session's target state we can:
>>
>> 1. Move the above line to iscsi_user_scan_session after we have scanned
>> the target.
>> 2. Add a new state ISCSI_SESSION_TARGET_ALLOCATED to reflect we have
>> allocated the target_id, but not yet scanned.
>> .
> 
> I have some wonder about the target_id like be2iscsi which allocated from 
> iscsi_sess_ida. Should not we get the target_id from iSCSI target?
> If they allocate target_id with an random value, how to handle the
> iscsi_user_scan_session which would check the session's target_id.
> 

For iscsi, that target id is only a number that's used on the initiator side
to track the target. The target has no idea what it is and it's never
sent/used/passed to the target.

For example, the qla4xxx driver uses it to lookup persistent target info it
has stored on it's flash. The other use is that we need a unique name for
the target in sysfs and that target id is used as part of that name.

Re: [PATCH v4] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Wenchao Hao]

On 2022/10/23 11:21, Mike Christie wrote:
>> I have some wonder about the target_id like be2iscsi which allocated from 
>> iscsi_sess_ida. Should not we get the target_id from iSCSI target?
>> If they allocate target_id with an random value, how to handle the
>> iscsi_user_scan_session which would check the session's target_id.
>>
> For iscsi, that target id is only a number that's used on the initiator side
> to track the target. The target has no idea what it is and it's never
> sent/used/passed to the target.
> 
> For example, the qla4xxx driver uses it to lookup persistent target info it
> has stored on it's flash. The other use is that we need a unique name for
> the target in sysfs and that target id is used as part of that name.
> .

Thanks for your answer, I would update this patch and open-iscsi's PR.