| 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: <89692b2b-90f7-e8e8-fa77-f14dbe996b72@huawei.com>
Date: Mon, 21 Nov 2022 22:17:23 +0800
From: Wenchao Hao <haowenchao@...wei.com>
To: Mike Christie <michael.christie@...cle.com>,
Lee Duncan <lduncan@...e.com>, Chris Leech <cleech@...hat.com>,
"Martin K . Petersen" <martin.petersen@...cle.com>,
"James E . J . Bottomley" <jejb@...ux.ibm.com>
CC: <open-iscsi@...glegroups.com>, <linux-scsi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linfeilong@...wei.com>
Subject: Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event
sent to userspace
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");
}
Powered by blists - more mailing lists