| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <806990ed-0e78-a717-db45-370a71bb23c2@huawei.com>
Date: Fri, 9 Dec 2022 10:23:42 +0800
From: Wu Bo <wubo40@...wei.com>
To: Wenchao Hao <haowenchao@...wei.com>, Lee Duncan <lduncan@...e.com>,
Chris Leech <cleech@...hat.com>,
Mike Christie <michael.christie@...cle.com>,
"James E . J . Bottomley" <jejb@...ux.ibm.com>,
"Martin K . Petersen" <martin.petersen@...cle.com>,
<open-iscsi@...glegroups.com>, <linux-scsi@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <liuzhiqiang26@...wei.com>,
<linfeilong@...wei.com>
Subject: Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event
sent to userspace
On 2022/11/26 9:07, 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)
>
> V7:
> - Define target state to string map and refer this map directly
> - Cleanup __iscsi_unbind_session, drop check for session's
> target_id == ISCSI_MAX_TARGET since it can be handled by target_state
>
> 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@...wei.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 47 ++++++++++++++++++++++++++---
> include/scsi/scsi_transport_iscsi.h | 9 ++++++
> 2 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..812578c20fe5 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,13 @@ static const char *iscsi_session_state_name(int state)
> return name;
> }
>
> +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",
> +};
> +
> int iscsi_session_chkready(struct iscsi_cls_session *session)
> {
> int err;
> @@ -1785,9 +1792,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:
> @@ -1960,31 +1971,41 @@ 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);
> }
>
> static void __iscsi_destroy_session(struct work_struct *work)
> @@ -2061,6 +2082,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 +4392,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 +4504,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 +4618,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 */
Reviewed-by: Wu Bo <wubo40@...wei.com>
--
Wu Bo
Powered by blists - more mailing lists