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:   Thu, 27 Oct 2022 16:02:26 -0500
From:   Mike Christie <michael.christie@...cle.com>
To:     Wenchao Hao <haowenchao@...wei.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 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.