[PATCH-v2 0/4] target: Close se_node_acl lookup race

[PATCH-v2 0/4] target: Close se_node_acl lookup race

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

This -v2 series addresses a long standing race between
fabric driver se_node_acl lookup + pointer dereference
during session login, and when kref_get() of ->acl_kref
actually happens within __transport_register_session()
code. 

Also as reported earlier by HCH, go ahead and convert
core_tpg_set_initiator_node_queue_depth() to use proper
se_node_acl->se_acl_list -> se_session dereference,
following how core_tpg_del_initiator_node_acl() works
for invoking explicit session shutdown.

Please review,

--nab

-v2 changes:
  - Have tcm_fc/ib_srpt conversion preceed other changes
  - Fix demo-mode acl regression with generate_node_acls=1
  - Fix set_initiator_node_queue_depth session reference
    usage.
  - Add ib_srpt hack to avoid potential user-space
    backwards-compat issue.

Nicholas Bellinger (4):
  tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage
  ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage
  target: Fix change depth se_session reference usage
  target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

 drivers/infiniband/ulp/srpt/ib_srpt.c        |  95 +++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h        |   2 -
 drivers/target/iscsi/iscsi_target_configfs.c |  14 ++-
 drivers/target/iscsi/iscsi_target_tpg.c      |  10 --
 drivers/target/iscsi/iscsi_target_tpg.h      |   2 -
 drivers/target/target_core_tpg.c             | 161 +++++++++++----------------
 drivers/target/target_core_transport.c       |  22 ++--
 drivers/target/tcm_fc/tfc_conf.c             |  26 ++---
 drivers/target/tcm_fc/tfc_sess.c             |  18 +--
 include/target/target_core_fabric.h          |   4 +-
 10 files changed, 137 insertions(+), 217 deletions(-)

-- 
1.9.1

[PATCH-v2 1/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch does a simple conversion of tcm_fc code to use
proper modern core_tpg_get_initiator_node_acl() lookup using
se_node_acl->acl_kref, and drops the legacy list walk from
ft_acl_get().

Note the original lookup also took node_name into account,
but since ft_init_nodeacl() only ever sets port_name for
se_node_acl->acl_group within configfs, this is purely
a mechanical change.

Cc: Vasu Dev <vasu.dev@linux.intel.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/tcm_fc/tfc_conf.c | 26 ++++++++------------------
 drivers/target/tcm_fc/tfc_sess.c | 18 +++++++++++-------
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_conf.c b/drivers/target/tcm_fc/tfc_conf.c
index 9cdb2ac..9389ba3 100644
--- a/drivers/target/tcm_fc/tfc_conf.c
+++ b/drivers/target/tcm_fc/tfc_conf.c
@@ -222,27 +222,17 @@ static int ft_init_nodeacl(struct se_node_acl *nacl, const char *name)
 
 struct ft_node_acl *ft_acl_get(struct ft_tpg *tpg, struct fc_rport_priv *rdata)
 {
-	struct ft_node_acl *found = NULL;
-	struct ft_node_acl *acl;
 	struct se_portal_group *se_tpg = &tpg->se_tpg;
 	struct se_node_acl *se_acl;
+	unsigned char initiatorname[TRANSPORT_IQN_LEN];
 
-	mutex_lock(&se_tpg->acl_node_mutex);
-	list_for_each_entry(se_acl, &se_tpg->acl_node_list, acl_list) {
-		acl = container_of(se_acl, struct ft_node_acl, se_node_acl);
-		pr_debug("acl %p port_name %llx\n",
-			acl, (unsigned long long)acl->node_auth.port_name);
-		if (acl->node_auth.port_name == rdata->ids.port_name ||
-		    acl->node_auth.node_name == rdata->ids.node_name) {
-			pr_debug("acl %p port_name %llx matched\n", acl,
-				    (unsigned long long)rdata->ids.port_name);
-			found = acl;
-			/* XXX need to hold onto ACL */
-			break;
-		}
-	}
-	mutex_unlock(&se_tpg->acl_node_mutex);
-	return found;
+	ft_format_wwn(&initiatorname[0], TRANSPORT_IQN_LEN, rdata->ids.port_name);
+
+	se_acl = core_tpg_get_initiator_node_acl(se_tpg, &initiatorname[0]);
+	if (!se_acl)
+		return NULL;
+
+	return container_of(se_acl, struct ft_node_acl, se_node_acl);
 }
 
 /*
diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index 45947e2..b3db638 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -191,9 +191,10 @@ out:
  * Caller holds ft_lport_lock.
  */
 static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
-				      struct ft_node_acl *acl)
+				      struct fc_rport_priv *rdata)
 {
 	struct ft_sess *sess;
+	struct ft_node_acl *acl;
 	struct hlist_head *head;
 
 	head = &tport->hash[ft_sess_hash(port_id)];
@@ -212,6 +213,14 @@ static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
 		kfree(sess);
 		return NULL;
 	}
+
+	acl = ft_acl_get(tport->tpg, rdata);
+	if (!acl) {
+		transport_free_session(sess->se_sess);
+		kfree(sess);
+		return NULL;
+	}
+
 	sess->se_sess->se_node_acl = &acl->se_node_acl;
 	sess->tport = tport;
 	sess->port_id = port_id;
