| 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: Wed, 23 Nov 2022 22:21:47 +0800
From: Wenchao Hao <haowenchao@...wei.com>
To: Mike Christie <michael.christie@...cle.com>,
Wenchao Hao <haowenchao22@...il.com>
CC: 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>,
<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/23 2:15, Mike Christie wrote:
> On 11/22/22 11:29 AM, Wenchao Hao wrote:
>> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>> <michael.christie@...cle.com> wrote:
>>>
>>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>>> 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) {
>>>
>>> What was the reason for not checking for ALLOCATED and freeing the ida
>>> in that case?
>>>
>>
>> target_state would be in "ALLOCATED" state if iscsid died after add
>> session successfully.
>> When iscsid restarted, if the session's target_state is "ALLOCATED",
>> it should scan
>> the session and the target_state would switch to "SCANNED".
>>
>> So I think we would not call in __iscsi_unbind_session() with
>> session's target_state
>> is ALLOCATED.
>
> Makes sense for the normal case.
>
> The only issue is when __iscsi_unbind_session is called via
> iscsi_remove_session for the cases where userspace didn't do
> the UNBIND event. Some tools don't do unbind or open-iscsi
> sometimes doesn't if the session is down. We will leak the ida,
> so you need some code to handle that.
>
> .
Sorry, I did not take this condition in consideration. I would change
the __iscsi_unbind_session as following:
1. do not check if target_id is ISCSI_MAX_TARGET
2. define remove_target and default set to true, if target_state is ALLOCATED, then set
it to false and continue the unbind flow; else if target_state not SCANNED, just return.
3. set target_state to ISCSI_SESSION_TARGET_UNBOUND after is sent to avoid potential race condition.
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..9264c75ad9ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1960,31 +1960,40 @@ 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);
}
And the function would be:
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;
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_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);
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);
if (remove_target)
scsi_remove_target(&session->dev);
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");
spin_lock_irqsave(&session->lock, flags);
session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
spin_unlock_irqrestore(&session->lock, flags);
}
Powered by blists - more mailing lists