diff mbox

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

Message ID 1452237348-2277-2-git-send-email-nab@daterainc.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Nicholas A. Bellinger Jan. 8, 2016, 7:15 a.m. UTC 
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(-)

Comments

Christoph Hellwig Jan. 8, 2016, 8:14 a.m. UTC | #1 
>  	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.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Jan. 8, 2016, 8:31 a.m. UTC | #2 
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.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Jan. 8, 2016, 8:35 a.m. UTC | #3 
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..
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger Jan. 8, 2016, 8:47 a.m. UTC | #4 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Jan. 8, 2016, 9:08 a.m. UTC | #5 
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..
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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