@@ -349,17 +358,12 @@ static int ft_prli_locked(struct fc_rport_priv *rdata, u32 spp_len,
 {
 	struct ft_tport *tport;
 	struct ft_sess *sess;
-	struct ft_node_acl *acl;
 	u32 fcp_parm;
 
 	tport = ft_tport_get(rdata->local_port);
 	if (!tport)
 		goto not_target;	/* not a target for this local port */
 
-	acl = ft_acl_get(tport->tpg, rdata);
-	if (!acl)
-		goto not_target;	/* no target for this remote */
-
 	if (!rspp)
 		goto fill;
 
@@ -381,7 +385,7 @@ static int ft_prli_locked(struct fc_rport_priv *rdata, u32 spp_len,
 		spp->spp_flags |= FC_SPP_EST_IMG_PAIR;
 		if (!(fcp_parm & FCP_SPPF_INIT_FCN))
 			return FC_SPP_RESP_CONF;
-		sess = ft_sess_create(tport, rdata->ids.port_id, acl);
+		sess = ft_sess_create(tport, rdata->ids.port_id, rdata);
 		if (!sess)
 			return FC_SPP_RESP_RES;
 		if (!sess->params)
-- 
1.9.1

Re: [PATCH-v2 1/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage

[Bart Van Assche]

On 01/10/2016 12:28 PM, Nicholas A. Bellinger wrote:
> This patch does a simple conversion of tcm_fc code to use
> proper modern core_tpg_get_initiator_node_acl() lookup using
> se_node_acl->acl_kref, and drops the legacy list walk from
> ft_acl_get().
>
> Note the original lookup also took node_name into account,
> but since ft_init_nodeacl() only ever sets port_name for
> se_node_acl->acl_group within configfs, this is purely
> a mechanical change.

Hi Nic,

This patch modifies ft_prli_locked() such that the ACL list lookup only 
happens if FC_SPP_EST_IMG_PAIR has been set by the initiator. That is a 
behavior change and this has not been mentioned in the patch 
description. I think the patch description should mention that behavior 
change and also that it should explain why that behavior change is 
acceptable - if it is acceptable.

Bart.

Re: [PATCH-v2 1/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage

[Christoph Hellwig]

On Sun, Jan 10, 2016 at 08:28:41PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch does a simple conversion of tcm_fc code to use
> proper modern core_tpg_get_initiator_node_acl() lookup using
> se_node_acl->acl_kref, and drops the legacy list walk from
> ft_acl_get().
> 
> Note the original lookup also took node_name into account,
> but since ft_init_nodeacl() only ever sets port_name for
> se_node_acl->acl_group within configfs, this is purely
> a mechanical change.

Please remove ft_acl_get and fold it's new implementation into the
caller.  The later patches actually remove the usage of it but keep it
around, but it would be much better to kill it off here.

Re: [PATCH-v2 1/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage

[Nicholas A. Bellinger]

On Tue, 2016-01-12 at 16:02 +0100, Christoph Hellwig wrote:
> On Sun, Jan 10, 2016 at 08:28:41PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch does a simple conversion of tcm_fc code to use
> > proper modern core_tpg_get_initiator_node_acl() lookup using
> > se_node_acl->acl_kref, and drops the legacy list walk from
> > ft_acl_get().
> > 
> > Note the original lookup also took node_name into account,
> > but since ft_init_nodeacl() only ever sets port_name for
> > se_node_acl->acl_group within configfs, this is purely
> > a mechanical change.
> 
> Please remove ft_acl_get and fold it's new implementation into the
> caller.  The later patches actually remove the usage of it but keep it
> around, but it would be much better to kill it off here.

Done.

[PATCH-v2 2/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch does a simple conversion of ib_srpt code to use
proper modern core_tpg_get_initiator_node_acl() lookup using
se_node_acl->acl_kref, and drops the legacy internal list
usage from srpt_lookup_acl().

This involves doing transport_init_session() earlier, and
making sure transport_free_session() is called during
a se_node_acl lookup failure to drop the last ->acl_kref.

Also, it adds a minor backwards-compat hack to avoid the
potential for user-space wrt node-acl WWPN formatting by
simply stripping off '0x' prefix from ch->sess_name, and
retrying once if core_tpg_get_initiator_node_acl() fails.

Finally, go ahead and drop port_acl_list port_acl_lock
since they are no longer used.

Cc: Vu Pham <vu@mellanox.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 95 +++++++++++------------------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 -
 2 files changed, 30 insertions(+), 67 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2e2fe81..2ef6466 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2370,31 +2370,6 @@ static void srpt_release_channel_work(struct work_struct *w)
 	kfree(ch);
 }
 
-static struct srpt_node_acl *__srpt_lookup_acl(struct srpt_port *sport,
-					       u8 i_port_id[16])
-{
-	struct srpt_node_acl *nacl;
-
-	list_for_each_entry(nacl, &sport->port_acl_list, list)
-		if (memcmp(nacl->i_port_id, i_port_id,
-			   sizeof(nacl->i_port_id)) == 0)
-			return nacl;
-
-	return NULL;
-}
-
-static struct srpt_node_acl *srpt_lookup_acl(struct srpt_port *sport,
-					     u8 i_port_id[16])
-{
-	struct srpt_node_acl *nacl;
-
-	spin_lock_irq(&sport->port_acl_lock);
-	nacl = __srpt_lookup_acl(sport, i_port_id);
-	spin_unlock_irq(&sport->port_acl_lock);
-
-	return nacl;
-}
-
 /**
  * srpt_cm_req_recv() - Process the event IB_CM_REQ_RECEIVED.
  *
@@ -2412,10 +2387,10 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	struct srp_login_rej *rej;
 	struct ib_cm_rep_param *rep_param;
 	struct srpt_rdma_ch *ch, *tmp_ch;
-	struct srpt_node_acl *nacl;
+	struct se_node_acl *se_acl;
 	u32 it_iu_len;
-	int i;
-	int ret = 0;
+	int i, ret = 0;
+	unsigned char *p;
 
 	WARN_ON_ONCE(irqs_disabled());
 
@@ -2565,33 +2540,47 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		       " RTR failed (error code = %d)\n", ret);
 		goto destroy_ib;
 	}
+
 	/*
-	 * Use the initator port identifier as the session name.
+	 * Use the initator port identifier as the session name, when
+	 * checking against se_node_acl->initiatorname[] this can be
+	 * with or without preceeding '0x'.
 	 */
 	snprintf(ch->sess_name, sizeof(ch->sess_name), "0x%016llx%016llx",
 			be64_to_cpu(*(__be64 *)ch->i_port_id),
 			be64_to_cpu(*(__be64 *)(ch->i_port_id + 8)));
 
 	pr_debug("registering session %s\n", ch->sess_name);
+	p = &ch->sess_name[0];
 
-	nacl = srpt_lookup_acl(sport, ch->i_port_id);
-	if (!nacl) {
-		pr_info("Rejected login because no ACL has been"
-			" configured yet for initiator %s.\n", ch->sess_name);
+	ch->sess = transport_init_session(TARGET_PROT_NORMAL);
+	if (IS_ERR(ch->sess)) {
 		rej->reason = cpu_to_be32(
-			      SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
+		pr_debug("Failed to create session\n");
 		goto destroy_ib;
 	}
 
-	ch->sess = transport_init_session(TARGET_PROT_NORMAL);
-	if (IS_ERR(ch->sess)) {
+try_again:
+	se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, p);
+	if (!se_acl) {
+		pr_info("Rejected login because no ACL has been"
+			" configured yet for initiator %s.\n", ch->sess_name);
+		/*
+		 * XXX: Hack to retry of ch->i_port_id without leading '0x'
+		 */
+		if (p == &ch->sess_name[0]) {
+			p += 2;
+			goto try_again;
+		}
 		rej->reason = cpu_to_be32(
-			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
-		pr_debug("Failed to create session\n");
-		goto deregister_session;
+				SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+		transport_free_session(ch->sess);
+		goto destroy_ib;
 	}
-	ch->sess->se_node_acl = &nacl->nacl;
-	transport_register_session(&sport->port_tpg_1, &nacl->nacl, ch->sess, ch);
+	ch->sess->se_node_acl = se_acl;
+
+	transport_register_session(&sport->port_tpg_1, se_acl, ch->sess, ch);
 
 	pr_debug("Establish connection sess=%p name=%s cm_id=%p\n", ch->sess,
 		 ch->sess_name, ch->cm_id);
@@ -2635,8 +2624,6 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 release_channel:
 	srpt_set_ch_state(ch, CH_RELEASING);
 	transport_deregister_session_configfs(ch->sess);
-
-deregister_session:
 	transport_deregister_session(ch->sess);
 	ch->sess = NULL;
 
@@ -3273,8 +3260,6 @@ static void srpt_add_one(struct ib_device *device)
 		sport->port_attrib.srp_max_rsp_size = DEFAULT_MAX_RSP_SIZE;
 		sport->port_attrib.srp_sq_size = DEF_SRPT_SQ_SIZE;
 		INIT_WORK(&sport->work, srpt_refresh_port_work);
-		INIT_LIST_HEAD(&sport->port_acl_list);
-		spin_lock_init(&sport->port_acl_lock);
 
 		if (srpt_refresh_port(sport)) {
 			pr_err("MAD registration failed for %s-%d.\n",
@@ -3522,28 +3507,9 @@ static int srpt_init_nodeacl(struct se_node_acl *se_nacl, const char *name)
 	memcpy(&nacl->i_port_id[0], &i_port_id[0], 16);
 	nacl->sport = sport;
 
-	spin_lock_irq(&sport->port_acl_lock);
-	list_add_tail(&nacl->list, &sport->port_acl_list);
-	spin_unlock_irq(&sport->port_acl_lock);
-
 	return 0;
 }
 
-/*
- * configfs callback function invoked for
- * rmdir /sys/kernel/config/target/$driver/$port/$tpg/acls/$i_port_id
- */
-static void srpt_cleanup_nodeacl(struct se_node_acl *se_nacl)
-{
-	struct srpt_node_acl *nacl =
-		container_of(se_nacl, struct srpt_node_acl, nacl);
-	struct srpt_port *sport = nacl->sport;
-
-	spin_lock_irq(&sport->port_acl_lock);
-	list_del(&nacl->list);
-	spin_unlock_irq(&sport->port_acl_lock);
-}
-
 static ssize_t srpt_tpg_attrib_srp_max_rdma_size_show(struct config_item *item,
 		char *page)
 {
@@ -3820,7 +3786,6 @@ static const struct target_core_fabric_ops srpt_template = {
 	.fabric_make_tpg		= srpt_make_tpg,
 	.fabric_drop_tpg		= srpt_drop_tpg,
 	.fabric_init_nodeacl		= srpt_init_nodeacl,
-	.fabric_cleanup_nodeacl		= srpt_cleanup_nodeacl,
 
 	.tfc_wwn_attrs			= srpt_wwn_attrs,
 	.tfc_tpg_base_attrs		= srpt_tpg_attrs,
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 5faad8ac..bb4fbe2 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -364,11 +364,9 @@ struct srpt_port {
 	u16			sm_lid;
 	u16			lid;
 	union ib_gid		gid;
-	spinlock_t		port_acl_lock;
 	struct work_struct	work;
 	struct se_portal_group	port_tpg_1;
 	struct se_wwn		port_wwn;
-	struct list_head	port_acl_list;
 	struct srpt_port_attrib port_attrib;
 };
 
-- 
1.9.1

Re: [PATCH-v2 2/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage

[Bart Van Assche]

On 01/10/2016 12:28 PM, Nicholas A. Bellinger wrote:
> This patch does a simple conversion of ib_srpt code to use
> proper modern core_tpg_get_initiator_node_acl() lookup using
> se_node_acl->acl_kref, and drops the legacy internal list
> usage from srpt_lookup_acl().
> 
> This involves doing transport_init_session() earlier, and
> making sure transport_free_session() is called during
> a se_node_acl lookup failure to drop the last ->acl_kref.
> 
> Also, it adds a minor backwards-compat hack to avoid the
> potential for user-space wrt node-acl WWPN formatting by
> simply stripping off '0x' prefix from ch->sess_name, and
> retrying once if core_tpg_get_initiator_node_acl() fails.
> 
> Finally, go ahead and drop port_acl_list port_acl_lock
> since they are no longer used.

Hello Nic,

I think that you promised last week to remove the srpt_node_acl
structure (see also
http://thread.gmane.org/gmane.linux.scsi/109275/focus=2121617).
However, this patch doesn't remove that structure.

> -	ch->sess = transport_init_session(TARGET_PROT_NORMAL);
> -	if (IS_ERR(ch->sess)) {
> +try_again:
> +	se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, p);
> +	if (!se_acl) {
> +		pr_info("Rejected login because no ACL has been"
> +			" configured yet for initiator %s.\n", ch->sess_name);
> +		/*
> +		 * XXX: Hack to retry of ch->i_port_id without leading '0x'
> +		 */
> +		if (p == &ch->sess_name[0]) {
> +			p += 2;
> +			goto try_again;
> +		}
>   		rej->reason = cpu_to_be32(
> -			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
> -		pr_debug("Failed to create session\n");
> -		goto deregister_session;
> +				SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
> +		transport_free_session(ch->sess);
> +		goto destroy_ib;
>   	}

The "goto try_again" statement is executed at most once. Since the above loop
can be unfolded with only a very minor code duplication I think it should
be unfolded, e.g. as follows:

	se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
	if (!se_acl)
		se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name + strlen("0x"));
	if (!se_acl) {
		pr_info("Rejected login because no ACL has been"
			" configured yet for initiator %s.\n",
			ch->sess_name);
		[ ... ]
	}

Bart.

Re: [PATCH-v2 2/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage

[Sagi Grimberg]

>
> The "goto try_again" statement is executed at most once. Since the above loop
> can be unfolded with only a very minor code duplication I think it should
> be unfolded, e.g. as follows:
>
> 	se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
> 	if (!se_acl)
> 		se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name + strlen("0x"));
> 	if (!se_acl) {
> 		pr_info("Rejected login because no ACL has been"
> 			" configured yet for initiator %s.\n",
> 			ch->sess_name);
> 		[ ... ]
> 	}
>

I tend to agree, and it would be a bit cleaner to hide this behind
srpt_get_initiator_node_acl wrapper.

[PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts core_tpg_set_initiator_node_queue_depth()
to use struct se_node_acl->acl_sess_list when performing
explicit se_tpg_tfo->shutdown_session() for active sessions,
in order for new se_node_acl->queue_depth to take effect.

This follows how core_tpg_del_initiator_node_acl() currently
works when invoking se_tpg_tfo->shutdown-session(), and ahead
of the next patch to take se_node_acl->acl_kref during lookup,
the extra get_initiator_node_acl() can go away.

This is because se_node_acl->acl_group is already protecting
se_node_acl->acl_group reference via configfs, and shutdown
within core_tpg_del_initiator_node_acl() won't occur until
sys_write() to core_tpg_set_initiator_node_queue_depth()
attribute returns back to user-space.

Also, it includes a bug-fix for iscsi-target where explicit
session shutdown in lio_tpg_shutdown_session() was not
obtaining se_portal_group->session_lock for both cases,
plus drop pointless wrapper.

Note iscsi-target is the only user of this code.

Reported-by: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_configfs.c |  14 +--
 drivers/target/iscsi/iscsi_target_tpg.c      |  10 --
 drivers/target/iscsi/iscsi_target_tpg.h      |   2 -
 drivers/target/target_core_tpg.c             | 143 +++++++++------------------
 drivers/target/target_core_transport.c       |   4 +-
 include/target/target_core_fabric.h          |   4 +-
 6 files changed, 59 insertions(+), 118 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 255204c..3c04a1c 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -726,10 +726,10 @@ static ssize_t lio_target_nacl_cmdsn_depth_store(struct config_item *item,
 	if (iscsit_get_tpg(tpg) < 0)
 		return -EINVAL;
 	/*
-	 * iscsit_tpg_set_initiator_node_queue_depth() assumes force=1
+	 * core_tpg_set_initiator_node_queue_depth() assumes force=1
 	 */
-	ret = iscsit_tpg_set_initiator_node_queue_depth(tpg,
-				config_item_name(acl_ci), cmdsn_depth, 1);
+	ret = core_tpg_set_initiator_node_queue_depth(se_tpg, se_nacl,
+						      cmdsn_depth, 1);
 
 	pr_debug("LIO_Target_ConfigFS: %s/%s Set CmdSN Window: %u for"
 		"InitiatorName: %s\n", config_item_name(wwn_ci),
@@ -1593,28 +1593,30 @@ static int lio_tpg_check_prot_fabric_only(
 }
 
 /*
- * Called with spin_lock_bh(struct se_portal_group->session_lock) held..
- *
  * Also, this function calls iscsit_inc_session_usage_count() on the
  * struct iscsi_session in question.
  */
 static int lio_tpg_shutdown_session(struct se_session *se_sess)
 {
 	struct iscsi_session *sess = se_sess->fabric_sess_ptr;
+	struct se_portal_group *se_tpg = &sess->tpg->tpg_se_tpg;
 
+	spin_lock_bh(&se_tpg->session_lock);
 	spin_lock(&sess->conn_lock);
 	if (atomic_read(&sess->session_fall_back_to_erl0) ||
 	    atomic_read(&sess->session_logout) ||
 	    (sess->time2retain_timer_flags & ISCSI_TF_EXPIRED)) {
 		spin_unlock(&sess->conn_lock);
+		spin_unlock_bh(&se_tpg->session_lock);
 		return 0;
 	}
 	atomic_set(&sess->session_reinstatement, 1);
 	spin_unlock(&sess->conn_lock);
 
 	iscsit_stop_time2retain_timer(sess);
-	iscsit_stop_session(sess, 1, 1);
+	spin_unlock_bh(&se_tpg->session_lock);
 
+	iscsit_stop_session(sess, 1, 1);
 	return 1;
 }
 
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 23c95cd..0814e58 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -590,16 +590,6 @@ int iscsit_tpg_del_network_portal(
 	return iscsit_tpg_release_np(tpg_np, tpg, np);
 }
 
-int iscsit_tpg_set_initiator_node_queue_depth(
-	struct iscsi_portal_group *tpg,
-	unsigned char *initiatorname,
-	u32 queue_depth,
-	int force)
-{
-	return core_tpg_set_initiator_node_queue_depth(&tpg->tpg_se_tpg,
-		initiatorname, queue_depth, force);
-}
-
 int iscsit_ta_authentication(struct iscsi_portal_group *tpg, u32 authentication)
 {
 	unsigned char buf1[256], buf2[256], *none = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
index 9db32bd..2da2119 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.h
+++ b/drivers/target/iscsi/iscsi_target_tpg.h
@@ -26,8 +26,6 @@ extern struct iscsi_tpg_np *iscsit_tpg_add_network_portal(struct iscsi_portal_gr
 			int);
 extern int iscsit_tpg_del_network_portal(struct iscsi_portal_group *,
 			struct iscsi_tpg_np *);
-extern int iscsit_tpg_set_initiator_node_queue_depth(struct iscsi_portal_group *,
-			unsigned char *, u32, int);
 extern int iscsit_ta_authentication(struct iscsi_portal_group *, u32);
 extern int iscsit_ta_login_timeout(struct iscsi_portal_group *, u32);
 extern int iscsit_ta_netif_timeout(struct iscsi_portal_group *, u32);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 62103a8..66a2c6f 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -157,28 +157,25 @@ void core_tpg_add_node_to_devs(
 	mutex_unlock(&tpg->tpg_lun_mutex);
 }
 
-/*      core_set_queue_depth_for_node():
- *
- *
- */
-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;
 }
 
 static struct se_node_acl *target_alloc_node_acl(struct se_portal_group *tpg,
 		const unsigned char *initiatorname)
 {
 	struct se_node_acl *acl;
+	u32 queue_depth;
 
 	acl = kzalloc(max(sizeof(*acl), tpg->se_tpg_tfo->node_acl_size),
 			GFP_KERNEL);
@@ -193,24 +190,20 @@ static struct se_node_acl *target_alloc_node_acl(struct se_portal_group *tpg,
 	spin_lock_init(&acl->nacl_sess_lock);
 	mutex_init(&acl->lun_entry_mutex);
 	atomic_set(&acl->acl_pr_ref_count, 0);
+
 	if (tpg->se_tpg_tfo->tpg_get_default_depth)
-		acl->queue_depth = tpg->se_tpg_tfo->tpg_get_default_depth(tpg);
+		queue_depth = tpg->se_tpg_tfo->tpg_get_default_depth(tpg);
 	else
-		acl->queue_depth = 1;
+		queue_depth = 1;
+	target_set_nacl_queue_depth(tpg, acl, queue_depth);
+
 	snprintf(acl->initiatorname, TRANSPORT_IQN_LEN, "%s", initiatorname);
 	acl->se_tpg = tpg;
 	acl->acl_index = scsi_get_new_index(SCSI_AUTH_INTR_INDEX);
 
 	tpg->se_tpg_tfo->set_default_node_attributes(acl);
 
-	if (core_set_queue_depth_for_node(tpg, acl) < 0)
-		goto out_free_acl;
-
 	return acl;
-
-out_free_acl:
-	kfree(acl);
-	return NULL;
 }
 
 static void target_add_node_acl(struct se_node_acl *acl)
@@ -327,7 +320,8 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
 		if (sess->sess_tearing_down != 0)
 			continue;
 
-		target_get_session(sess);
+		if (!target_get_session(sess))
+			continue;
 		list_move(&sess->sess_acl_list, &sess_list);
 	}
 	spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
@@ -365,34 +359,26 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
  */
 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)
 {
-	struct se_session *sess, *init_sess = NULL;
-	struct se_node_acl *acl;
+	LIST_HEAD(sess_list);
+	struct se_session *sess, *sess_tmp;
 	unsigned long flags;
-	int dynamic_acl = 0;
+	int rc;
 
-	mutex_lock(&tpg->acl_node_mutex);
-	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
-	if (!acl) {
-		pr_err("Access Control List entry for %s Initiator"
-			" Node %s does not exists for TPG %hu, ignoring"
-			" request.\n", tpg->se_tpg_tfo->get_fabric_name(),
-			initiatorname, tpg->se_tpg_tfo->tpg_get_tag(tpg));
-		mutex_unlock(&tpg->acl_node_mutex);
-		return -ENODEV;
-	}
-	if (acl->dynamic_node_acl) {
-		acl->dynamic_node_acl = 0;
-		dynamic_acl = 1;
-	}
-	mutex_unlock(&tpg->acl_node_mutex);
+	/*
+	 * 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(&tpg->session_lock, flags);
-	list_for_each_entry(sess, &tpg->tpg_sess_list, sess_list) {
-		if (sess->se_node_acl != acl)
+	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(), initiatorname);
-			spin_unlock_irqrestore(&tpg->session_lock, flags);
-
-			mutex_lock(&tpg->acl_node_mutex);
-			if (dynamic_acl)
-				acl->dynamic_node_acl = 1;
-			mutex_unlock(&tpg->acl_node_mutex);
+				tpg->se_tpg_tfo->get_fabric_name(),
+				acl->initiatorname);
+			spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
 			return -EEXIST;
 		}
-		/*
-		 * Determine if the session needs to be closed by our context.
-		 */
-		if (!tpg->se_tpg_tfo->shutdown_session(sess))
+		if (!target_get_session(sess))
 			continue;
+		spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
 
-		init_sess = sess;
-		break;
-	}
-
-	/*
-	 * 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
-	 * core_set_queue_depth_for_node() to add the requested queue depth.
-	 *
-	 * 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.
-	 */
-	acl->queue_depth = queue_depth;
-
-	if (core_set_queue_depth_for_node(tpg, acl) < 0) {
-		spin_unlock_irqrestore(&tpg->session_lock, flags);
 		/*
-		 * Force session reinstatement if
-		 * core_set_queue_depth_for_node() failed, because we assume
-		 * the $FABRIC_MOD has already the set session reinstatement
-		 * bit from tpg->se_tpg_tfo->shutdown_session() called above.
+		 * 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.
 		 */
-		if (init_sess)
-			tpg->se_tpg_tfo->close_session(init_sess);
-
-		mutex_lock(&tpg->acl_node_mutex);
-		if (dynamic_acl)
-			acl->dynamic_node_acl = 1;
-		mutex_unlock(&tpg->acl_node_mutex);
-		return -EINVAL;
+		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(&tpg->session_lock, flags);
-	/*
-	 * If the $FABRIC_MOD session for the Initiator Node ACL exists,
-	 * forcefully shutdown the $FABRIC_MOD session/nexus.
-	 */
-	if (init_sess)
-		tpg->se_tpg_tfo->close_session(init_sess);
+	spin_unlock_irqrestore(&acl->nacl_sess_lock, flags);
 
 	pr_debug("Successfully changed queue depth to: %d for Initiator"
-		" Node: %s on %s Target Portal Group: %u\n", queue_depth,
-		initiatorname, tpg->se_tpg_tfo->get_fabric_name(),
+		" Node: %s on %s Target Portal Group: %u\n", acl->queue_depth,
+		acl->initiatorname, tpg->se_tpg_tfo->get_fabric_name(),
 		tpg->se_tpg_tfo->tpg_get_tag(tpg));
 
-	mutex_lock(&tpg->acl_node_mutex);
-	if (dynamic_acl)
-		acl->dynamic_node_acl = 1;
-	mutex_unlock(&tpg->acl_node_mutex);
-
 	return 0;
 }
 EXPORT_SYMBOL(core_tpg_set_initiator_node_queue_depth);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index eb7aaf0..7b05ebf 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -384,9 +384,9 @@ static void target_release_session(struct kref *kref)
 	se_tpg->se_tpg_tfo->close_session(se_sess);
 }
 
-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);
 
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index de21130..dc6b09e 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -117,7 +117,7 @@ void	__transport_register_session(struct se_portal_group *,
 		struct se_node_acl *, struct se_session *, void *);
 void	transport_register_session(struct se_portal_group *,
 		struct se_node_acl *, struct se_session *, void *);
-void	target_get_session(struct se_session *);
+int	target_get_session(struct se_session *);
 void	target_put_session(struct se_session *);
 ssize_t	target_show_dynamic_sessions(struct se_portal_group *, char *);
 void	transport_free_session(struct se_session *);
@@ -172,7 +172,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
 struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *,
 		unsigned char *);
 int	core_tpg_set_initiator_node_queue_depth(struct se_portal_group *,
-		unsigned char *, u32, int);
+		struct se_node_acl *, u32, int);
 int	core_tpg_set_initiator_node_tag(struct se_portal_group *,
 		struct se_node_acl *, const char *);
 int	core_tpg_register(struct se_wwn *, struct se_portal_group *, int);
-- 
1.9.1

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Christoph Hellwig]

> -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..

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Nicholas A. Bellinger]

On Tue, 2016-01-12 at 16:07 +0100, Christoph Hellwig wrote:
> > -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?

With this patch in place, this function is now also called by
core_tpg_set_initiator_node_queue_depth(), where previously it was
called only during target_alloc_node_acl().

Might as well drop the ignored return while we're at it..

> 
> >  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.
> 

Done.

> >  {
> > +	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.
> 

Not exactly.

It's the final target_put_session() -> kref_put() upon
se_sess->sess_kref that invokes TFO->close_session().

> > -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..

Since this will need to hit stable at some point, it likely needs to
stay with the original bug-fix.

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Christoph Hellwig]

On Tue, Jan 12, 2016 at 11:29:32PM -0800, Nicholas A. Bellinger wrote:
> With this patch in place, this function is now also called by
> core_tpg_set_initiator_node_queue_depth(), where previously it was
> called only during target_alloc_node_acl().
> 
> Might as well drop the ignored return while we're at it..

Please add it to the patch description..

> > > +			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.
> > 
> 
> Not exactly.
> 
> It's the final target_put_session() -> kref_put() upon
> se_sess->sess_kref that invokes TFO->close_session().

I know.  But we are dropping the only long term held reference
here, so the two are more or less equivalent. 

> > > -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..
> 
> Since this will need to hit stable at some point, it likely needs to
> stay with the original bug-fix.

Please at least document it in the patch description.  I'd still
be happier to have the change to target_get_session as a preparatory
patch, though.

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Nicholas A. Bellinger]

On Wed, 2016-01-13 at 09:24 +0100, Christoph Hellwig wrote:
> On Tue, Jan 12, 2016 at 11:29:32PM -0800, Nicholas A. Bellinger wrote:
> > With this patch in place, this function is now also called by
> > core_tpg_set_initiator_node_queue_depth(), where previously it was
> > called only during target_alloc_node_acl().
> > 
> > Might as well drop the ignored return while we're at it..
> 
> Please add it to the patch description..
> 

Done.

> > > > +			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.
> > > 
> > 
> > Not exactly.
> > 
> > It's the final target_put_session() -> kref_put() upon
> > se_sess->sess_kref that invokes TFO->close_session().
> 
> I know.  But we are dropping the only long term held reference
> here, so the two are more or less equivalent. 

No.  If ->shutdown_session() succeeds, then target-core is
responsible for dropping both references.

> > > > -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..
> > 
> > Since this will need to hit stable at some point, it likely needs to
> > stay with the original bug-fix.
> 
> Please at least document it in the patch description.  I'd still
> be happier to have the change to target_get_session as a preparatory
> patch, though.

Done.

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Christoph Hellwig]

>  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)

And while we're at it - the tpg should be dropped as well, we can
get it from acl->se_tpg trivially.

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Nicholas A. Bellinger]

On Wed, 2016-01-13 at 09:27 +0100, Christoph Hellwig wrote:
> >  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)
> 
> And while we're at it - the tpg should be dropped as well, we can
> get it from acl->se_tpg trivially.

Done.

Re: [PATCH-v2 3/4] target: Fix change depth se_session reference usage

[Sagi Grimberg]

> Also, it includes a bug-fix for iscsi-target where explicit
> session shutdown in lio_tpg_shutdown_session() was not
> obtaining se_portal_group->session_lock for both cases,
> plus drop pointless wrapper.

It'd be better to move this to a dedicated patch IMO.

The other split I was going to suggest was target_get_session()
which you already took care of...

[PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch addresses a long standing race where obtaining
se_node_acl->acl_kref in __transport_register_session()
happens a bit too late, and leaves open the potential
for core_tpg_del_initiator_node_acl() to hit a NULL
pointer dereference.

Instead, take ->acl_kref in core_tpg_get_initiator_node_acl()
while se_portal_group->acl_node_mutex is held, and move the
final target_put_nacl() from transport_deregister_session()
into transport_free_session() so that fabric driver login
failure handling using the modern method to still work
as expected.

Also, update core_tpg_check_initiator_node_acl() to take
an extra reference for dynamically generated acls for
demo-mode, before returning to fabric caller.

Note the existing wait_for_completion(&acl->acl_free_comp)
in core_tpg_del_initiator_node_acl() does not change.

Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_tpg.c       | 18 +++++++++++++++++-
 drivers/target/target_core_transport.c | 18 ++++++++++++------
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 66a2c6f..5221aee 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -75,9 +75,16 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
 	unsigned char *initiatorname)
 {
 	struct se_node_acl *acl;
-
+	/*
+	 * Obtain the acl_kref now, which will be dropped upon the
+	 * release of se_sess memory within transport_free_session().
+	 */
 	mutex_lock(&tpg->acl_node_mutex);
 	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
+	if (acl) {
+		if (!kref_get_unless_zero(&acl->acl_kref))
+			acl = NULL;
+	}
 	mutex_unlock(&tpg->acl_node_mutex);
 
 	return acl;
@@ -240,6 +247,15 @@ struct se_node_acl *core_tpg_check_initiator_node_acl(
 	acl = target_alloc_node_acl(tpg, initiatorname);
 	if (!acl)
 		return NULL;
+	/*
+	 * When allocating a dynamically generated node_acl, go ahead
+	 * and take the extra kref now before returning to the fabric
+	 * driver caller.
+	 *
+	 * Note this reference will be released at session shutdown
+	 * time within transport_free_session() code.
+	 */
+	kref_get(&acl->acl_kref);
 	acl->dynamic_node_acl = 1;
 
 	/*
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7b05ebf..c5035b9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -341,7 +341,6 @@ void __transport_register_session(
 					&buf[0], PR_REG_ISID_LEN);
 			se_sess->sess_bin_isid = get_unaligned_be64(&buf[0]);
 		}
-		kref_get(&se_nacl->acl_kref);
 
 		spin_lock_irq(&se_nacl->nacl_sess_lock);
 		/*
@@ -464,6 +463,15 @@ EXPORT_SYMBOL(transport_deregister_session_configfs);
 
 void transport_free_session(struct se_session *se_sess)
 {
+	struct se_node_acl *se_nacl = se_sess->se_node_acl;
+	/*
+	 * Drop the se_node_acl->nacl_kref obtained from within
+	 * core_tpg_get_initiator_node_acl().
+	 */
+	if (se_nacl) {
+		se_sess->se_node_acl = NULL;
+		target_put_nacl(se_nacl);
+	}
 	if (se_sess->sess_cmd_map) {
 		percpu_ida_destroy(&se_sess->sess_tag_pool);
 		kvfree(se_sess->sess_cmd_map);
@@ -478,7 +486,7 @@ void transport_deregister_session(struct se_session *se_sess)
 	const struct target_core_fabric_ops *se_tfo;
 	struct se_node_acl *se_nacl;
 	unsigned long flags;
-	bool comp_nacl = true, drop_nacl = false;
+	bool drop_nacl = false;
 
 	if (!se_tpg) {
 		transport_free_session(se_sess);
@@ -510,18 +518,16 @@ void transport_deregister_session(struct se_session *se_sess)
 	if (drop_nacl) {
 		core_tpg_wait_for_nacl_pr_ref(se_nacl);
 		core_free_device_list_for_node(se_nacl, se_tpg);
+		se_sess->se_node_acl = NULL;
 		kfree(se_nacl);
-		comp_nacl = false;
 	}
 	pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",
 		se_tpg->se_tpg_tfo->get_fabric_name());
 	/*
 	 * If last kref is dropping now for an explicit NodeACL, awake sleeping
 	 * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group
-	 * removal context.
+	 * removal context from within transport_free_session() code.
 	 */
-	if (se_nacl && comp_nacl)
-		target_put_nacl(se_nacl);
 
 	transport_free_session(se_sess);
 }
-- 
1.9.1

Re: [PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Christoph Hellwig]

> +++ b/drivers/target/target_core_tpg.c
> @@ -75,9 +75,16 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
>  	unsigned char *initiatorname)
>  {
>  	struct se_node_acl *acl;
> -
> +	/*
> +	 * Obtain the acl_kref now, which will be dropped upon the
> +	 * release of se_sess memory within transport_free_session().
> +	 */

As said before this is incorrect, or rather shold be incorrect.  I
think it should be removed, but if you want to keep it should be
in core_tpg_check_initiator_node_acl and not here.

The call of core_tpg_get_initiator_node_acl in
iscsit_build_sendtargets_response needs a put to balance out the
get of this kref while we're at it.

Re: [PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Nicholas A. Bellinger]

On Tue, 2016-01-12 at 16:09 +0100, Christoph Hellwig wrote:
> > +++ b/drivers/target/target_core_tpg.c
> > @@ -75,9 +75,16 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
> >  	unsigned char *initiatorname)
> >  {
> >  	struct se_node_acl *acl;
> > -
> > +	/*
> > +	 * Obtain the acl_kref now, which will be dropped upon the
> > +	 * release of se_sess memory within transport_free_session().
> > +	 */
> 
> As said before this is incorrect, or rather shold be incorrect.  I
> think it should be removed, but if you want to keep it should be
> in core_tpg_check_initiator_node_acl and not here.
> 

Done.

Here's a incremental patch being folded in for -v3:

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 52ae8ba..62f02ea 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -75,16 +75,9 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
        unsigned char *initiatorname)
 {
        struct se_node_acl *acl;
-       /*
-        * Obtain the acl_kref now, which will be dropped upon the
-        * release of se_sess memory within transport_free_session().
-        */
+
        mutex_lock(&tpg->acl_node_mutex);
        acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
-       if (acl) {
-               if (!kref_get_unless_zero(&acl->acl_kref))
-                       acl = NULL;
-       }
        mutex_unlock(&tpg->acl_node_mutex);
 
        return acl;
@@ -236,11 +229,24 @@ struct se_node_acl *core_tpg_check_initiator_node_acl(
        unsigned char *initiatorname)
 {
        struct se_node_acl *acl;
+       /*
+        * Obtain the acl_kref now, which will be dropped upon the
+        * release of se_sess memory within transport_free_session().
+        */
+       mutex_lock(&tpg->acl_node_mutex);
+       acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
+       if (acl) {
+               if (!kref_get_unless_zero(&acl->acl_kref))
+                       acl = NULL;
+               mutex_unlock(&tpg->acl_node_mutex);
+               if (!acl)
+                       goto check_demo_mode;
 
-       acl = core_tpg_get_initiator_node_acl(tpg, initiatorname);
-       if (acl)
                return acl;
+       }
+       mutex_unlock(&tpg->acl_node_mutex);
 
+check_demo_mode:
        if (!tpg->se_tpg_tfo->tpg_check_demo_mode(tpg))
                return NULL;
> 
> 
> 
> The call of core_tpg_get_initiator_node_acl in
> iscsit_build_sendtargets_response needs a put to balance out the
> get of this kref while we're at it.

How about the following..?

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index a81c0e5..4125520 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3383,6 +3383,7 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,
        struct iscsi_portal_group *tpg;
        struct iscsi_tiqn *tiqn;
        struct iscsi_tpg_np *tpg_np;
+       struct se_node_acl *se_acl;
        int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
        int target_name_printed;
        unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
@@ -3435,10 +3436,14 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,
 
                        if ((tpg->tpg_attrib.generate_node_acls == 0) &&
                            (tpg->tpg_attrib.demo_mode_discovery == 0) &&
-                           (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
-                               cmd->conn->sess->sess_ops->InitiatorName))) {
+                           (!(se_acl = core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+                               cmd->conn->sess->sess_ops->InitiatorName)))) {
                                continue;
                        }
+                       if (se_acl) {
+                               target_put_nacl(se_acl);
+                               se_acl = NULL;
+                       }
 
                        spin_lock(&tpg->tpg_state_lock);
                        active = (tpg->tpg_state == TPG_STATE_ACTIVE);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c5035b9..a7c1bb5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -431,6 +431,7 @@ void target_put_nacl(struct se_node_acl *nacl)
 {
        kref_put(&nacl->acl_kref, target_complete_nacl);
 }
+EXPORT_SYMBOL(target_put_nacl);
 
 void transport_deregister_session_configfs(struct se_session *se_sess)
 {

Re: [PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Christoph Hellwig]

On Wed, Jan 13, 2016 at 12:00:28AM -0800, Nicholas A. Bellinger wrote:
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index 52ae8ba..62f02ea 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -75,16 +75,9 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
>         unsigned char *initiatorname)
>  {
>         struct se_node_acl *acl;
> -       /*
> -        * Obtain the acl_kref now, which will be dropped upon the
> -        * release of se_sess memory within transport_free_session().
> -        */
> +
>         mutex_lock(&tpg->acl_node_mutex);
>         acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> -       if (acl) {
> -               if (!kref_get_unless_zero(&acl->acl_kref))
> -                       acl = NULL;
> -       }

nah, please keep the kref_get_unless_zero here, it's needed.  It's
just the misleding comment I was complaining about :)

> @@ -3435,10 +3436,14 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,
>  
>                         if ((tpg->tpg_attrib.generate_node_acls == 0) &&
>                             (tpg->tpg_attrib.demo_mode_discovery == 0) &&
> -                           (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> -                               cmd->conn->sess->sess_ops->InitiatorName))) {
> +                           (!(se_acl = core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> +                               cmd->conn->sess->sess_ops->InitiatorName)))) {
>                                 continue;
>                         }
> +                       if (se_acl) {
> +                               target_put_nacl(se_acl);
> +                               se_acl = NULL;
> +                       }


Seems like with the current code we don't need to keep the ACL,
so a new helper like:

bool target_tpg_has_node_acl(struct se_portal_group *tpg,
	const char *initiatorname)
{
	struct se_node_acl *acl;

	acl = core_tpg_get_initiator_node_acl(tpg, initiatorname);
	if (acl) {
		target_put_nacl(se_acl);
		return true;
	}

	return false;
}

or fully open coded as:

bool target_tpg_has_node_acl(struct se_portal_group *tpg,
		 const char *initiatorname)
{
	struct se_node_acl *acl;
	bool found = false;

	mutex_lock(&tpg->acl_node_mutex);
	list_for_each_entry(acl, &tpg->acl_node_list, acl_list) {
		if (!strcmp(acl->initiatorname, initiatorname)) {
			found = true;
			break;
		}
	}
	mutex_unlock(&tpg->acl_node_mutex);

	return found;
}

might be better to keep the code readable.

Re: [PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Nicholas A. Bellinger]

On Wed, 2016-01-13 at 09:15 +0100, Christoph Hellwig wrote:
> On Wed, Jan 13, 2016 at 12:00:28AM -0800, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> > index 52ae8ba..62f02ea 100644
> > --- a/drivers/target/target_core_tpg.c
> > +++ b/drivers/target/target_core_tpg.c
> > @@ -75,16 +75,9 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
> >         unsigned char *initiatorname)
> >  {
> >         struct se_node_acl *acl;
> > -       /*
> > -        * Obtain the acl_kref now, which will be dropped upon the
> > -        * release of se_sess memory within transport_free_session().
> > -        */
> > +
> >         mutex_lock(&tpg->acl_node_mutex);
> >         acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> > -       if (acl) {
> > -               if (!kref_get_unless_zero(&acl->acl_kref))
> > -                       acl = NULL;
> > -       }
> 
> nah, please keep the kref_get_unless_zero here, it's needed.  It's
> just the misleding comment I was complaining about :)

Ok.  How about the following comment instead..?

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 62f02ea..1984bb2 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -75,9 +75,21 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
        unsigned char *initiatorname)
 {
        struct se_node_acl *acl;
-
+       /*
+        * Obtain se_node_acl->acl_kref using fabric driver provided
+        * initiatorname[] during node acl endpoint lookup driven by
+        * new se_session login.
+        *
+        * The reference is held until se_session shutdown -> release
+        * occurs via fabric driver invoked transport_deregister_session()
+        * or transport_free_session() code.
+        */

> 
> > @@ -3435,10 +3436,14 @@ iscsit_build_sendtargets_response(struct iscsi_cmd *cmd,
> >  
> >                         if ((tpg->tpg_attrib.generate_node_acls == 0) &&
> >                             (tpg->tpg_attrib.demo_mode_discovery == 0) &&
> > -                           (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> > -                               cmd->conn->sess->sess_ops->InitiatorName))) {
> > +                           (!(se_acl = core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
> > +                               cmd->conn->sess->sess_ops->InitiatorName)))) {
> >                                 continue;
> >                         }
> > +                       if (se_acl) {
> > +                               target_put_nacl(se_acl);
> > +                               se_acl = NULL;
> > +                       }
> 
> 
> Seems like with the current code we don't need to keep the ACL,
> so a new helper like:
> 
> bool target_tpg_has_node_acl(struct se_portal_group *tpg,
> 	const char *initiatorname)
> {
> 	struct se_node_acl *acl;
> 
> 	acl = core_tpg_get_initiator_node_acl(tpg, initiatorname);
> 	if (acl) {
> 		target_put_nacl(se_acl);
> 		return true;
> 	}
> 
> 	return false;
> }
> 
> or fully open coded as:
> 
> bool target_tpg_has_node_acl(struct se_portal_group *tpg,
> 		 const char *initiatorname)
> {
> 	struct se_node_acl *acl;
> 	bool found = false;
> 
> 	mutex_lock(&tpg->acl_node_mutex);
> 	list_for_each_entry(acl, &tpg->acl_node_list, acl_list) {
> 		if (!strcmp(acl->initiatorname, initiatorname)) {
> 			found = true;
> 			break;
> 		}
> 	}
> 	mutex_unlock(&tpg->acl_node_mutex);
> 
> 	return found;
> }
> 
> might be better to keep the code readable.

Seems reasonable.  Adding this instead.

Re: [PATCH-v2 4/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

[Christoph Hellwig]

On Wed, Jan 13, 2016 at 12:38:08AM -0800, Nicholas A. Bellinger wrote:
> Ok.  How about the following comment instead..?

That one looks correct.  I still don't think it adds much of a value,
though..