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

[PATCH v6] 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)

V6:
- Set target state to ALLOCATED in iscsi_add_session
- Rename state BOUND to SCANNED
- Make iscsi_session_target_state_name() more efficient

V5:
- Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
  target has been allocated but not scanned yet. We should
  sync this session and scan this session when iscsid restarted.

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 | 57 ++++++++++++++++++++++++++++-
 include/scsi/scsi_transport_iscsi.h |  9 +++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..4bbd1aa4a609 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,21 @@ static const char *iscsi_session_state_name(int state)
 	return name;
 }
 
+static char *iscsi_session_target_state_names[] = {
+	"UNBOUND",
+	"ALLOCATED",
+	"SCANNED",
+	"UNBINDING",
+};
+
+static const char *iscsi_session_target_state_name(int state)
+{
+	if (state > ISCSI_SESSION_TARGET_MAX)
+		return NULL;
+
+	return iscsi_session_target_state_names[state];
+}
+
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
 	int err;
@@ -1785,9 +1800,13 @@ static int iscsi_user_scan_session(struct device *dev, void *data)
 		if ((scan_data->channel == SCAN_WILD_CARD ||
 		     scan_data->channel == 0) &&
 		    (scan_data->id == SCAN_WILD_CARD ||
-		     scan_data->id == id))
+		     scan_data->id == id)) {
 			scsi_scan_target(&session->dev, 0, id,
 					 scan_data->lun, scan_data->rescan);
+			spin_lock_irqsave(&session->lock, flags);
+			session->target_state = ISCSI_SESSION_TARGET_SCANNED;
+			spin_unlock_irqrestore(&session->lock, flags);
+		}
 	}
 
 user_scan_exit:
@@ -1961,6 +1980,21 @@ 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_ALLOCATED) {
+		spin_unlock_irqrestore(&session->lock, flags);
+		if (session->ida_used)
+			ida_free(&iscsi_sess_ida, session->target_id);
+		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: allocated\n");
+		goto unbind_session_exit;
+	}
+	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
+		spin_unlock_irqrestore(&session->lock, flags);
+		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: unbinding or unbound\n");
+		return;
+	}
+	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 */
@@ -1972,6 +2006,7 @@ static void __iscsi_unbind_session(struct work_struct *work)
 		goto unbind_session_exit;
 	}
 
+	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
 	target_id = session->target_id;
 	session->target_id = ISCSI_MAX_TARGET;
 	spin_unlock_irqrestore(&session->lock, flags);
@@ -1983,6 +2018,10 @@ static void __iscsi_unbind_session(struct work_struct *work)
 		ida_free(&iscsi_sess_ida, target_id);
 
 unbind_session_exit:
+	spin_lock_irqsave(&session->lock, flags);
+	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+	spin_unlock_irqrestore(&session->lock, flags);
+
 	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
 	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
 }
@@ -2061,6 +2100,9 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 		session->ida_used = true;
 	} else
 		session->target_id = target_id;
+	spin_lock_irqsave(&session->lock, flags);
+	session->target_state = ISCSI_SESSION_TARGET_ALLOCATED;
+	spin_unlock_irqrestore(&session->lock, flags);
 
 	dev_set_name(&session->dev, "session%u", session->sid);
 	err = device_add(&session->dev);
@@ -4368,6 +4410,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 +4522,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 +4636,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..34c03707fb6e 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -236,6 +236,14 @@ enum {
 	ISCSI_SESSION_FREE,
 };
 
