[PATCH] tcm_qla2xxx: Don't insert nacls without sessions into the btree

[PATCH] tcm_qla2xxx: Don't insert nacls without sessions into the btree

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

When we create an explicit node ACL in tcm_qla2xxx_make_nodeacl(),
there is a call to tcm_qla2xxx_setup_nacl_from_rport(), which puts the
node ACL into the lport_fcport_map even though there is no session yet
for the initiator.  Since the only time we remove entries from this
map is when we free a session, this means that if we later delete this
node ACL without the initiator ever creating a session, we'll leave
the nacl pointer in the btree pointing at freed memory.

This is especially bad if that initiator later does send us a command
that would cause us to create a dynamic ACL and session: we'll find
the stale freed nacl pointer in the btree and end up with use-after-free.

We could add more code to clear the btree entry when deleting the
explicit nacl, but the original insertion is pointless: without a
session attached, we'll just have to update the entry when a session
appears anyway.  So we can just delete tcm_qla2xxx_setup_nacl_from_rport()
and the code that calls it.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
Hi Nick, Chad,

Not sure how you guys want to handle merging qla2xxx target patches,
especially ones that only touch the tcm_qla2xxx code, so I'm just
sending this to both of you.

 - R.

 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   73 ------------------------------------
 1 file changed, 73 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 436598f..8ece6fd 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -762,65 +762,6 @@ static u16 tcm_qla2xxx_set_fabric_sense_len(struct se_cmd *se_cmd,
 struct target_fabric_configfs *tcm_qla2xxx_fabric_configfs;
 struct target_fabric_configfs *tcm_qla2xxx_npiv_fabric_configfs;
 
-static int tcm_qla2xxx_setup_nacl_from_rport(
-	struct se_portal_group *se_tpg,
-	struct se_node_acl *se_nacl,
-	struct tcm_qla2xxx_lport *lport,
-	struct tcm_qla2xxx_nacl *nacl,
-	u64 rport_wwnn)
-{
-	struct scsi_qla_host *vha = lport->qla_vha;
-	struct Scsi_Host *sh = vha->host;
-	struct fc_host_attrs *fc_host = shost_to_fc_host(sh);
-	struct fc_rport *rport;
-	unsigned long flags;
-	void *node;
-	int rc;
-
-	/*
-	 * Scan the existing rports, and create a session for the
-	 * explict NodeACL is an matching rport->node_name already
-	 * exists.
-	 */
-	spin_lock_irqsave(sh->host_lock, flags);
-	list_for_each_entry(rport, &fc_host->rports, peers) {
-		if (rport_wwnn != rport->node_name)
-			continue;
-
-		pr_debug("Located existing rport_wwpn and rport->node_name: 0x%016LX, port_id: 0x%04x\n",
-		    rport->node_name, rport->port_id);
-		nacl->nport_id = rport->port_id;
-
-		spin_unlock_irqrestore(sh->host_lock, flags);
-
-		spin_lock_irqsave(&vha->hw->hardware_lock, flags);
-		node = btree_lookup32(&lport->lport_fcport_map, rport->port_id);
-		if (node) {
-			rc = btree_update32(&lport->lport_fcport_map,
-					    rport->port_id, se_nacl);
-		} else {
-			rc = btree_insert32(&lport->lport_fcport_map,
-					    rport->port_id, se_nacl,
-					    GFP_ATOMIC);
-		}
-		spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
-
-		if (rc) {
-			pr_err("Unable to insert se_nacl into fcport_map");
-			WARN_ON(rc > 0);
-			return rc;
-		}
-
-		pr_debug("Inserted into fcport_map: %p for WWNN: 0x%016LX, port_id: 0x%08x\n",
-		    se_nacl, rport_wwnn, nacl->nport_id);
-
-		return 1;
-	}
-	spin_unlock_irqrestore(sh->host_lock, flags);
-
-	return 0;
-}
-
 /*
  * Expected to be called with struct qla_hw_data->hardware_lock held
  */
@@ -859,14 +800,10 @@ static struct se_node_acl *tcm_qla2xxx_make_nodeacl(
 	struct config_group *group,
 	const char *name)
 {
-	struct se_wwn *se_wwn = se_tpg->se_tpg_wwn;
-	struct tcm_qla2xxx_lport *lport = container_of(se_wwn,
-				struct tcm_qla2xxx_lport, lport_wwn);
 	struct se_node_acl *se_nacl, *se_nacl_new;
 	struct tcm_qla2xxx_nacl *nacl;
 	u64 wwnn;
 	u32 qla2xxx_nexus_depth;
-	int rc;
 
 	if (tcm_qla2xxx_parse_wwn(name, &wwnn, 1) < 0)
 		return ERR_PTR(-EINVAL);
@@ -893,16 +830,6 @@ static struct se_node_acl *tcm_qla2xxx_make_nodeacl(
 	nacl = container_of(se_nacl, struct tcm_qla2xxx_nacl, se_node_acl);
 	nacl->nport_wwnn = wwnn;
 	tcm_qla2xxx_format_wwn(&nacl->nport_name[0], TCM_QLA2XXX_NAMELEN, wwnn);
-	/*
-	 * Setup a se_nacl handle based on an a matching struct fc_rport setup
-	 * via drivers/scsi/qla2xxx/qla_init.c:qla2x00_reg_remote_port()
-	 */
-	rc = tcm_qla2xxx_setup_nacl_from_rport(se_tpg, se_nacl, lport,
-					nacl, wwnn);
-	if (rc < 0) {
-		tcm_qla2xxx_release_fabric_acl(se_tpg, se_nacl_new);
-		return ERR_PTR(rc);
-	}
 
 	return se_nacl;
 }
-- 
1.7.9.5

Re: [PATCH] tcm_qla2xxx: Don't insert nacls without sessions into the btree

[Nicholas A. Bellinger]

Hey Roland,

On Mon, 2012-06-04 at 23:37 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> When we create an explicit node ACL in tcm_qla2xxx_make_nodeacl(),
> there is a call to tcm_qla2xxx_setup_nacl_from_rport(), which puts the
> node ACL into the lport_fcport_map even though there is no session yet
> for the initiator.  Since the only time we remove entries from this
> map is when we free a session, this means that if we later delete this
> node ACL without the initiator ever creating a session, we'll leave
> the nacl pointer in the btree pointing at freed memory.
> 
> This is especially bad if that initiator later does send us a command
> that would cause us to create a dynamic ACL and session: we'll find
> the stale freed nacl pointer in the btree and end up with use-after-free.
> 
> We could add more code to clear the btree entry when deleting the
> explicit nacl, but the original insertion is pointless: without a
> session attached, we'll just have to update the entry when a session
> appears anyway.  So we can just delete tcm_qla2xxx_setup_nacl_from_rport()
> and the code that calls it.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>

Looking at this patch which removes tcm_qla2xxx_setup_nacl_from_rport()
usage during explicit tcm_qla2xxx_make_nodeacl() creation, how does an
explicit NodeACL get picked up in tcm_qla2xxx_check_initiator_node_acl()
now with this change ?

AFAICT this patch makes every new initiator login attempt result in a
demo-mode se_node_acl being generated with virtual LUN=0 access, and
ignores explicit se_node_acl + MappedLUNs groups + links to TPG_LUN in
individual ../target/qla2xxx/$TARGET_WWPN/tpgt_1/acls/$INITIATOR_WWPN/

Is there something else that I'm missing here..?

> ---
> Hi Nick, Chad,
> 
> Not sure how you guys want to handle merging qla2xxx target patches,
> especially ones that only touch the tcm_qla2xxx code, so I'm just
> sending this to both of you.
> 

Just a heads up that I'm planning to soon deprecate lio-core.git and
start using target-pending.git as the main development tree as
recommended by hch following a branch structure similar to what
virt/kvm/kvm.git has adopted.

To that end, I'm thinking that a target-pending/qla-tgt-queue branch
that's merged into for-next makes the most sense so that Chad + Co. can
pickup the latest qla-tgt development items ASAP for their internal
regression testing.

For tcm_qla2xxx specific patches, I'm fine with having these go via
target-pending through the usual for-next / rc-fixes updates to Linus
with the necessary signoffs.

For qla_target.c items beyond mechanical + minor changes we'll require
an Qlogic review + ACK, and likely end up merging via scsi.git as
necessary if the change effects qla2xxx LLD initiator mode operation.

Chad & Co, do you have any thoughts on your preferred workflow here..?

Thanks,

--nab

Re: [PATCH] tcm_qla2xxx: Don't insert nacls without sessions into the btree

[Roland Dreier]

> Looking at this patch which removes tcm_qla2xxx_setup_nacl_from_rport()
> usage during explicit tcm_qla2xxx_make_nodeacl() creation, how does an
> explicit NodeACL get picked up in tcm_qla2xxx_check_initiator_node_acl()
> now with this change ?
>
> AFAICT this patch makes every new initiator login attempt result in a
> demo-mode se_node_acl being generated with virtual LUN=0 access, and
> ignores explicit se_node_acl + MappedLUNs groups + links to TPG_LUN in
> individual ../target/qla2xxx/$TARGET_WWPN/tpgt_1/acls/$INITIATOR_WWPN/
>
> Is there something else that I'm missing here..?

[Reply to all this time]

We still insert the nacl into the tpg->acl_node_list so we'll find it when we
do the check when the session actually shows up.

Inserting the nacl into the btree doesn't do anything, since it would have no
session pointer and so tcm_qla2xxx_find_sess_by_s_id() will return NULL
whether or not we insert the nacl in tcm_qla2xxx_setup_nacl_from_rport().
(And that's the only place that does a btree lookup)

 - R.

Re: [PATCH] tcm_qla2xxx: Don't insert nacls without sessions into the btree

[Nicholas A. Bellinger]

On Tue, 2012-06-05 at 16:41 -0700, Roland Dreier wrote:
> > Looking at this patch which removes tcm_qla2xxx_setup_nacl_from_rport()
> > usage during explicit tcm_qla2xxx_make_nodeacl() creation, how does an
> > explicit NodeACL get picked up in tcm_qla2xxx_check_initiator_node_acl()
> > now with this change ?
> >
> > AFAICT this patch makes every new initiator login attempt result in a
> > demo-mode se_node_acl being generated with virtual LUN=0 access, and
> > ignores explicit se_node_acl + MappedLUNs groups + links to TPG_LUN in
> > individual ../target/qla2xxx/$TARGET_WWPN/tpgt_1/acls/$INITIATOR_WWPN/
> >
> > Is there something else that I'm missing here..?
> 
> [Reply to all this time]
> 
> We still insert the nacl into the tpg->acl_node_list so we'll find it when we
> do the check when the session actually shows up.
> 

Err, yes of course..

> Inserting the nacl into the btree doesn't do anything, since it would have no
> session pointer and so tcm_qla2xxx_find_sess_by_s_id() will return NULL
> whether or not we insert the nacl in tcm_qla2xxx_setup_nacl_from_rport().
> (And that's the only place that does a btree lookup)
> 

OK, I've applied this patch to lio-core.git on top of the following
tcm_qla2xxx patches that where not included in the initial merge:

commit dfebe3b51dd3e263fdc6151e3f1ec9aba1716d03
Author: Joern Engel <joern@logfs.org>
Date:   Fri May 18 13:58:23 2012 -0700

    tcm_qla2xxx: Convert to TFO->put_session() usage

commit 6fc162d3f4a4647576e5dad23435e505a8ef0dc3
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Fri May 18 15:37:53 2012 -0700

    tcm_qla2xxx: Clear session s_id + loop_id earlier during shutdown


I'll include these over the next days into target-pending as
3.5-rc-fixes items.

Thanks Roland!

--nab