| 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
| ||
|
Date: Thu, 27 Oct 2022 09:18:25 -0700
From: Lee Duncan <lduncan@...e.com>
To: Wenchao Hao <haowenchao@...wei.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>,
linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Steffen Maier <maier@...ux.ibm.com>, liuzhiqiang26@...wei.com,
linfeilong@...wei.com
Subject: Re: [PATCH v5] scsi:iscsi: Fix multiple iscsi session unbind event
sent to userspace
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@...wei.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
Powered by blists - more mailing lists