linux-kernel - Re: [PATCH-v2 3/4] target: Fix change depth se

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, 12 Jan 2016 16:07:07 +0100
From:	Christoph Hellwig <hch@....de>
To:	"Nicholas A. Bellinger" <nab@...erainc.com>
Cc:	target-devel <target-devel@...r.kernel.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Sagi Grimberg <sagig@...lanox.com>,
	Christoph Hellwig <hch@....de>, Hannes Reinecke <hare@...e.de>,
	Andy Grover <agrover@...hat.com>,
	Vasu Dev <vasu.dev@...ux.intel.com>, Vu Pham <vu@...lanox.com>,
	Nicholas Bellinger <nab@...ux-iscsi.org>
Subject: Re: [PATCH-v2 3/4] target: Fix change depth se_session reference
	usage

> -static int core_set_queue_depth_for_node(
> -	struct se_portal_group *tpg,
> -	struct se_node_acl *acl)
> +static void
> +target_set_nacl_queue_depth(struct se_portal_group *tpg,
> +			    struct se_node_acl *acl, u32 queue_depth)
>  {
> +	acl->queue_depth = queue_depth;
> +
>  	if (!acl->queue_depth) {
> -		pr_err("Queue depth for %s Initiator Node: %s is 0,"
> +		pr_warn("Queue depth for %s Initiator Node: %s is 0,"
>  			"defaulting to 1.\n", tpg->se_tpg_tfo->get_fabric_name(),
>  			acl->initiatorname);
>  		acl->queue_depth = 1;
>  	}
> -
> -	return 0;
>  }

These changes seem unrelated to the rest, care to explain them or
preferably split them out?

>  int core_tpg_set_initiator_node_queue_depth(
>  	struct se_portal_group *tpg,
> -	unsigned char *initiatorname,
> +	struct se_node_acl *acl,
>  	u32 queue_depth,
>  	int force)

please drop th force parameter as it's always 1.

>  {
> +	LIST_HEAD(sess_list);
> +	struct se_session *sess, *sess_tmp;
>  	unsigned long flags;
> +	int rc;
>  
> +	/*
> +	 * User has requested to change the queue depth for a Initiator Node.
> +	 * Change the value in the Node's struct se_node_acl, and call
> +	 * target_set_nacl_queue_depth() to set the new queue depth.
> +	 */
> +	target_set_nacl_queue_depth(tpg, acl, queue_depth);
>  
> +	spin_lock_irqsave(&acl->nacl_sess_lock, flags);
> +	list_for_each_entry_safe(sess, sess_tmp, &acl->acl_sess_list,
> +				 sess_acl_list) {
> +		if (sess->sess_tearing_down != 0)
>  			continue;
>  
>  		if (!force) {
> @@ -401,71 +387,36 @@ int core_tpg_set_initiator_node_queue_depth(
>  				" operational.  To forcefully change the queue"
>  				" depth and force session reinstatement"
>  				" use the \"force=1\" parameter.\n",
> +				tpg->se_tpg_tfo->get_fabric_name(),
> +				acl->initiatorname);
> +			spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
>  			return -EEXIST;
>  		}
> +		if (!target_get_session(sess))
>  			continue;
> +		spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
>  
> +		 * Finally call tpg->se_tpg_tfo->close_session() to force session
> +		 * reinstatement to occur if there is an active session for the
> +		 * $FABRIC_MOD Initiator Node in question.
>  		 */
> +		rc = tpg->se_tpg_tfo->shutdown_session(sess);
> +		target_put_session(sess);
> +		if (!rc) {
> +			spin_lock_irqsave(&acl->nacl_sess_lock, flags);
> +			continue;
> +		}
> +		target_put_session(sess);
> +		spin_lock_irqsave(&acl->nacl_sess_lock, flags);
>  	}
> +	spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);

It seems at thus point there is no need for ->shutdown_session, it
could be folded into ->close_session in a follow on patch.

> -void target_get_session(struct se_session *se_sess)
> +int target_get_session(struct se_session *se_sess)
>  {
> -	kref_get(&se_sess->sess_kref);
> +	return kref_get_unless_zero(&se_sess->sess_kref);
>  }
>  EXPORT_SYMBOL(target_get_session);

I'd be much happier to have a separate prep patch for this..