Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1304221 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] target: Close se_node_acl lookup race "Nicholas A. Bellinger" <nab@daterainc.com> - 2016-01-08 08:20 +0100
    [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl "Nicholas A. Bellinger" <nab@daterainc.com> - 2016-01-08 08:20 +0100
      Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during  get_initiator_node_acl Christoph Hellwig <hch@lst.de> - 2016-01-08 09:20 +0100
        Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during  get_initiator_node_acl Christoph Hellwig <hch@lst.de> - 2016-01-08 09:40 +0100
        Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during  get_initiator_node_acl "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-01-08 09:50 +0100
          Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during  get_initiator_node_acl Christoph Hellwig <hch@lst.de> - 2016-01-08 10:10 +0100
            Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during  get_initiator_node_acl Christoph Hellwig <hch@lst.de> - 2016-01-08 10:40 +0100
    [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup "Nicholas A. Bellinger" <nab@daterainc.com> - 2016-01-08 08:20 +0100
      Re: [PATCH 2/4] target: Remove useless  set_initiator_node_queue_depth acl lookup Christoph Hellwig <hch@lst.de> - 2016-01-08 09:20 +0100

#1304221 — [PATCH 0/4] target: Close se_node_acl lookup race

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

Hi folks,

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

To address this bug, patch #1 makes get_initiator_node_acl
lookup obtain ->acl_kref while ->acl_node_mutex is held,
and uses existing core_tpg_del_initiator_node_acl() logic
for shutdown based on struct kref + struct completion.

Also while auditing existing se_node_acl lookup users,
there is one particular case in target-core during session
queue depth change where lookup is completely unnecessary.
Go ahead and drop this pointless lookup in patch #2.

Finally, convert the last two remaining fabric drivers
that once upon a time where using some manner of internal
or quasi internal driver methods for node acl lookup.
Do this for tcm_fc + ib_srpt drivers in patch #3 + #4.

Please review,

--nab

Nicholas Bellinger (4):
  target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
  target: Remove useless set_initiator_node_queue_depth acl lookup
  tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage
  ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage

 drivers/infiniband/ulp/srpt/ib_srpt.c        | 78 ++++++----------------------
 drivers/infiniband/ulp/srpt/ib_srpt.h        |  2 -
 drivers/target/iscsi/iscsi_target_configfs.c |  6 +--
 drivers/target/iscsi/iscsi_target_tpg.c      | 10 ----
 drivers/target/iscsi/iscsi_target_tpg.h      |  2 -
 drivers/target/target_core_tpg.c             | 29 ++++-------
 drivers/target/target_core_transport.c       | 18 ++++---
 drivers/target/tcm_fc/tfc_conf.c             | 26 +++-------
 drivers/target/tcm_fc/tfc_sess.c             | 18 ++++---
 include/target/target_core_fabric.h          |  2 +-
 10 files changed, 61 insertions(+), 130 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1304222 — [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

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

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       |  6 ++++++
 drivers/target/target_core_transport.c | 18 ++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 62103a8..fb77fe1 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -78,6 +78,12 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
 
 	mutex_lock(&tpg->acl_node_mutex);
 	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
+	/*
+	 * Obtain the acl_kref now, which will be dropped upon the
+	 * release of se_sess memory within transport_free_session().
+	 */
+	if (acl)
+		kref_get(&acl->acl_kref);
 	mutex_unlock(&tpg->acl_node_mutex);
 
 	return acl;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index eb7aaf0..81cc699 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

[toc] | [prev] | [next] | [standalone]

#1304251 — Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

>  	mutex_lock(&tpg->acl_node_mutex);
>  	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> +	/*
> +	 * Obtain the acl_kref now, which will be dropped upon the
> +	 * release of se_sess memory within transport_free_session().
> +	 */
> +	if (acl)
> +		kref_get(&acl->acl_kref);

І think the comment is highly confusing as it's about one of the
callers, while the function has many.

I'd suggest you move it to core_tpg_check_initiator_node_acl instead.

Also I think iscsit_build_sendtargets_response will need a put on
the nacl, otherwise you'll leak references.

While we're at it - is there any god reason to keep acl_pr_ref_count
as a separate entity from acl_kref?

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

Whats the point of zeroing se_node_acl just before freeing se_sess?

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

The comment neds to move to transport_free_session.  Or maybe just removed
given that it's obvious from the code flow.

[toc] | [prev] | [next] | [standalone]

#1304262 — Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

On Fri, Jan 08, 2016 at 09:31:14AM +0100, Bart Van Assche wrote:
> Indeed. All error paths in all target drivers will have to be modified to 
> avoid that an acl reference leak is triggered if transport_init_session() 
> fails after core_tpg_check_initiator_node_acl() succeeded.

I'm still hoping for a nice helper that does transport_init_session +
core_tpg_check_initiator_node_acl + transport_register_session to
isolated all that.  It might need a callout to the driver somewhere
to be flexible enough but still would be a huge win..

[toc] | [prev] | [next] | [standalone]

#1304273 — Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

On Fri, 2016-01-08 at 09:31 +0100, Bart Van Assche wrote:
> On 01/08/2016 09:14 AM, Christoph Hellwig wrote:
> >>   	mutex_lock(&tpg->acl_node_mutex);
> >>   	acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
> >> +	/*
> >> +	 * Obtain the acl_kref now, which will be dropped upon the
> >> +	 * release of se_sess memory within transport_free_session().
> >> +	 */
> >> +	if (acl)
> >> +		kref_get(&acl->acl_kref);
> >
> > І think the comment is highly confusing as it's about one of the
> > callers, while the function has many.
> >
> > I'd suggest you move it to core_tpg_check_initiator_node_acl instead.
> >
> > Also I think iscsit_build_sendtargets_response will need a put on
> > the nacl, otherwise you'll leak references.
> 
> Indeed. All error paths in all target drivers will have to be modified 
> to avoid that an acl reference leak is triggered if 
> transport_init_session() fails after core_tpg_check_initiator_node_acl() 
> succeeded.
> 

Actually no, they do not.  That's the way that everything outside of
tcm_fc + ib_srpt driver code has already worked for a long time.

[toc] | [prev] | [next] | [standalone]

#1304292 — Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

On Fri, Jan 08, 2016 at 12:47:39AM -0800, Nicholas A. Bellinger wrote:
> Actually no, they do not.  That's the way that everything outside of
> tcm_fc + ib_srpt driver code has already worked for a long time.

Another reason to introduce a helper to enforce that ordering!

Everything but iscsi and qla2xxx is absolutely trivial to convert.
qla2xxx needs some work, but I think it's actually wrong currently
as it sets the s_id and loop_id unconditionally even if we're
reusing an existing node ACL.  iscsi is black magic as usual, so I'm
a little lost..

[toc] | [prev] | [next] | [standalone]

#1304312 — Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl

On Fri, Jan 08, 2016 at 10:08:46AM +0100, Christoph Hellwig wrote:
> Another reason to introduce a helper to enforce that ordering!
> 
> Everything but iscsi and qla2xxx is absolutely trivial to convert.
> qla2xxx needs some work, but I think it's actually wrong currently
> as it sets the s_id and loop_id unconditionally even if we're
> reusing an existing node ACL.  iscsi is black magic as usual, so I'm
> a little lost..

FYI, here is the patch for the trivial cases.  Those needing the
tag allocator will need a little bit more work first


diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 4fb0eca..3df1b21 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -806,54 +806,33 @@ static int tcm_loop_make_nexus(
 	struct tcm_loop_tpg *tl_tpg,
 	const char *name)
 {
-	struct se_portal_group *se_tpg;
 	struct tcm_loop_hba *tl_hba = tl_tpg->tl_hba;
 	struct tcm_loop_nexus *tl_nexus;
-	int ret = -ENOMEM;
 
 	if (tl_tpg->tl_nexus) {
 		pr_debug("tl_tpg->tl_nexus already exists\n");
 		return -EEXIST;
 	}
-	se_tpg = &tl_tpg->tl_se_tpg;
 
 	tl_nexus = kzalloc(sizeof(struct tcm_loop_nexus), GFP_KERNEL);
 	if (!tl_nexus) {
 		pr_err("Unable to allocate struct tcm_loop_nexus\n");
 		return -ENOMEM;
 	}
-	/*
-	 * Initialize the struct se_session pointer
-	 */
-	tl_nexus->se_sess = transport_init_session(
+
+	tl_nexus->se_sess = target_alloc_session(&tl_tpg->tl_se_tpg, name,
+				tl_nexus,
 				TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS);
 	if (IS_ERR(tl_nexus->se_sess)) {
-		ret = PTR_ERR(tl_nexus->se_sess);
-		goto out;
-	}
-	/*
-	 * Since we are running in 'demo mode' this call with generate a
-	 * struct se_node_acl for the tcm_loop struct se_portal_group with the SCSI
-	 * Initiator port name of the passed configfs group 'name'.
-	 */
-	tl_nexus->se_sess->se_node_acl = core_tpg_check_initiator_node_acl(
-				se_tpg, (unsigned char *)name);
-	if (!tl_nexus->se_sess->se_node_acl) {
-		transport_free_session(tl_nexus->se_sess);
-		goto out;
+		kfree(tl_nexus);
+		return PTR_ERR(tl_nexus->se_sess);
 	}
-	/* Now, register the I_T Nexus as active. */
-	transport_register_session(se_tpg, tl_nexus->se_sess->se_node_acl,
-			tl_nexus->se_sess, tl_nexus);
+
 	tl_tpg->tl_nexus = tl_nexus;
 	pr_debug("TCM_Loop_ConfigFS: Established I_T Nexus to emulated"
 		" %s Initiator Port: %s\n", tcm_loop_dump_proto_id(tl_hba),
 		name);
 	return 0;
-
-out:
-	kfree(tl_nexus);
-	return ret;
 }
 
 static int tcm_loop_drop_nexus(
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 35f7d31..371d538 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -198,45 +198,28 @@ static struct sbp_session *sbp_session_create(
 	struct sbp_session *sess;
 	int ret;
 	char guid_str[17];
-	struct se_node_acl *se_nacl;
+
+	snprintf(guid_str, sizeof(guid_str), "%016llx", guid);
 
 	sess = kmalloc(sizeof(*sess), GFP_KERNEL);
 	if (!sess) {
 		pr_err("failed to allocate session descriptor\n");
 		return ERR_PTR(-ENOMEM);
 	}
+	spin_lock_init(&sess->lock);
+	INIT_LIST_HEAD(&sess->login_list);
+	INIT_DELAYED_WORK(&sess->maint_work, session_maintenance_work);
+	sess->guid = guid;
 
-	sess->se_sess = transport_init_session(TARGET_PROT_NORMAL);
+	sess->se_sess = target_alloc_session(&tpg->se_tpg, guid_str, sess,
+			TARGET_PROT_NORMAL);
 	if (IS_ERR(sess->se_sess)) {
 		pr_err("failed to init se_session\n");
-
 		ret = PTR_ERR(sess->se_sess);
 		kfree(sess);
 		return ERR_PTR(ret);
 	}
 
-	snprintf(guid_str, sizeof(guid_str), "%016llx", guid);
-
-	se_nacl = core_tpg_check_initiator_node_acl(&tpg->se_tpg, guid_str);
-	if (!se_nacl) {
-		pr_warn("Node ACL not found for %s\n", guid_str);
-
-		transport_free_session(sess->se_sess);
-		kfree(sess);
-
-		return ERR_PTR(-EPERM);
-	}
-
-	sess->se_sess->se_node_acl = se_nacl;
-
-	spin_lock_init(&sess->lock);
-	INIT_LIST_HEAD(&sess->login_list);
-	INIT_DELAYED_WORK(&sess->maint_work, session_maintenance_work);
-
-	sess->guid = guid;
-
-	transport_register_session(&tpg->se_tpg, se_nacl, sess->se_sess, sess);
-
 	return sess;
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4fdcee2..f3074dd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -375,6 +375,27 @@ void transport_register_session(
 }
 EXPORT_SYMBOL(transport_register_session);
 
+struct se_session *target_alloc_session(struct se_portal_group *tpg,
+		const char *name, void *private, enum target_prot_op prot_op)
+{
+	struct se_session *sess;
+
+	sess = transport_init_session(prot_op);
+	if (IS_ERR(sess))
+		return sess;
+
+	sess->se_node_acl = core_tpg_check_initiator_node_acl(tpg,
+			(unsigned char *)name);
+	if (!sess->se_node_acl) {
+		transport_free_session(sess);
+		return ERR_PTR(-EACCES);
+	}
+
+	transport_register_session(tpg, sess->se_node_acl, sess, private);
+	return sess;
+}
+EXPORT_SYMBOL(target_alloc_session);
+
 static void target_release_session(struct kref *kref)
 {
 	struct se_session *se_sess = container_of(kref,
diff --git a/drivers/usb/gadget/legacy/tcm_usb_gadget.c b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
index 22e5615..48b661f 100644
--- a/drivers/usb/gadget/legacy/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
@@ -1541,7 +1541,6 @@ out:
 
 static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 {
-	struct se_portal_group *se_tpg;
 	struct tcm_usbg_nexus *tv_nexus;
 	int ret;
 
@@ -1549,44 +1548,24 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 	if (tpg->tpg_nexus) {
 		ret = -EEXIST;
 		pr_debug("tpg->tpg_nexus already exists\n");
-		goto err_unlock;
+		goto out_unlock;
 	}
-	se_tpg = &tpg->se_tpg;
 
 	ret = -ENOMEM;
 	tv_nexus = kzalloc(sizeof(*tv_nexus), GFP_KERNEL);
 	if (!tv_nexus)
-		goto err_unlock;
-	tv_nexus->tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
-	if (IS_ERR(tv_nexus->tvn_se_sess))
-		goto err_free;
+		goto out_unlock;
 
-	/*
-	 * Since we are running in 'demo mode' this call with generate a
-	 * struct se_node_acl for the tcm_vhost struct se_portal_group with
-	 * the SCSI Initiator port name of the passed configfs group 'name'.
-	 */
-	tv_nexus->tvn_se_sess->se_node_acl = core_tpg_check_initiator_node_acl(
-			se_tpg, name);
-	if (!tv_nexus->tvn_se_sess->se_node_acl) {
-		pr_debug("core_tpg_check_initiator_node_acl() failed"
-				" for %s\n", name);
-		goto err_session;
+	tv_nexus->tvn_se_sess = target_alloc_session(&tpg->se_tpg, name,
+			tv_nexus, TARGET_PROT_NORMAL);
+	if (IS_ERR(tv_nexus->tvn_se_sess)) {
+		kfree(tv_nexus);
+		goto out_unlock;
 	}
-	/*
-	 * Now register the TCM vHost virtual I_T Nexus as active.
-	 */
-	transport_register_session(se_tpg, tv_nexus->tvn_se_sess->se_node_acl,
-			tv_nexus->tvn_se_sess, tv_nexus);
-	tpg->tpg_nexus = tv_nexus;
-	mutex_unlock(&tpg->tpg_mutex);
-	return 0;
 
-err_session:
-	transport_free_session(tv_nexus->tvn_se_sess);
-err_free:
-	kfree(tv_nexus);
-err_unlock:
+	tpg->tpg_nexus = tv_nexus;
+	ret = 0;
+out_unlock:
 	mutex_unlock(&tpg->tpg_mutex);
 	return ret;
 }
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..f09c30c 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1485,58 +1485,34 @@ static struct configfs_attribute *scsiback_param_attrs[] = {
 static int scsiback_make_nexus(struct scsiback_tpg *tpg,
 				const char *name)
 {
-	struct se_portal_group *se_tpg;
-	struct se_session *se_sess;
 	struct scsiback_nexus *tv_nexus;
+	int ret = 0;
 
 	mutex_lock(&tpg->tv_tpg_mutex);
 	if (tpg->tpg_nexus) {
-		mutex_unlock(&tpg->tv_tpg_mutex);
 		pr_debug("tpg->tpg_nexus already exists\n");
-		return -EEXIST;
+		ret = -EEXIST;
+		goto out_unlock;
 	}
-	se_tpg = &tpg->se_tpg;
 
 	tv_nexus = kzalloc(sizeof(struct scsiback_nexus), GFP_KERNEL);
 	if (!tv_nexus) {
-		mutex_unlock(&tpg->tv_tpg_mutex);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_unlock;
 	}
-	/*
-	 * Initialize the struct se_session pointer
-	 */
-	tv_nexus->tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
+
+	tv_nexus->tvn_se_sess = target_alloc_session(&tpg->se_tpg, name,
+			tv_nexus, TARGET_PROT_NORMAL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
-		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(tv_nexus);
-		return -ENOMEM;
-	}
-	se_sess = tv_nexus->tvn_se_sess;
-	/*
-	 * Since we are running in 'demo mode' this call with generate a
-	 * struct se_node_acl for the scsiback struct se_portal_group with
-	 * the SCSI Initiator port name of the passed configfs group 'name'.
-	 */
-	tv_nexus->tvn_se_sess->se_node_acl = core_tpg_check_initiator_node_acl(
-				se_tpg, (unsigned char *)name);
-	if (!tv_nexus->tvn_se_sess->se_node_acl) {
-		mutex_unlock(&tpg->tv_tpg_mutex);
-		pr_debug("core_tpg_check_initiator_node_acl() failed for %s\n",
-			 name);
-		goto out;
+		ret = -ENOMEM;
+		goto out_unlock;
 	}
-	/* Now register the TCM pvscsi virtual I_T Nexus as active. */
-	transport_register_session(se_tpg, tv_nexus->tvn_se_sess->se_node_acl,
-			tv_nexus->tvn_se_sess, tv_nexus);
-	tpg->tpg_nexus = tv_nexus;
 
+	tpg->tpg_nexus = tv_nexus;
+out_unlock:
 	mutex_unlock(&tpg->tv_tpg_mutex);
-	return 0;
-
-out:
-	transport_free_session(se_sess);
-	kfree(tv_nexus);
-	return -ENOMEM;
+	return ret;
 }
 
 static int scsiback_drop_nexus(struct scsiback_tpg *tpg)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 7fb2557..bbf7fb5 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -108,6 +108,9 @@ void target_unregister_template(const struct target_core_fabric_ops *fo);
 int target_depend_item(struct config_item *item);
 void target_undepend_item(struct config_item *item);
 
+struct se_session *target_alloc_session(struct se_portal_group *tpg,
+		const char *name, void *private, enum target_prot_op prot_op);
+
 struct se_session *transport_init_session(enum target_prot_op);
 int transport_alloc_session_tags(struct se_session *, unsigned int,
 		unsigned int);

[toc] | [prev] | [next] | [standalone]

#1304223 — [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup

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

With the changes in place to obtain se_node_acl->acl_kref
from within core_tpg_del_initiator_node_acl() and auditing
existing users, it's clear there is no need to perform the
lookup during core_tpg_set_initiator_node_queue_depth().

This is because se_node_acl->acl_group is already protecting
the se_node_acl reference via configfs, and ->acl_group
shutdown in core_tpg_del_initiator_node_acl() can't occur
until core_tpg_set_initiator_node_queue_depth() completes.

Also, remove a related pointless wrapper in iscsi-target.

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 |  6 +++---
 drivers/target/iscsi/iscsi_target_tpg.c      | 10 ----------
 drivers/target/iscsi/iscsi_target_tpg.h      |  2 --
 drivers/target/target_core_tpg.c             | 23 ++++-------------------
 include/target/target_core_fabric.h          |  2 +-
 5 files changed, 8 insertions(+), 35 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 255204c..6469321 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),
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 fb77fe1..550d6f8 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -371,30 +371,18 @@ 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;
 	unsigned long flags;
 	int dynamic_acl = 0;
 
-	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);
 
 	spin_lock_irqsave(&tpg->session_lock, flags);
 	list_for_each_entry(sess, &tpg->tpg_sess_list, sess_list) {
@@ -407,13 +395,12 @@ 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);
+				tpg->se_tpg_tfo->get_fabric_name(),
+				acl->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);
 			return -EEXIST;
 		}
 		/*
@@ -464,13 +451,11 @@ int core_tpg_set_initiator_node_queue_depth(
 
 	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(),
+		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;
 }
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index de21130..7f83295 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -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

[toc] | [prev] | [next] | [standalone]

#1304252 — Re: [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup

On Fri, Jan 08, 2016 at 07:15:46AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> With the changes in place to obtain se_node_acl->acl_kref
> from within core_tpg_del_initiator_node_acl() and auditing
> existing users, it's clear there is no need to perform the
> lookup during core_tpg_set_initiator_node_queue_depth().
> 
> This is because se_node_acl->acl_group is already protecting
> the se_node_acl reference via configfs, and ->acl_group
> shutdown in core_tpg_del_initiator_node_acl() can't occur
> until core_tpg_set_initiator_node_queue_depth() completes.
> 
> Also, remove a related pointless wrapper in iscsi-target.

While we're at it, can you please remove the always true
force argument from core_tpg_set_initiator_node_queue_depth and rename
the funcion to something like target_set_initiator_node_queue_depth.

Btw, what's the use case for modifying this on a 'live' session that
gets shutdown for that purpose?  The whole algorithm looks somewhat
fishy to me to be honest.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web