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

[Lee Duncan <lduncan@suse.com>]

On 10/27/22 19:00, 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)
> 
> 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 | 51 +++++++++++++++++++++++++++++
>   include/scsi/scsi_transport_iscsi.h |  8 +++++
>   2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..2e0d1cd6d4ea 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,30 @@ 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_ALLOCATED,	"ALLOCATED" },
> +	{ 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;
> +}

It seems like it might be more efficient to use the target state as the 
array index, so you don't have to loop to find the name, e.g. something 
like:

 > static char* iscsi_session_target_state_names[] = {
 > 	.ISCSI_SESSION_TARGET_UNBOUND = "UNBOUND",
 > 	.ISCSI_SESSION_TARGET_ALLOCATED = "ALLOCATED",
 > 	.ISCSI_SESSION_TARGET_BOUND = "BOUND",
 > 	.ISCSI_SESSION_TARGET_UNBINDING = "UNBINDING",
 > };

I know there are only 4 states, and it's only used for sysfs, so not 
sure it matters much.

> +
>   int iscsi_session_chkready(struct iscsi_cls_session *session)
>   {
>   	int err;
> @@ -1779,6 +1803,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data)
>   		goto user_scan_exit;
>   	}
>   	id = session->target_id;
> +	session->target_state = ISCSI_SESSION_TARGET_BOUND;
>   	spin_unlock_irqrestore(&session->lock, flags);
>   
>   	if (id != ISCSI_MAX_TARGET) {
> @@ -1899,6 +1924,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_ALLOCATED;
>   	spin_unlock_irqrestore(&session->lock, flags);
>   	/* start IO */
>   	scsi_target_unblock(&session->dev, SDEV_RUNNING);
> @@ -1961,6 +1987,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");

It'd be nice if this said more, since debugging "Abort unbind sessions" 
would require finding the sources. How about "Abort unbind session: not 
bound", for example?

> +		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 +2019,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 +4406,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 +4518,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 +4632,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..db5db8dea28a 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -236,6 +236,13 @@ enum {
>   	ISCSI_SESSION_FREE,
>   };
>   
> +enum {
> +	ISCSI_SESSION_TARGET_UNBOUND,
> +	ISCSI_SESSION_TARGET_ALLOCATED,
> +	ISCSI_SESSION_TARGET_BOUND,
> +	ISCSI_SESSION_TARGET_UNBINDING,
> +};
> +
>   #define ISCSI_MAX_TARGET -1
>   
>   struct iscsi_cls_session {
> @@ -264,6 +271,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 */

Thank you for sticking with this. It is very much appreciated.
-- 
Lee Duncan