+enum {
+	ISCSI_SESSION_TARGET_UNBOUND,
+	ISCSI_SESSION_TARGET_ALLOCATED,
+	ISCSI_SESSION_TARGET_SCANNED,
+	ISCSI_SESSION_TARGET_UNBINDING,
+	ISCSI_SESSION_TARGET_MAX,
+};
+
 #define ISCSI_MAX_TARGET -1
 
 struct iscsi_cls_session {
@@ -264,6 +272,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.32.0

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

[Mike Christie]

On 11/7/22 7:44 PM, Wenchao Hao wrote:
> 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)
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> V5:
> - Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
>   target has been allocated but not scanned yet. We should
>   sync this session and scan this session when iscsid restarted.
> 
> 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 | 57 ++++++++++++++++++++++++++++-
>  include/scsi/scsi_transport_iscsi.h |  9 +++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..4bbd1aa4a609 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,21 @@ static const char *iscsi_session_state_name(int state)
>  	return name;
>  }
>  
> +static char *iscsi_session_target_state_names[] = {
> +	"UNBOUND",
> +	"ALLOCATED",
> +	"SCANNED",
> +	"UNBINDING",
> +};

I think maybe Lee meant you to do something like:

static int iscsi_target_state_to_name[] = {
	[ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
	[ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
	.....


> +
> +static const char *iscsi_session_target_state_name(int state)
> +{
> +	if (state > ISCSI_SESSION_TARGET_MAX)
> +		return NULL;
> +
> +	return iscsi_session_target_state_names[state];
> +}
> +
>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>  {
>  	int err;
> @@ -1785,9 +1800,13 @@ static int iscsi_user_scan_session(struct device *dev, void *data)
>  		if ((scan_data->channel == SCAN_WILD_CARD ||
>  		     scan_data->channel == 0) &&
>  		    (scan_data->id == SCAN_WILD_CARD ||
> -		     scan_data->id == id))
> +		     scan_data->id == id)) {
>  			scsi_scan_target(&session->dev, 0, id,
>  					 scan_data->lun, scan_data->rescan);
> +			spin_lock_irqsave(&session->lock, flags);
> +			session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> +			spin_unlock_irqrestore(&session->lock, flags);
> +		}
>  	}
>  
>  user_scan_exit:
> @@ -1961,6 +1980,21 @@ 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_ALLOCATED) {
> +		spin_unlock_irqrestore(&session->lock, flags);
> +		if (session->ida_used)
> +			ida_free(&iscsi_sess_ida, session->target_id);
> +		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: allocated\n");

Could you change the error message to "Skipping target unbinding: Session not yet scanned.\n"

> +		goto unbind_session_exit;
> +	}

Just add a newline/return here.

> +	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
> +		spin_unlock_irqrestore(&session->lock, flags);
> +		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: unbinding or unbound\n");> +		return;

Could you change the error message to "Skipping target unbinding: Session is unbound/unbinding.\n"

> +	}
> +	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 */


I think you want to move both state checks to after the we take the host lock and
session lock after the line above. You don't have to take the lock multiple times
and we can drop the target_id == ISCSI_MAX_TARGET since it would then rely on the
state checks (I left out the ISCSI_DBG_TRANS_SESSION because I'm lazy):

	bool remove_target = false;
.....


        /* Prevent new scans and make sure scanning is not in progress */
        mutex_lock(&ihost->mutex);
        spin_lock_irqsave(&session->lock, flags);
	if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
		remove_target = true;
		goto unbind_target;
	}

	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
		spin_unlock_irqrestore(&session->lock, flags);
		mutex_unlock(&ihost->mutex);
       		return;
	}

unbind_target:
	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
        target_id = session->target_id;
        session->target_id = ISCSI_MAX_TARGET;
        spin_unlock_irqrestore(&session->lock, flags);
        mutex_unlock(&ihost->mutex);

	if (remove_target)
        	scsi_remove_target(&session->dev);

	if (session->ida_used)
		ida_free(&iscsi_sess_ida, target_id);

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

[Dan Carpenter]

Hi Wenchao,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenchao-Hao/scsi-iscsi-Fix-multiple-iscsi-session-unbind-event-sent-to-userspace/20221107-202840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20221108014414.3510940-1-haowenchao%40huawei.com
patch subject: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace
config: m68k-randconfig-m031-20221108
compiler: m68k-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

smatch warnings:
drivers/scsi/scsi_transport_iscsi.c:1691 iscsi_session_target_state_name() error: buffer overflow 'iscsi_session_target_state_names' 4 <= 4

