linux-kernel - Re: [PATCH v5] 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:   Fri, 28 Oct 2022 17:49:44 +0800
From:   Wenchao Hao <haowenchao@...wei.com>
To:     Mike Christie <michael.christie@...cle.com>,
        Lee Duncan <lduncan@...e.com>, Chris Leech <cleech@...hat.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 2022/10/28 5:02, Mike Christie wrote:
> On 10/27/22 9:00 PM, Wenchao Hao wrote:
>> @@ -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;
> 
> I think there are 2 issues with setting this to ISCSI_SESSION_TARGET_ALLOCATED
> here.
> 
> 1. We allocated the target_id in iscsi_add_session so it's weird to set
> the state late above.
> 2. We call the above function when we relogin, so it overwrites 
> ISCSI_SESSION_TARGET_BOUND.
> 
> I think we can just do:
> 
> @@ -1785,7 +1785,12 @@ 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)) {
> +
> +			spin_lock_irqsave(&session->lock, flags);
> +			session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> +			spin_unlock_irqsave(&session->lock, flags);
> +
>  			scsi_scan_target(&session->dev, 0, id,
>  					 scan_data->lun, scan_data->rescan);
>  	}
> @@ -1972,6 +1977,7 @@ static void __iscsi_unbind_session(struct work_struct *work)
>  		goto unbind_session_exit;
>  	}
>  
> +	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
>  	target_id = session->target_id;
>  	session->target_id = ISCSI_MAX_TARGET;
>  	spin_unlock_irqrestore(&session->lock, flags);
> @@ -1983,6 +1989,10 @@ static void __iscsi_unbind_session(struct work_struct *work)
>  		ida_free(&iscsi_sess_ida, target_id);
>  
>  unbind_session_exit:
> +	spin_lock_irqsave(&session->lock, flags);
> +	session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
> +	spin_unlock_irqrestore(&session->lock, flags);
> +
>  	iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
>  	ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
>  }
> @@ -2061,6 +2071,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
>  		session->ida_used = true;
>  	} else
>  		session->target_id = target_id;
> +	session->target_state = ISCSI_SESSION_TARGET_ID_ALLOCATED;
>  
>  	dev_set_name(&session->dev, "session%u", session->sid);
>  	err = device_add(&session->dev);
> 
> 
> then in userspace we can do:
> 
> 1. if have conn in sysfs
> 	if session->target_state == ISCSI_SESSION_TARGET_ID_ALLOCATED
> 		if conn->state == ISCSI_CONN_UP
> 			/*
> 			 * we did the initial login and did a ISCSI_UEVENT_START_CONN
> 			 * but crashed/restarted
> 			 * before we were able to scan.
> 			 */
> 			sync session and scan session
> 		else
> 			/*
> 			 * the initial login didn't happen so
> 			 * just cleanup the kernel. Note: we could also
> 			 * finish setting the conn up but it probably doesn't
> 			 * come up enough to do this.
> 			 */
> 
> 	else if session->target_state == ISCSI_SESSION_TARGET_SCANNED
> 		do normal sync
> 	else if session->target_state == ISCSI_SESSION_TARGET_REMOVING
> 		wait for state to change to ISCSI_SESSION_TARGET_UNBOUND
> 		or setup session/conn in userspace so we can wait for
> 		ISCSI_KEVENT_UNBIND_SESSION then cleanup session/conn.
> 	else if session->target_state == ISCSI_SESSION_TARGET_UNBOUND
> 		cleanup session/conn based on their state and what's in sysfs
> 
>    if no conn in sysfs
> 	cleanup session
> 
> 
> If we are in agreement, then you can take my code above and merge it into
> your patch and resubmit in one patch.
> .

Thanks a lot for your suggestions, I just updated the PR in last night.

I would update this patch according to your suggestions.