linux-kernel - Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Nov 2022 12:15:45 -0600
From:   Mike Christie <michael.christie@...cle.com>
To:     Wenchao Hao <haowenchao22@...il.com>
Cc:     Wenchao Hao <haowenchao@...wei.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>,
        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 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.