vim +/iscsi_session_target_state_names +1691 drivers/scsi/scsi_transport_iscsi.c

3aa534a2c8b080 Wenchao Hao 2022-11-07  1686  static const char *iscsi_session_target_state_name(int state)
3aa534a2c8b080 Wenchao Hao 2022-11-07  1687  {
3aa534a2c8b080 Wenchao Hao 2022-11-07  1688  	if (state > ISCSI_SESSION_TARGET_MAX)

Should be >=

3aa534a2c8b080 Wenchao Hao 2022-11-07  1689  		return NULL;
3aa534a2c8b080 Wenchao Hao 2022-11-07  1690  
3aa534a2c8b080 Wenchao Hao 2022-11-07 @1691  	return iscsi_session_target_state_names[state];
3aa534a2c8b080 Wenchao Hao 2022-11-07  1692  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Wenchao Hao]

On 2022/11/9 11:47, Mike Christie wrote:
> On 11/7/22 7:44 PM, Wenchao Hao wrote:
>> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
>> for multiple times which should be fixed.
>>  
>> +static char *iscsi_session_target_state_names[] = {
>> +	"UNBOUND",
>> +	"ALLOCATED",
>> +	"SCANNED",
>> +	"UNBINDING",
>> +};
> 
> I think maybe Lee meant you to do something like:
> 
> static int iscsi_target_state_to_name[] = {
> 	[ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
> 	[ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
> 	.....
> 
> 

Define array as following and remove previous helper function:

static char *iscsi_session_target_state_name[] = {
       [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
       [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
       [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
       [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
};

Reference the array directly:

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

>> +	spin_lock_irqsave(&session->lock, flags);
>> +	if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
>> +		spin_unlock_irqrestore(&session->lock, flags);
>> +		if (session->ida_used)
>> +			ida_free(&iscsi_sess_ida, session->target_id);
>> +		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: allocated\n");
> 
> Could you change the error message to "Skipping target unbinding: Session not yet scanned.\n"
> 
>> +		goto unbind_session_exit;
>> +	}
> 
> Just add a newline/return here.

Actually we should skip unbind this session if call into __iscsi_unbind_session() with target state
is ALLOCATED. So I removed the check, and check only one condition in __iscsi_unbind_session(): if the
target state is SCANNED.

> 
> I think you want to move both state checks to after the we take the host lock and
> session lock after the line above. You don't have to take the lock multiple times
> and we can drop the target_id == ISCSI_MAX_TARGET since it would then rely on the
> state checks (I left out the ISCSI_DBG_TRANS_SESSION because I'm lazy):
> 
> 	bool remove_target = false;
> .....
> 
> 
I think it's not necessary to add a flag remove_target, here is my changes for function __iscsi_unbind_session:

@@ -1966,23 +1977,28 @@ static void __iscsi_unbind_session(struct work_struct *work)
        /* Prevent new scans and make sure scanning is not in progress */
        mutex_lock(&ihost->mutex);
        spin_lock_irqsave(&session->lock, flags);
-       if (session->target_id == ISCSI_MAX_TARGET) {
+       if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
                spin_unlock_irqrestore(&session->lock, flags);
                mutex_unlock(&ihost->mutex);
-               goto unbind_session_exit;
+               ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is %s.\n",
+                                       iscsi_session_target_state_name[session->target_state]);
+               return;
        }
-
        target_id = session->target_id;
        session->target_id = ISCSI_MAX_TARGET;
+       session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
        spin_unlock_irqrestore(&session->lock, flags);
        mutex_unlock(&ihost->mutex);
 
        scsi_remove_target(&session->dev);
 
+       spin_lock_irqsave(&session->lock, flags);
+       session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+       spin_unlock_irqrestore(&session->lock, flags);
+
        if (session->ida_used)
                ida_free(&iscsi_sess_ida, target_id);
 
-unbind_session_exit:
        iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
        ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
 }

And the function looks like following after change:

static void __iscsi_unbind_session(struct work_struct *work)
{
	struct iscsi_cls_session *session =
			container_of(work, struct iscsi_cls_session,
				     unbind_work);
	struct Scsi_Host *shost = iscsi_session_to_shost(session);
	struct iscsi_cls_host *ihost = shost->shost_data;
	unsigned long flags;
	unsigned int target_id;

	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");

	/* Prevent new scans and make sure scanning is not in progress */
	mutex_lock(&ihost->mutex);
	spin_lock_irqsave(&session->lock, flags);
	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
		spin_unlock_irqrestore(&session->lock, flags);
		mutex_unlock(&ihost->mutex);
		ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is %s.\n",
					iscsi_session_target_state_name[session->target_state]);
		return;
	}
	target_id = session->target_id;
	session->target_id = ISCSI_MAX_TARGET;
	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
	spin_unlock_irqrestore(&session->lock, flags);
	mutex_unlock(&ihost->mutex);

	scsi_remove_target(&session->dev);

	spin_lock_irqsave(&session->lock, flags);
	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
	spin_unlock_irqrestore(&session->lock, flags);

	if (session->ida_used)
		ida_free(&iscsi_sess_ida, target_id);

	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
}

Antw: [EXT] Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

[Ulrich Windl]

>>> "'Wenchao Hao' via open-iscsi" <open-iscsi@googlegroups.com> schrieb am
21.11.2022 um 15:17 in Nachricht
<89692b2b-90f7-e8e8-fa77-f14dbe996b72@huawei.com>:
> On 2022/11/9 11:47, Mike Christie wrote:
>> On 11/7/22 7:44 PM, Wenchao Hao wrote:
>>> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
>>> for multiple times which should be fixed.
>>>  
>>> +static char *iscsi_session_target_state_names[] = {
>>> +	"UNBOUND",
>>> +	"ALLOCATED",
>>> +	"SCANNED",
>>> +	"UNBINDING",
>>> +};
>> 
>> I think maybe Lee meant you to do something like:
>> 
>> static int iscsi_target_state_to_name[] = {
>> 	[ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
>> 	[ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
>> 	.....
>> 
>> 
> 
> Define array as following and remove previous helper function:
> 
> static char *iscsi_session_target_state_name[] = {
>        [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
>        [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
>        [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
>        [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
> };
> 
> Reference the array directly:

Actually I think with a modern optimizing compiler there should be little difference in the code created.

> 
> 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]);
> }
> 
>>> +	spin_lock_irqsave(&session->lock, flags);
>>> +	if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
>>> +		spin_unlock_irqrestore(&session->lock, flags);
>>> +		if (session->ida_used)
>>> +			ida_free(&iscsi_sess_ida, session->target_id);
>>> +		ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: allocated\n");
>> 
>> Could you change the error message to "Skipping target unbinding: Session 
> not yet scanned.\n"
>> 
>>> +		goto unbind_session_exit;
>>> +	}
>> 
>> Just add a newline/return here.
> 
> Actually we should skip unbind this session if call into 
> __iscsi_unbind_session() with target state
> is ALLOCATED. So I removed the check, and check only one condition in 
> __iscsi_unbind_session(): if the
> target state is SCANNED.
> 
>> 
>> I think you want to move both state checks to after the we take the host 
> lock and
>> session lock after the line above. You don't have to take the lock multiple 
> times
>> and we can drop the target_id == ISCSI_MAX_TARGET since it would then rely 
> on the
>> state checks (I left out the ISCSI_DBG_TRANS_SESSION because I'm lazy):
>> 
>> 	bool remove_target = false;
>> .....
>> 
>> 
> I think it's not necessary to add a flag remove_target, here is my changes 
> for function __iscsi_unbind_session:
> 
> @@ -1966,23 +1977,28 @@ static void __iscsi_unbind_session(struct 
> work_struct *work)
>         /* Prevent new scans and make sure scanning is not in progress */
>         mutex_lock(&ihost->mutex);
>         spin_lock_irqsave(&session->lock, flags);
> -       if (session->target_id == ISCSI_MAX_TARGET) {
> +       if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>                 spin_unlock_irqrestore(&session->lock, flags);
>                 mutex_unlock(&ihost->mutex);
> -               goto unbind_session_exit;
> +               ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: 
> Session is %s.\n",
> +                                       
> iscsi_session_target_state_name[session->target_state]);
> +               return;
>         }
> -
>         target_id = session->target_id;
>         session->target_id = ISCSI_MAX_TARGET;
> +       session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
>         spin_unlock_irqrestore(&session->lock, flags);
>         mutex_unlock(&ihost->mutex);
>  
>         scsi_remove_target(&session->dev);
>  
> +       spin_lock_irqsave(&session->lock, flags);
> +       session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
> +       spin_unlock_irqrestore(&session->lock, flags);
> +
>         if (session->ida_used)
>                 ida_free(&iscsi_sess_ida, target_id);
>  
> -unbind_session_exit:
>         iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
>         ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
>  }
> 
> And the function looks like following after change:
> 
> static void __iscsi_unbind_session(struct work_struct *work)
> {
> 	struct iscsi_cls_session *session =
> 			container_of(work, struct iscsi_cls_session,
> 				     unbind_work);
> 	struct Scsi_Host *shost = iscsi_session_to_shost(session);
> 	struct iscsi_cls_host *ihost = shost->shost_data;
> 	unsigned long flags;
> 	unsigned int target_id;
> 
> 	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
> 
> 	/* Prevent new scans and make sure scanning is not in progress */
> 	mutex_lock(&ihost->mutex);
> 	spin_lock_irqsave(&session->lock, flags);
> 	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
> 		spin_unlock_irqrestore(&session->lock, flags);
> 		mutex_unlock(&ihost->mutex);
> 		ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is 
> %s.\n",
> 					iscsi_session_target_state_name[session->target_state]);
> 		return;
> 	}
> 	target_id = session->target_id;
> 	session->target_id = ISCSI_MAX_TARGET;
> 	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
> 	spin_unlock_irqrestore(&session->lock, flags);
> 	mutex_unlock(&ihost->mutex);
> 
> 	scsi_remove_target(&session->dev);
> 
> 	spin_lock_irqsave(&session->lock, flags);
> 	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
> 	spin_unlock_irqrestore(&session->lock, flags);
> 
> 	if (session->ida_used)
> 		ida_free(&iscsi_sess_ida, target_id);
> 
> 	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
> 	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
> }
> 
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to open-iscsi+unsubscribe@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/open-iscsi/89692b2b-90f7-e8e8-fa77-f14dbe99 
> 6b72%40huawei.com.

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

[Mike Christie]

On 11/21/22 8:17 AM, Wenchao Hao wrote:
> And the function looks like following after change:
> 
> static void __iscsi_unbind_session(struct work_struct *work)
> {
> 	struct iscsi_cls_session *session =
> 			container_of(work, struct iscsi_cls_session,
> 				     unbind_work);
> 	struct Scsi_Host *shost = iscsi_session_to_shost(session);
> 	struct iscsi_cls_host *ihost = shost->shost_data;
> 	unsigned long flags;
> 	unsigned int target_id;
> 
> 	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
> 
> 	/* Prevent new scans and make sure scanning is not in progress */
> 	mutex_lock(&ihost->mutex);
> 	spin_lock_irqsave(&session->lock, flags);
> 	if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {

What was the reason for not checking for ALLOCATED and freeing the ida
in that case?

> 		spin_unlock_irqrestore(&session->lock, flags);
> 		mutex_unlock(&ihost->mutex);
> 		ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is %s.\n",
> 					iscsi_session_target_state_name[session->target_state]);
> 		return;
> 	}
> 	target_id = session->target_id;
> 	session->target_id = ISCSI_MAX_TARGET;
> 	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
> 	spin_unlock_irqrestore(&session->lock, flags);
> 	mutex_unlock(&ihost->mutex);
> 
> 	scsi_remove_target(&session->dev);
> 
> 	spin_lock_irqsave(&session->lock, flags);
> 	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
> 	spin_unlock_irqrestore(&session->lock, flags);
> 
> 	if (session->ida_used)
> 		ida_free(&iscsi_sess_ida, target_id);
> 
> 	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
> 	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
> }
> 
> 
> 

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

[Wenchao Hao]

On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
<michael.christie@oracle.com> wrote:
>
> On 11/21/22 8:17 AM, Wenchao Hao wrote:
> > And the function looks like following after change:
> >
> > static void __iscsi_unbind_session(struct work_struct *work)
> > {
> >       struct iscsi_cls_session *session =
> >                       container_of(work, struct iscsi_cls_session,
> >                                    unbind_work);
> >       struct Scsi_Host *shost = iscsi_session_to_shost(session);
> >       struct iscsi_cls_host *ihost = shost->shost_data;
> >       unsigned long flags;
> >       unsigned int target_id;
> >
> >       ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
> >
> >       /* Prevent new scans and make sure scanning is not in progress */
> >       mutex_lock(&ihost->mutex);
> >       spin_lock_irqsave(&session->lock, flags);
> >       if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>
> What was the reason for not checking for ALLOCATED and freeing the ida
> in that case?
>

target_state would be in "ALLOCATED" state if iscsid died after add
session successfully.
When iscsid restarted, if the session's target_state is "ALLOCATED",
it should scan
the session and the target_state would switch to "SCANNED".

So I think we would not call in __iscsi_unbind_session() with
session's target_state
is ALLOCATED.

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

[Mike Christie]

On 11/22/22 11:29 AM, Wenchao Hao wrote:
> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
> <michael.christie@oracle.com> wrote:
>>
>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>> And the function looks like following after change:
>>>
>>> static void __iscsi_unbind_session(struct work_struct *work)
>>> {
>>>       struct iscsi_cls_session *session =
>>>                       container_of(work, struct iscsi_cls_session,
>>>                                    unbind_work);
>>>       struct Scsi_Host *shost = iscsi_session_to_shost(session);
>>>       struct iscsi_cls_host *ihost = shost->shost_data;
>>>       unsigned long flags;
>>>       unsigned int target_id;
>>>
>>>       ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>>
>>>       /* Prevent new scans and make sure scanning is not in progress */
>>>       mutex_lock(&ihost->mutex);
>>>       spin_lock_irqsave(&session->lock, flags);
>>>       if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>>
>> What was the reason for not checking for ALLOCATED and freeing the ida
>> in that case?
>>
> 
> target_state would be in "ALLOCATED" state if iscsid died after add
> session successfully.
> When iscsid restarted, if the session's target_state is "ALLOCATED",
> it should scan
> the session and the target_state would switch to "SCANNED".
> 
> So I think we would not call in __iscsi_unbind_session() with
> session's target_state
> is ALLOCATED.

Makes sense for the normal case.

The only issue is when __iscsi_unbind_session is called via
iscsi_remove_session for the cases where userspace didn't do
the  UNBIND event. Some tools don't do unbind or open-iscsi
sometimes doesn't if the session is down. We will leak the ida,
so you need some code to handle that.

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

[Wenchao Hao]

On 2022/11/23 2:15, Mike Christie wrote:
> On 11/22/22 11:29 AM, Wenchao Hao wrote:
>> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>> <michael.christie@oracle.com> wrote:
>>>
>>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>>> And the function looks like following after change:
>>>>
>>>> static void __iscsi_unbind_session(struct work_struct *work)
>>>> {
>>>>       struct iscsi_cls_session *session =
>>>>                       container_of(work, struct iscsi_cls_session,
>>>>                                    unbind_work);
>>>>       struct Scsi_Host *shost = iscsi_session_to_shost(session);
>>>>       struct iscsi_cls_host *ihost = shost->shost_data;
>>>>       unsigned long flags;
>>>>       unsigned int target_id;
>>>>
>>>>       ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>>>
>>>>       /* Prevent new scans and make sure scanning is not in progress */
>>>>       mutex_lock(&ihost->mutex);
>>>>       spin_lock_irqsave(&session->lock, flags);
>>>>       if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>>>
>>> What was the reason for not checking for ALLOCATED and freeing the ida
>>> in that case?
>>>
>>
>> target_state would be in "ALLOCATED" state if iscsid died after add
>> session successfully.
>> When iscsid restarted, if the session's target_state is "ALLOCATED",
>> it should scan
>> the session and the target_state would switch to "SCANNED".
>>
>> So I think we would not call in __iscsi_unbind_session() with
>> session's target_state
>> is ALLOCATED.
> 
> Makes sense for the normal case.
> 
> The only issue is when __iscsi_unbind_session is called via
> iscsi_remove_session for the cases where userspace didn't do
> the  UNBIND event. Some tools don't do unbind or open-iscsi
> sometimes doesn't if the session is down. We will leak the ida,
> so you need some code to handle that.
> 
> .

Sorry, I did not take this condition in consideration. I would change
the __iscsi_unbind_session as following:

1. do not check if target_id is ISCSI_MAX_TARGET
2. define remove_target and default set to true, if target_state is ALLOCATED, then set
   it to false and continue the unbind flow; else if target_state not SCANNED, just return.
3. set target_state to ISCSI_SESSION_TARGET_UNBOUND after is sent to avoid potential race condition.

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..9264c75ad9ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1960,31 +1960,40 @@ static void __iscsi_unbind_session(struct work_struct *work)
        struct iscsi_cls_host *ihost = shost->shost_data;
        unsigned long flags;
        unsigned int target_id;
+       bool remove_target = true;
 
        ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
        /* Prevent new scans and make sure scanning is not in progress */
        mutex_lock(&ihost->mutex);
        spin_lock_irqsave(&session->lock, flags);
-       if (session->target_id == ISCSI_MAX_TARGET) {
+       if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+               remove_target = false;
+       } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
                spin_unlock_irqrestore(&session->lock, flags);
                mutex_unlock(&ihost->mutex);
-               goto unbind_session_exit;
+               ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is unbound/unbinding.\n");
+               return;
        }
 
+       session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
        target_id = session->target_id;
        session->target_id = ISCSI_MAX_TARGET;
        spin_unlock_irqrestore(&session->lock, flags);
        mutex_unlock(&ihost->mutex);
 
-       scsi_remove_target(&session->dev);
+       if (remove_target)
+               scsi_remove_target(&session->dev);
 
        if (session->ida_used)
                ida_free(&iscsi_sess_ida, target_id);
 
-unbind_session_exit:
        iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
        ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
+
+       spin_lock_irqsave(&session->lock, flags);
+       session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+       spin_unlock_irqrestore(&session->lock, flags);
 }

And the function would be:

static void __iscsi_unbind_session(struct work_struct *work)
{
	struct iscsi_cls_session *session =
			container_of(work, struct iscsi_cls_session,
				     unbind_work);
	struct Scsi_Host *shost = iscsi_session_to_shost(session);
	struct iscsi_cls_host *ihost = shost->shost_data;
	unsigned long flags;
	unsigned int target_id;
	bool remove_target = true;

	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");

	/* Prevent new scans and make sure scanning is not in progress */
	mutex_lock(&ihost->mutex);
	spin_lock_irqsave(&session->lock, flags);
	if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
		remove_target = false;
	} else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
		spin_unlock_irqrestore(&session->lock, flags);
		mutex_unlock(&ihost->mutex);
		ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: Session is unbound/unbinding.\n");
		return;
	}

	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
	target_id = session->target_id;
	session->target_id = ISCSI_MAX_TARGET;
	spin_unlock_irqrestore(&session->lock, flags);
	mutex_unlock(&ihost->mutex);

	if (remove_target)
		scsi_remove_target(&session->dev);

	if (session->ida_used)
		ida_free(&iscsi_sess_ida, target_id);

	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");

	spin_lock_irqsave(&session->lock, flags);
	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
	spin_unlock_irqrestore(&session->lock, flags);
}