linux-kernel - Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event

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, 14 Apr 2022 10:22:22 -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>,
        open-iscsi@...glegroups.com, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     linfeilong@...wei.com
Subject: Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid
 multiple unbind event

On 4/13/22 8:49 PM, Wenchao Hao wrote:
> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
> If session is in UNBOUND state, do not perform unbind operations anymore,
> else unbind session and set session to UNBOUND state.
> 

I don't think we want this to be a state because you can have a session
with no target or it could be partially deleted and it could be in the
logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of
the target/device removal operation, and we lose the session then we could
go through recovery and the state will go from failed to logged in, and
your new unbound state will have been overwritten.

I think it might be better to have a new sysfs file, target_state, for
this where you could have values like scanning, bound, unbinding, and
unbound, or just a sysfs file, target_bound, that is bool.

> Reference:https://github.com/open-iscsi/open-iscsi/issues/338
> 

You should add a description of the problem in the commit, because that
link might be gone one day.


> Signed-off-by: Wenchao Hao <haowenchao@...wei.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 19 +++++++++++++++++--
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 27951ea05dd4..97a9fee02efa 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1656,6 +1656,7 @@ static struct {
>  	{ ISCSI_SESSION_LOGGED_IN,	"LOGGED_IN" },
>  	{ ISCSI_SESSION_FAILED,		"FAILED" },
>  	{ ISCSI_SESSION_FREE,		"FREE" },
> +	{ ISCSI_SESSION_UNBOUND,	"UNBOUND" },
>  };
>  
>  static const char *iscsi_session_state_name(int state)
> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session *session)
>  	case ISCSI_SESSION_FREE:
>  		err = DID_TRANSPORT_FAILFAST << 16;
>  		break;
> +	case ISCSI_SESSION_UNBOUND:
> +		err = DID_NO_CONNECT << 16;
> +		break;
>  	default:
>  		err = DID_NO_CONNECT << 16;
>  		break;
> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
>  
>  	spin_lock_irqsave(&session->lock, flags);
>  	while (session->state != ISCSI_SESSION_LOGGED_IN) {
> -		if (session->state == ISCSI_SESSION_FREE) {
> +		if ((session->state == ISCSI_SESSION_FREE) ||
> +		    (session->state == ISCSI_SESSION_UNBOUND)) {
>  			ret = FAST_IO_FAIL;
>  			break;
>  		}
> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct *work)
>  		break;
>  	case ISCSI_SESSION_LOGGED_IN:
>  	case ISCSI_SESSION_FREE:
> +	case ISCSI_SESSION_UNBOUND:
>  		/* we raced with the unblock's flush */
>  		spin_unlock_irqrestore(&session->lock, flags);
>  		return;
> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct *work)
>  	unsigned long flags;
>  	unsigned int target_id;
>  
> +	spin_lock_irqsave(&session->lock, flags);
> +	if (session->state == ISCSI_SESSION_UNBOUND) {
> +		spin_unlock_irqrestore(&session->lock, flags);
> +		return;
> +	}
> +	session->state = ISCSI_SESSION_UNBOUND;
> +	spin_unlock_irqrestore(&session->lock, flags);
> +
>  	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>  	/* Prevent new scans and make sure scanning is not in progress */
> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev,				\
>  	struct iscsi_cls_session *session =				\
>  		iscsi_dev_to_session(dev->parent);			\
>  	if ((session->state == ISCSI_SESSION_FREE) ||			\
> -	    (session->state == ISCSI_SESSION_FAILED))			\
> +	    (session->state == ISCSI_SESSION_FAILED) ||			\
> +	    (session->state == ISCSI_SESSION_UNBOUND))			\
>  		return -EBUSY;						\
>  	if (strncmp(buf, "off", 3) == 0) {				\
>  		session->field = -1;					\
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..80149643cbcd 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -232,6 +232,7 @@ enum {
>  	ISCSI_SESSION_LOGGED_IN,
>  	ISCSI_SESSION_FAILED,
>  	ISCSI_SESSION_FREE,
> +	ISCSI_SESSION_UNBOUND,
>  };
>  
>  #define ISCSI_MAX_TARGET -1