* [PATCH 0/4] target: Close se_node_acl lookup race
@ 2016-01-08 7:15 Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl Nicholas A. Bellinger
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 7:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 7:15 [PATCH 0/4] target: Close se_node_acl lookup race Nicholas A. Bellinger
@ 2016-01-08 7:15 ` Nicholas A. Bellinger
2016-01-08 8:14 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup Nicholas A. Bellinger
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 7:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas 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, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup
2016-01-08 7:15 [PATCH 0/4] target: Close se_node_acl lookup race Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl Nicholas A. Bellinger
@ 2016-01-08 7:15 ` Nicholas A. Bellinger
2016-01-08 8:17 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 4/4] ib_srpt: " Nicholas A. Bellinger
3 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 7:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 7:15 [PATCH 0/4] target: Close se_node_acl lookup race Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup Nicholas A. Bellinger
@ 2016-01-08 7:15 ` Nicholas A. Bellinger
2016-01-08 8:21 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 4/4] ib_srpt: " Nicholas A. Bellinger
3 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 7:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 7:15 [PATCH 0/4] target: Close se_node_acl lookup race Nicholas A. Bellinger
` (2 preceding siblings ...)
2016-01-08 7:15 ` [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage Nicholas A. Bellinger
@ 2016-01-08 7:15 ` Nicholas A. Bellinger
2016-01-08 8:52 ` Bart Van Assche
3 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 7:15 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas 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, 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 | 78 +++++++----------------------------
drivers/infiniband/ulp/srpt/ib_srpt.h | 2 -
2 files changed, 16 insertions(+), 64 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2e2fe81..0a6ca2d 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,7 +2387,7 @@ 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;
@@ -2565,6 +2540,15 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
" RTR failed (error code = %d)\n", ret);
goto destroy_ib;
}
+
+ ch->sess = transport_init_session(TARGET_PROT_NORMAL);
+ if (IS_ERR(ch->sess)) {
+ rej->reason = cpu_to_be32(
+ SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
+ pr_debug("Failed to create session\n");
+ goto destroy_ib;
+ }
+
/*
* Use the initator port identifier as the session name.
*/
@@ -2574,24 +2558,18 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
pr_debug("registering session %s\n", ch->sess_name);
- nacl = srpt_lookup_acl(sport, ch->i_port_id);
- if (!nacl) {
+ se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
+ if (!se_acl) {
pr_info("Rejected login because no ACL has been"
" configured yet for initiator %s.\n", ch->sess_name);
rej->reason = cpu_to_be32(
- SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+ SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+ transport_free_session(ch->sess);
goto destroy_ib;
}
+ ch->sess->se_node_acl = se_acl;
- ch->sess = transport_init_session(TARGET_PROT_NORMAL);
- if (IS_ERR(ch->sess)) {
- rej->reason = cpu_to_be32(
- SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
- pr_debug("Failed to create session\n");
- goto deregister_session;
- }
- ch->sess->se_node_acl = &nacl->nacl;
- transport_register_session(&sport->port_tpg_1, &nacl->nacl, ch->sess, ch);
+ 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 +2613,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 +3249,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 +3496,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 +3775,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 7:15 ` [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl Nicholas A. Bellinger
@ 2016-01-08 8:14 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 8:14 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
@ 2016-01-08 8:14 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 8:14 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup
2016-01-08 7:15 ` [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup Nicholas A. Bellinger
@ 2016-01-08 8:17 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 8:17 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 7:15 ` [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage Nicholas A. Bellinger
@ 2016-01-08 8:21 ` Christoph Hellwig
2016-01-08 9:05 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 8:21 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
On Fri, Jan 08, 2016 at 07:15:47AM +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.
>
> 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 se_portal_group *se_tpg = &tpg->se_tpg;
> struct se_node_acl *se_acl;
> + unsigned char initiatorname[TRANSPORT_IQN_LEN];
>
> + 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);
I'd say kill this function and opencode it in ft_sess_create. It
shouldn't be used elsewhere, and ft_sess_create really wants the
se_node_acl and not the ft_node_acl anyway.
Btw, who is dropping the reference we're acuiring through
core_tpg_get_initiator_node_acl?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 8:14 ` Christoph Hellwig
(?)
@ 2016-01-08 8:31 ` Bart Van Assche
2016-01-08 8:35 ` Christoph Hellwig
2016-01-08 8:47 ` Nicholas A. Bellinger
-1 siblings, 2 replies; 18+ messages in thread
From: Bart Van Assche @ 2016-01-08 8:31 UTC (permalink / raw)
To: Christoph Hellwig, Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Hannes Reinecke,
Andy Grover, Vasu Dev, Vu Pham, Nicholas Bellinger
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 8:31 ` Bart Van Assche
@ 2016-01-08 8:35 ` Christoph Hellwig
2016-01-08 8:47 ` Nicholas A. Bellinger
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 8:35 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, Nicholas A. Bellinger, target-devel,
linux-scsi, lkml, Sagi Grimberg, Hannes Reinecke, Andy Grover,
Vasu Dev, Vu Pham, Nicholas Bellinger
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..
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 8:31 ` Bart Van Assche
2016-01-08 8:35 ` Christoph Hellwig
@ 2016-01-08 8:47 ` Nicholas A. Bellinger
2016-01-08 9:08 ` Christoph Hellwig
1 sibling, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 8:47 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, Nicholas A. Bellinger, target-devel,
linux-scsi, lkml, Sagi Grimberg, Hannes Reinecke, Andy Grover,
Vasu Dev, Vu Pham
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 7:15 ` [PATCH 4/4] ib_srpt: " Nicholas A. Bellinger
@ 2016-01-08 8:52 ` Bart Van Assche
2016-01-08 9:17 ` Nicholas A. Bellinger
0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2016-01-08 8:52 UTC (permalink / raw)
To: Nicholas A. Bellinger, target-devel
Cc: linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
On 01/08/2016 08:15 AM, Nicholas A. Bellinger wrote:
> @@ -2574,24 +2558,18 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
>
> pr_debug("registering session %s\n", ch->sess_name);
>
> - nacl = srpt_lookup_acl(sport, ch->i_port_id);
> - if (!nacl) {
> + se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
> + if (!se_acl) {
> pr_info("Rejected login because no ACL has been"
> " configured yet for initiator %s.\n", ch->sess_name);
> rej->reason = cpu_to_be32(
> - SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
> + SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
> + transport_free_session(ch->sess);
> goto destroy_ib;
> }
> + ch->sess->se_node_acl = se_acl;
This is a backwards-incompatible change. Today the ib_srpt target driver
accepts initiator port names with and without leading "0x". With this
change the "0x" prefix becomes mandatory.
> 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;
> };
With this patch applied the following srpt_node_acl members are no
longer read: i_port_id, sport and list. Hence please remove the
srpt_node_acl structure definition and use struct se_node_acl instead.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 8:21 ` Christoph Hellwig
@ 2016-01-08 9:05 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 9:05 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, lkml, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Vasu Dev, Vu Pham,
Nicholas Bellinger
On Fri, Jan 08, 2016 at 09:21:15AM +0100, Christoph Hellwig wrote:
> Btw, who is dropping the reference we're acuiring through
> core_tpg_get_initiator_node_acl?
Ok, we do in transport_deregister_session/transport_free_session.
But that means patches 3 and 4 need to go first, otherwise we fail
to grab a reference in tcm_fc and srpt that we later drop after patch 1.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 8:47 ` Nicholas A. Bellinger
@ 2016-01-08 9:08 ` Christoph Hellwig
2016-01-08 9:37 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 9:08 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Bart Van Assche, Christoph Hellwig, Nicholas A. Bellinger,
target-devel, linux-scsi, lkml, Sagi Grimberg, Hannes Reinecke,
Andy Grover, Vasu Dev, Vu Pham, Himanshu Madhani,
Giridhar Malavali
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..
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 8:52 ` Bart Van Assche
@ 2016-01-08 9:17 ` Nicholas A. Bellinger
2016-01-08 9:35 ` Bart Van Assche
0 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-08 9:17 UTC (permalink / raw)
To: Bart Van Assche
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, lkml,
Sagi Grimberg, Christoph Hellwig, Hannes Reinecke, Andy Grover,
Vasu Dev, Vu Pham
On Fri, 2016-01-08 at 09:52 +0100, Bart Van Assche wrote:
> On 01/08/2016 08:15 AM, Nicholas A. Bellinger wrote:
> > @@ -2574,24 +2558,18 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
> >
> > pr_debug("registering session %s\n", ch->sess_name);
> >
> > - nacl = srpt_lookup_acl(sport, ch->i_port_id);
> > - if (!nacl) {
> > + se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
> > + if (!se_acl) {
> > pr_info("Rejected login because no ACL has been"
> > " configured yet for initiator %s.\n", ch->sess_name);
> > rej->reason = cpu_to_be32(
> > - SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
> > + SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
> > + transport_free_session(ch->sess);
> > goto destroy_ib;
> > }
> > + ch->sess->se_node_acl = se_acl;
>
> This is a backwards-incompatible change. Today the ib_srpt target driver
> accepts initiator port names with and without leading "0x". With this
> change the "0x" prefix becomes mandatory.
The internally ib_srpt formatted ch->sess_name already needs to match
se_node_acl->initiatorname for se_node_acl->acl_group configfs group
shutdown reference..
How does this patch become a backworks-incompatible change for that..?
>
> > 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;
> > };
>
> With this patch applied the following srpt_node_acl members are no
> longer read: i_port_id, sport and list. Hence please remove the
> srpt_node_acl structure definition and use struct se_node_acl instead.
>
Dropping these now.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] ib_srpt: Convert acl lookup to modern get_initiator_node_acl usage
2016-01-08 9:17 ` Nicholas A. Bellinger
@ 2016-01-08 9:35 ` Bart Van Assche
0 siblings, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2016-01-08 9:35 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, lkml,
Sagi Grimberg, Christoph Hellwig, Hannes Reinecke, Andy Grover,
Vasu Dev, Vu Pham
On 01/08/2016 10:17 AM, Nicholas A. Bellinger wrote:
> On Fri, 2016-01-08 at 09:52 +0100, Bart Van Assche wrote:
>> On 01/08/2016 08:15 AM, Nicholas A. Bellinger wrote:
>>> @@ -2574,24 +2558,18 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
>>>
>>> pr_debug("registering session %s\n", ch->sess_name);
>>>
>>> - nacl = srpt_lookup_acl(sport, ch->i_port_id);
>>> - if (!nacl) {
>>> + se_acl = core_tpg_get_initiator_node_acl(&sport->port_tpg_1, ch->sess_name);
>>> + if (!se_acl) {
>>> pr_info("Rejected login because no ACL has been"
>>> " configured yet for initiator %s.\n", ch->sess_name);
>>> rej->reason = cpu_to_be32(
>>> - SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
>>> + SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
>>> + transport_free_session(ch->sess);
>>> goto destroy_ib;
>>> }
>>> + ch->sess->se_node_acl = se_acl;
>>
>> This is a backwards-incompatible change. Today the ib_srpt target driver
>> accepts initiator port names with and without leading "0x". With this
>> change the "0x" prefix becomes mandatory.
>
> The internally ib_srpt formatted ch->sess_name already needs to match
> se_node_acl->initiatorname for se_node_acl->acl_group configfs group
> shutdown reference..
>
> How does this patch become a backworks-incompatible change for that..?
Hello Nic,
Personally I'm not that worried about this change but I wanted to report
it anyway. The current algorithm is as follows:
- When a directory is created in configfs for an initiator, the
initiator name is parsed and stored in binary form in struct
srpt_node_acl. The parsing function accepts both initiator
names that start with a "0x" prefix and initiator names that
do not have that prefix.
- During login the initiator port ID provided by the initiator in
the SRP login request is compared with the binary initiator port ID
in struct srpt_node_acl.
The patch at the start of this e-mail thread changes this behavior
because core_tpg_get_initiator_node_acl() looks up the initiator port ID
in a list that contains the ASCII representations of the initiator port
ID. This means that the initiator port name will only be found by
core_tpg_get_initiator_node_acl() if the name format used in the mkdir
command matches the initiator name format used by the
core_tpg_get_initiator_node_acl() caller, this means with "0x" prefix.
Bart.
Bart.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl
2016-01-08 9:08 ` Christoph Hellwig
@ 2016-01-08 9:37 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2016-01-08 9:37 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Bart Van Assche, Christoph Hellwig, Nicholas A. Bellinger,
target-devel, linux-scsi, lkml, Sagi Grimberg, Hannes Reinecke,
Andy Grover, Vasu Dev, Vu Pham, Himanshu Madhani,
Giridhar Malavali
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);
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-01-08 9:37 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-08 7:15 [PATCH 0/4] target: Close se_node_acl lookup race Nicholas A. Bellinger
2016-01-08 7:15 ` [PATCH 1/4] target: Obtain se_node_acl->acl_kref during get_initiator_node_acl Nicholas A. Bellinger
2016-01-08 8:14 ` Christoph Hellwig
2016-01-08 8:14 ` Christoph Hellwig
2016-01-08 8:31 ` Bart Van Assche
2016-01-08 8:35 ` Christoph Hellwig
2016-01-08 8:47 ` Nicholas A. Bellinger
2016-01-08 9:08 ` Christoph Hellwig
2016-01-08 9:37 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 2/4] target: Remove useless set_initiator_node_queue_depth acl lookup Nicholas A. Bellinger
2016-01-08 8:17 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 3/4] tcm_fc: Convert acl lookup to modern get_initiator_node_acl usage Nicholas A. Bellinger
2016-01-08 8:21 ` Christoph Hellwig
2016-01-08 9:05 ` Christoph Hellwig
2016-01-08 7:15 ` [PATCH 4/4] ib_srpt: " Nicholas A. Bellinger
2016-01-08 8:52 ` Bart Van Assche
2016-01-08 9:17 ` Nicholas A. Bellinger
2016-01-08 9:35 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.