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: <105fcfc2-2518-cf61-c17c-9848b32da8fd@huawei.com> Date: Mon, 5 Dec 2022 20:36:08 +0800 From: Wenchao Hao <haowenchao@...wei.com> To: 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 */ ping...
Powered by blists - more mailing lists