LIO Per initiator target discovery question.

Post by Benjamin ESTRABAUD
Hi!
After some search on google, it would appear that LIO doesn't support a
"per initiator (IQN) target discovery" feature like IET did with the
initiators.allow file (although it did more than just "hiding" targets
to initiators, it also refused connection from a particular initiator).
I am right with this assertion?

No.

By default (eg: when generate_node_acls=0) all initiators are denied
access to individual TargetName+TargetPortalGroupTag endpoints until an
explicit NodeACL based on InitiatorName is added by the target
administrator.

So while when discovery authentication is disabled, any initiator can
obtain the list of targets through sendtargets discovery, but default,
they are *not* allowed to login to any target endpoint without an
explicit NodeACL, nor without per NodeACL CHAP authentication
credentials.

--nab

Thomas Glanzmann

2013-09-14 08:54:12 UTC

Hello Nab,

Post by Nicholas A. Bellinger
So while when discovery authentication is disabled, any initiator can
obtain the list of targets through sendtargets discovery, but default,
they are *not* allowed to login to any target endpoint without an
explicit NodeACL, nor without per NodeACL CHAP authentication
credentials.

I have the same problem as Ben. I don't want to enable discovery
authentication but at the same time I want that only targets are
discovered that are either in demo mode or have at least one LUN with a
node acl presented. Are you willing to add such a feature to the code,
if so would you prefer to write a patch by yourself or should I do a
proposal?

Cheers,
Thomas

Nicholas A. Bellinger

2013-09-16 20:37:39 UTC

Post by Thomas Glanzmann
Hello Nab,

I'm open to accepting a patch for this.. However, I'd prefer to keep
the default action of being able to perform sendtargets discovery of all
TargetNames, regardless of these changes.

So that said, I'm thinking the patch should include a new TPG attribute
that allows the endpoint to be hidden from sendtargets discovery unless
a valid NodeACL exists for the connected InitiatorName. This TPG
attribute will be disabled by default, and can be enabled by admin on a
endpoint by endpoint basis.

If enabled + generate_node_acls=0 (eg: demo mode dislabed), the
discovery logic should walk through the list of NodeACLs for a given
TargetName+TargetPortalGroupTag endpoint, looking for match. If a match
is found then TargetName + Portals will be returned.

FYI, iscsit_build_sendtargets_response() is already a bit convoluted as
is, so I'll expect a patch to add this type of functionality to pretty
up the existing code as well.

--nab

Thomas Glanzmann

2013-09-25 13:26:58 UTC

Hello Ben,

I was wondering if you had made some progress on the target discovery
project?

not much. I setup a system, checkout the kernel, compiled it and booted
it and began writing on the patch but was interrupted by more pressing
work. I hope to finish it up over the weekend. My progress so far is:

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index bbfd288..79a9de2 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1043,6 +1043,11 @@ TPG_ATTR(demo_mode_write_protect, S_IRUGO | S_IWUSR);
*/
DEF_TPG_ATTRIB(prod_mode_write_protect);
TPG_ATTR(prod_mode_write_protect, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_hide_from_unauthorized
+ */
+DEF_TPG_ATTRIB(hide_from_unauthorized);
+TPG_ATTR(hide_from_unauthorized, S_IRUGO | S_IWUSR);

static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_authentication.attr,
@@ -1053,6 +1058,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_cache_dynamic_acls.attr,
&iscsi_tpg_attrib_demo_mode_write_protect.attr,
&iscsi_tpg_attrib_prod_mode_write_protect.attr,
+ &iscsi_tpg_attrib_hide_from_unauthorized.attr,
NULL,
};

@@ -1850,6 +1856,14 @@ static int lio_tpg_check_prod_mode_write_protect(
return ISCSI_TPG_ATTRIB(tpg)->prod_mode_write_protect;
}

+static int lio_tpg_check_hide_from_unauthorized(
+ struct se_portal_group *se_tpg)
+{
+ struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
+
+ return ISCSI_TPG_ATTRIB(tpg)->hide_from_unauthorized;
+}
+
static void lio_tpg_release_fabric_acl(
struct se_portal_group *se_tpg,
struct se_node_acl *se_acl)
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 4f77a78..ad0d007e 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -754,6 +754,7 @@ struct iscsi_tpg_attrib {
u32 default_cmdsn_depth;
u32 demo_mode_write_protect;
u32 prod_mode_write_protect;
+ u32 hide_from_unauthorized;
struct iscsi_portal_group *tpg;
};

I'm trying to get some time from our kernel developer to look at the
problem here also, so if you have made any progress we could merge our
effort.

If your developer starts working on it, please let me know. However I
hope that I'll finish it up over the weekend.

My roadmap is the following:

- Add a per target configfs paramter (started but untested and
probably unfinished)

- Cleanup iscsit_build_sendtargets_response

- Implement hide_from_unauthorized functionality

- Test it with 12 ESX servers and Linux initiator

- Post the patches to the mailinglist

- Work in the suggestions from the feedback of the mailinglist
until the patch is accepted upstream.

In addition to that I want to do some tests with ESX servers in order to
verify that LIO code is stable. Particular I want to test:

- Concurrent login from 12 initiators
- XCOPY with limited queue depth on 72 concurrent operations

After the above is finished I want to use it in production.

Cheers,
Thomas

Nicholas A. Bellinger

2013-09-25 18:20:35 UTC

Post by Thomas Glanzmann
Hello Ben,

I was wondering if you had made some progress on the target discovery
project?

not much. I setup a system, checkout the kernel, compiled it and booted
it and began writing on the patch but was interrupted by more pressing
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index bbfd288..79a9de2 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1043,6 +1043,11 @@ TPG_ATTR(demo_mode_write_protect, S_IRUGO | S_IWUSR);
*/
DEF_TPG_ATTRIB(prod_mode_write_protect);
TPG_ATTR(prod_mode_write_protect, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_hide_from_unauthorized
+ */
+DEF_TPG_ATTRIB(hide_from_unauthorized);
+TPG_ATTR(hide_from_unauthorized, S_IRUGO | S_IWUSR);
static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_authentication.attr,
@@ -1053,6 +1058,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_cache_dynamic_acls.attr,
&iscsi_tpg_attrib_demo_mode_write_protect.attr,
&iscsi_tpg_attrib_prod_mode_write_protect.attr,
+ &iscsi_tpg_attrib_hide_from_unauthorized.attr,
NULL,
};
@@ -1850,6 +1856,14 @@ static int lio_tpg_check_prod_mode_write_protect(
return ISCSI_TPG_ATTRIB(tpg)->prod_mode_write_protect;
}
+static int lio_tpg_check_hide_from_unauthorized(
+ struct se_portal_group *se_tpg)
+{
+ struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
+
+ return ISCSI_TPG_ATTRIB(tpg)->hide_from_unauthorized;
+}
+
static void lio_tpg_release_fabric_acl(
struct se_portal_group *se_tpg,
struct se_node_acl *se_acl)
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 4f77a78..ad0d007e 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -754,6 +754,7 @@ struct iscsi_tpg_attrib {
u32 default_cmdsn_depth;
u32 demo_mode_write_protect;
u32 prod_mode_write_protect;
+ u32 hide_from_unauthorized;
struct iscsi_portal_group *tpg;
};

Good start. :-)

I'm trying to get some time from our kernel developer to look at the
problem here also, so if you have made any progress we could merge our
effort.

If your developer starts working on it, please let me know. However I
hope that I'll finish it up over the weekend.
- Add a per target configfs paramter (started but untested and
probably unfinished)
- Cleanup iscsit_build_sendtargets_response
- Implement hide_from_unauthorized functionality
- Test it with 12 ESX servers and Linux initiator
- Post the patches to the mailinglist
- Work in the suggestions from the feedback of the mailinglist
until the patch is accepted upstream.
In addition to that I want to do some tests with ESX servers in order to
- Concurrent login from 12 initiators

FYI, I've been testing with 256 sessions, and with the target updates
now in v3.12-rc1 to multiplex login attempts per network portal, can
complete all logins (with mutual CHAP enabled) within 2-3 seconds.

Post by Thomas Glanzmann
- XCOPY with limited queue depth on 72 concurrent operations

Actually, in upstream code there is no notion of queue_depth that is
being enforced per device by the target (eg: it's all done below by the
block layer).

--nab

Thomas Glanzmann

2013-09-25 18:52:42 UTC

Hello Nab,

Post by Nicholas A. Bellinger
Good start. :-)

thanks.

Post by Nicholas A. Bellinger
FYI, I've been testing with 256 sessions, and with the target updates
now in v3.12-rc1 to multiplex login attempts per network portal, can
complete all logins (with mutual CHAP enabled) within 2-3 seconds.

Nice. For me it will only be 24 sessions without chap so I assume it
will be very fast.

Post by Nicholas A. Bellinger
Actually, in upstream code there is no notion of queue_depth that is
being enforced per device by the target (eg: it's all done below by
the block layer).

I see. Now that you mentioned it I remember that you told me before about
your plans do remove it. I'll do some testing myself and report back if
everything worked out. But I assume it will. :-)

Cheers,
Thomas

Benjamin ESTRABAUD

2013-09-26 10:40:59 UTC

Post by Thomas Glanzmann
Hello Ben,

Hi Thomas,

I was wondering if you had made some progress on the target discovery
project?

This is the attribute setting in configFS bit as I can gather, good start.

I'm trying to get some time from our kernel developer to look at the
problem here also, so if you have made any progress we could merge our
effort.

If your developer starts working on it, please let me know. However I
hope that I'll finish it up over the weekend.

I will indeed! Thanks.

Post by Thomas Glanzmann
- Add a per target configfs paramter (started but untested and
probably unfinished)
- Cleanup iscsit_build_sendtargets_response
- Implement hide_from_unauthorized functionality
- Test it with 12 ESX servers and Linux initiator
- Post the patches to the mailinglist
- Work in the suggestions from the feedback of the mailinglist
until the patch is accepted upstream.
In addition to that I want to do some tests with ESX servers in order to
- Concurrent login from 12 initiators
- XCOPY with limited queue depth on 72 concurrent operations
After the above is finished I want to use it in production.

Sounds like a very promising roadmap! We'll be able to do some extensive
testing here with a few initiators also.

Post by Thomas Glanzmann
Cheers,
Thomas

Regards,
Ben.

Post by Thomas Glanzmann
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thomas Glanzmann

2013-10-03 20:54:46 UTC

Hello Nab,

Post by Nicholas A. Bellinger
I'm open to accepting a patch for this.. However, I'd prefer to keep
the default action of being able to perform sendtargets discovery of
all TargetNames, regardless of these changes.

done. Please let me know if you like the attribute name
hide_from_unauthorized or if I should change it?

Post by Nicholas A. Bellinger
So that said, I'm thinking the patch should include a new TPG
attribute that allows the endpoint to be hidden from sendtargets
discovery unless a valid NodeACL exists for the connected
InitiatorName. This TPG attribute will be disabled by default, and
can be enabled by admin on a endpoint by endpoint basis.

done.

Post by Nicholas A. Bellinger
If enabled + generate_node_acls=0 (eg: demo mode dislabed), the
discovery logic should walk through the list of NodeACLs for a given
TargetName+TargetPortalGroupTag endpoint, looking for match. If a
match is found then TargetName + Portals will be returned.

done.

Post by Nicholas A. Bellinger
FYI, iscsit_build_sendtargets_response() is already a bit convoluted
as is, so I'll expect a patch to add this type of functionality to
pretty up the existing code as well.

I thought about this several hours, but need advice from you. I thought
about cleaning up this function the following ways but discarded it for
the reasons below:

- Split up the scenarios:

- Normal
- IFC_SENDTARGETS_SINGLE
- hide_from_unauthorized

I discarded this option because it results in a lot of
duplicated code.

- Make the function shorter by:

- Putting if (cmd->cmd_flags & IFC_SENDTARGETS_SINGLE) {
text_ptr = strchr(text_in, '=');
...
}
in a seperate function. And put the sprintf ...
payload_len += len; parts in a seperate function.

Make the function shorter but not much nicer.

- Now that I put my thoughts in writing I should have probably
done both.

Please give me advice on howto pretty up the existing code.

Post by Nicholas A. Bellinger
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
spin_unlock(&tpg->tpg_state_lock);
continue;
}
spin_unlock(&tpg->tpg_state_lock);

Is the spinlock necessary? I think it is not because the tpg_state is an
enum that is mapped to a byte, int or long int. The access to it is
atomic. So I don't think we need it.

Post by Nicholas A. Bellinger
if (! target_name_printed) {
len = sprintf(buf, "TargetName=%s",
tiqn->tiqn);
len += 1;
if ((len + payload_len) > buffer_len) {
spin_unlock(&tpg->tpg_np_lock);
spin_unlock(&tiqn->tiqn_tpg_lock);
end_of_buf = 1;
goto eob;
}
memcpy(payload + payload_len, buf, len);
payload_len += len;
target_name_printed = 1;
}
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?
np->np_ip : conn->local_ip,
(inaddr_any == false) ?
np->np_port : conn->local_port,
tpg->tpgt);
len += 1;
if ((len + payload_len) > buffer_len) {
spin_unlock(&tpg->tpg_np_lock);
spin_unlock(&tiqn->tiqn_tpg_lock);
end_of_buf = 1;
goto eob;
}
memcpy(payload + payload_len, buf, len);
payload_len += len;

With the current code path it is possible that TargetName is printed but
TargetAddress is not because the payload buffer runs out of space. Is
this okay, or should we change it?

I often have a configuration where I export multiple demo mode LUNs
in different VLANs but I only want initiators from same VLAN access that
LUN. To my knowledge that is currently not possible, is it? Would you
accept another patch which only exposes demo mode LUNs if a portal
exists for that LUN for the same ip address as the discovery is
targeted or another patch which gives me same functionality but maybe
does it another way?

Cheers,
Thomas

Thomas Glanzmann

2013-10-03 20:56:11 UTC

Add a new TPG attribute hide_from_unauthorized which is disabled by default.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target_configfs.c | 16 ++++++++++++++++
drivers/target/iscsi/iscsi_target_core.h | 2 ++
drivers/target/iscsi/iscsi_target_tpg.c | 20 ++++++++++++++++++++
drivers/target/iscsi/iscsi_target_tpg.h | 1 +
include/target/target_core_fabric.h | 1 +
5 files changed, 40 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index fd14525..c622ad5 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1041,6 +1041,11 @@ TPG_ATTR(demo_mode_write_protect, S_IRUGO | S_IWUSR);
*/
DEF_TPG_ATTRIB(prod_mode_write_protect);
TPG_ATTR(prod_mode_write_protect, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_hide_from_unauthorized
+ */
+DEF_TPG_ATTRIB(hide_from_unauthorized);
+TPG_ATTR(hide_from_unauthorized, S_IRUGO | S_IWUSR);

static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_authentication.attr,
@@ -1051,6 +1056,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_cache_dynamic_acls.attr,
&iscsi_tpg_attrib_demo_mode_write_protect.attr,
&iscsi_tpg_attrib_prod_mode_write_protect.attr,
+ &iscsi_tpg_attrib_hide_from_unauthorized.attr,
NULL,
};

@@ -1848,6 +1854,14 @@ static int lio_tpg_check_prod_mode_write_protect(
return ISCSI_TPG_ATTRIB(tpg)->prod_mode_write_protect;
}

+static int lio_tpg_check_hide_from_unauthorized(
+ struct se_portal_group *se_tpg)
+{
+ struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
+
+ return ISCSI_TPG_ATTRIB(tpg)->hide_from_unauthorized;
+}
+
static void lio_tpg_release_fabric_acl(
struct se_portal_group *se_tpg,
struct se_node_acl *se_acl)
@@ -1960,6 +1974,8 @@ int iscsi_target_register_configfs(void)
&lio_tpg_check_demo_mode_write_protect;
fabric->tf_ops.tpg_check_prod_mode_write_protect =
&lio_tpg_check_prod_mode_write_protect;
+ fabric->tf_ops.tpg_check_hide_from_unauthorized=
+ &lio_tpg_check_hide_from_unauthorized;
fabric->tf_ops.tpg_alloc_fabric_acl = &lio_tpg_alloc_fabric_acl;
fabric->tf_ops.tpg_release_fabric_acl = &lio_tpg_release_fabric_acl;
fabric->tf_ops.tpg_get_inst_index = &lio_tpg_get_inst_index;
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 9a5721b..94e6bb5 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -58,6 +58,7 @@
#define TA_DEMO_MODE_WRITE_PROTECT 1
/* Disabled by default in production mode w/ explict ACLs */
#define TA_PROD_MODE_WRITE_PROTECT 0
+#define TA_HIDE_FROM_UNAUTHORIZED 0
#define TA_CACHE_CORE_NPS 0

@@ -769,6 +770,7 @@ struct iscsi_tpg_attrib {
u32 default_cmdsn_depth;
u32 demo_mode_write_protect;
u32 prod_mode_write_protect;
+ u32 hide_from_unauthorized;
struct iscsi_portal_group *tpg;
};

diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 4faeb47..2a09a93 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -223,6 +223,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
a->cache_dynamic_acls = TA_CACHE_DYNAMIC_ACLS;
a->demo_mode_write_protect = TA_DEMO_MODE_WRITE_PROTECT;
a->prod_mode_write_protect = TA_PROD_MODE_WRITE_PROTECT;
+ a->hide_from_unauthorized = TA_HIDE_FROM_UNAUTHORIZED;
}

int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
@@ -820,3 +821,22 @@ int iscsit_ta_prod_mode_write_protect(

return 0;
}
+
+int iscsit_ta_hide_from_unauthorized(
+ struct iscsi_portal_group *tpg,
+ u32 flag)
+{
+ struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
+
+ if ((flag != 0) && (flag != 1)) {
+ pr_err("Illegal value %d\n", flag);
+ return -EINVAL;
+ }
+
+ a->hide_from_unauthorized = flag;
+ pr_debug("iSCSI_TPG[%hu] - Hide From Unauthorized bit:"
+ " %s\n", tpg->tpgt, (a->hide_from_unauthorized) ?
+ "ON" : "OFF");
+
+ return 0;
+}
diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
index b77693e..bfdd2ad 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.h
+++ b/drivers/target/iscsi/iscsi_target_tpg.h
@@ -37,5 +37,6 @@ extern int iscsit_ta_default_cmdsn_depth(struct iscsi_portal_group *, u32);
extern int iscsit_ta_cache_dynamic_acls(struct iscsi_portal_group *, u32);
extern int iscsit_ta_demo_mode_write_protect(struct iscsi_portal_group *, u32);
extern int iscsit_ta_prod_mode_write_protect(struct iscsi_portal_group *, u32);
+extern int iscsit_ta_hide_from_unauthorized(struct iscsi_portal_group *, u32);

#endif /* ISCSI_TARGET_TPG_H */
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 882b650e..5eb977e 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -21,6 +21,7 @@ struct target_core_fabric_ops {
int (*tpg_check_demo_mode_cache)(struct se_portal_group *);
int (*tpg_check_demo_mode_write_protect)(struct se_portal_group *);
int (*tpg_check_prod_mode_write_protect)(struct se_portal_group *);
+ int (*tpg_check_hide_from_unauthorized)(struct se_portal_group *);
/*
* Optionally used by fabrics to allow demo-mode login, but not
* expose any TPG LUNs, and return 'not connected' in standard

--
1.7.10.4

Nicholas A. Bellinger

2013-10-03 22:18:43 UTC

Post by Thomas Glanzmann
Add a new TPG attribute hide_from_unauthorized which is disabled by default.
---
drivers/target/iscsi/iscsi_target_configfs.c | 16 ++++++++++++++++
drivers/target/iscsi/iscsi_target_core.h | 2 ++
drivers/target/iscsi/iscsi_target_tpg.c | 20 ++++++++++++++++++++
drivers/target/iscsi/iscsi_target_tpg.h | 1 +
include/target/target_core_fabric.h | 1 +
5 files changed, 40 insertions(+)
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index fd14525..c622ad5 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1041,6 +1041,11 @@ TPG_ATTR(demo_mode_write_protect, S_IRUGO | S_IWUSR);
*/
DEF_TPG_ATTRIB(prod_mode_write_protect);
TPG_ATTR(prod_mode_write_protect, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_hide_from_unauthorized
+ */
+DEF_TPG_ATTRIB(hide_from_unauthorized);
+TPG_ATTR(hide_from_unauthorized, S_IRUGO | S_IWUSR);

So as mentioned in the previous email, let's change this to
demo_mode_discovery as it's slightly more descriptive about what the
attribute is actually used for..

Post by Thomas Glanzmann
static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_authentication.attr,
@@ -1051,6 +1056,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_cache_dynamic_acls.attr,
&iscsi_tpg_attrib_demo_mode_write_protect.attr,
&iscsi_tpg_attrib_prod_mode_write_protect.attr,
+ &iscsi_tpg_attrib_hide_from_unauthorized.attr,
NULL,
};

Extra whitespace here..

Post by Thomas Glanzmann
@@ -1848,6 +1854,14 @@ static int lio_tpg_check_prod_mode_write_protect(
return ISCSI_TPG_ATTRIB(tpg)->prod_mode_write_protect;
}
+static int lio_tpg_check_hide_from_unauthorized(
+ struct se_portal_group *se_tpg)
+{
+ struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
+
+ return ISCSI_TPG_ATTRIB(tpg)->hide_from_unauthorized;
+}
+
static void lio_tpg_release_fabric_acl(
struct se_portal_group *se_tpg,
struct se_node_acl *se_acl)
@@ -1960,6 +1974,8 @@ int iscsi_target_register_configfs(void)
&lio_tpg_check_demo_mode_write_protect;
fabric->tf_ops.tpg_check_prod_mode_write_protect =
&lio_tpg_check_prod_mode_write_protect;
+ fabric->tf_ops.tpg_check_hide_from_unauthorized=
+ &lio_tpg_check_hide_from_unauthorized;

Give that only iscsi_target_mod needs to know about this attribute,
there is no reason why it needs to be part of target_core_fabric_ops.

So that said, it's fine to drop this part.

Post by Thomas Glanzmann
fabric->tf_ops.tpg_alloc_fabric_acl = &lio_tpg_alloc_fabric_acl;
fabric->tf_ops.tpg_release_fabric_acl = &lio_tpg_release_fabric_acl;
fabric->tf_ops.tpg_get_inst_index = &lio_tpg_get_inst_index;
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 9a5721b..94e6bb5 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -58,6 +58,7 @@
#define TA_DEMO_MODE_WRITE_PROTECT 1
/* Disabled by default in production mode w/ explict ACLs */
#define TA_PROD_MODE_WRITE_PROTECT 0
+#define TA_HIDE_FROM_UNAUTHORIZED 0
#define TA_CACHE_CORE_NPS 0
@@ -769,6 +770,7 @@ struct iscsi_tpg_attrib {
u32 default_cmdsn_depth;
u32 demo_mode_write_protect;
u32 prod_mode_write_protect;
+ u32 hide_from_unauthorized;
struct iscsi_portal_group *tpg;
};
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 4faeb47..2a09a93 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -223,6 +223,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
a->cache_dynamic_acls = TA_CACHE_DYNAMIC_ACLS;
a->demo_mode_write_protect = TA_DEMO_MODE_WRITE_PROTECT;
a->prod_mode_write_protect = TA_PROD_MODE_WRITE_PROTECT;
+ a->hide_from_unauthorized = TA_HIDE_FROM_UNAUTHORIZED;
}
int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
@@ -820,3 +821,22 @@ int iscsit_ta_prod_mode_write_protect(
return 0;
}
+
+int iscsit_ta_hide_from_unauthorized(
+ struct iscsi_portal_group *tpg,
+ u32 flag)
+{
+ struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
+
+ if ((flag != 0) && (flag != 1)) {
+ pr_err("Illegal value %d\n", flag);
+ return -EINVAL;
+ }
+
+ a->hide_from_unauthorized = flag;
+ pr_debug("iSCSI_TPG[%hu] - Hide From Unauthorized bit:"
+ " %s\n", tpg->tpgt, (a->hide_from_unauthorized) ?
+ "ON" : "OFF");
+
+ return 0;
+}
diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
index b77693e..bfdd2ad 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.h
+++ b/drivers/target/iscsi/iscsi_target_tpg.h
@@ -37,5 +37,6 @@ extern int iscsit_ta_default_cmdsn_depth(struct iscsi_portal_group *, u32);
extern int iscsit_ta_cache_dynamic_acls(struct iscsi_portal_group *, u32);
extern int iscsit_ta_demo_mode_write_protect(struct iscsi_portal_group *, u32);
extern int iscsit_ta_prod_mode_write_protect(struct iscsi_portal_group *, u32);
+extern int iscsit_ta_hide_from_unauthorized(struct iscsi_portal_group *, u32);
#endif /* ISCSI_TARGET_TPG_H */
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 882b650e..5eb977e 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -21,6 +21,7 @@ struct target_core_fabric_ops {
int (*tpg_check_demo_mode_cache)(struct se_portal_group *);
int (*tpg_check_demo_mode_write_protect)(struct se_portal_group *);
int (*tpg_check_prod_mode_write_protect)(struct se_portal_group *);
+ int (*tpg_check_hide_from_unauthorized)(struct se_portal_group *);

Ditto here..

--nab

Thomas Glanzmann

2013-10-03 20:57:04 UTC

If hide_from_unauthorized=1 and generate_node_acls=0 (demo mode dislabed) do not
return TargetName+TargetAddress unless a NodeACL exists.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target.c | 39 +++++++++++++++++++++++++++--------
drivers/target/target_core_tpg.c | 1 +
2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 35b61f7..8e1b3ff 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3369,6 +3369,7 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_tiqn *tiqn;
struct iscsi_tpg_np *tpg_np;
int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+ int target_name_printed;
unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
unsigned char *text_in = cmd->text_in_ptr, *text_ptr = NULL;

@@ -3406,19 +3407,23 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
continue;
}

- len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
- len += 1;
-
- if ((len + payload_len) > buffer_len) {
- end_of_buf = 1;
- goto eob;
- }
- memcpy(payload + payload_len, buf, len);
- payload_len += len;
+ target_name_printed = 0;

spin_lock(&tiqn->tiqn_tpg_lock);
list_for_each_entry(tpg, &tiqn->tiqn_tpg_list, tpg_list) {

+ /* If hide_from_unauthorized and generate_node_acls=0
+ * (demo mode dislabed) do not return
+ * TargetName+TargetAddress unless a NodeACL exists.
+ */
+
+ if ((tpg->tpg_attrib.generate_node_acls == 0)
+ && (tpg->tpg_attrib.hide_from_unauthorized == 1)
+ && (! core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+ cmd->conn->sess->sess_ops->InitiatorName))) {
+ continue;
+ }
+
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
@@ -3433,6 +3438,22 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_np *np = tpg_np->tpg_np;
bool inaddr_any = iscsit_check_inaddr_any(np);

+ if (! target_name_printed) {
+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);
+ len += 1;
+
+ if ((len + payload_len) > buffer_len) {
+ spin_unlock(&tpg->tpg_np_lock);
+ spin_unlock(&tiqn->tiqn_tpg_lock);
+ end_of_buf = 1;
+ goto eob;
+ }
+ memcpy(payload + payload_len, buf, len);
+ payload_len += len;
+ target_name_printed = 1;
+ }
+
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(

return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

/* core_tpg_add_node_to_devs():
*

--
1.7.10.4

Nicholas A. Bellinger

2013-10-03 22:27:10 UTC

Post by Thomas Glanzmann
If hide_from_unauthorized=1 and generate_node_acls=0 (demo mode dislabed) do not
return TargetName+TargetAddress unless a NodeACL exists.
---
drivers/target/iscsi/iscsi_target.c | 39 +++++++++++++++++++++++++++--------
drivers/target/target_core_tpg.c | 1 +
2 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 35b61f7..8e1b3ff 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3369,6 +3369,7 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_tiqn *tiqn;
struct iscsi_tpg_np *tpg_np;
int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+ int target_name_printed;
unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
unsigned char *text_in = cmd->text_in_ptr, *text_ptr = NULL;
@@ -3406,19 +3407,23 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
continue;
}
- len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
- len += 1;
-
- if ((len + payload_len) > buffer_len) {
- end_of_buf = 1;
- goto eob;
- }
- memcpy(payload + payload_len, buf, len);
- payload_len += len;
+ target_name_printed = 0;

Extra white-space here.. All these changes need to be using TAB spacing
btw. ;)

Post by Thomas Glanzmann
spin_lock(&tiqn->tiqn_tpg_lock);
list_for_each_entry(tpg, &tiqn->tiqn_tpg_list, tpg_list) {
+ /* If hide_from_unauthorized and generate_node_acls=0
+ * (demo mode dislabed) do not return
+ * TargetName+TargetAddress unless a NodeACL exists.
+ */
+
+ if ((tpg->tpg_attrib.generate_node_acls == 0)
+ && (tpg->tpg_attrib.hide_from_unauthorized == 1)
+ && (! core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+ cmd->conn->sess->sess_ops->InitiatorName))) {
+ continue;
+ }
+

Kernel style stays that the "&&" operators should be on the preceding
line.

Also, drop the extra whitespace between (! core_tpg_...())

Post by Thomas Glanzmann
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
@@ -3433,6 +3438,22 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_np *np = tpg_np->tpg_np;
bool inaddr_any = iscsit_check_inaddr_any(np);
+ if (! target_name_printed) {

Ditto here on the extra whitespace.

Post by Thomas Glanzmann
+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);

Kernel style is to make the following tiqn->tiqn); part here line up
with the sprintf(, eg:

len = sprintf(foo, "%s',
bar);

Post by Thomas Glanzmann
+ len += 1;
+
+ if ((len + payload_len) > buffer_len) {
+ spin_unlock(&tpg->tpg_np_lock);
+ spin_unlock(&tiqn->tiqn_tpg_lock);
+ end_of_buf = 1;
+ goto eob;
+ }
+ memcpy(payload + payload_len, buf, len);
+ payload_len += len;
+ target_name_printed = 1;
+ }
+
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(
return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);
*

The rest looks reasonable to me. Nice work.

--nab

Thomas Glanzmann

2013-10-04 19:56:14 UTC

Hello Nab,

attribute name hide_from_unauthorized

How about renaming the new attribute to demo_mode_discovery, and using
a default value to 1 + inverting the current checks?

done.

Post by Nicholas A. Bellinger
Mmm, all fair points above. Thinking about this some more, I'm fine
with taking these patches without further cleanups here, and leave
this as a separate item for now.

If you have any input for me, let me know and I'll take care of it.

spin_lock(&tpg->tpg_state_lock);

tpg->tpg_state is protected every place else by tpg_state_lock. Your
right that it's probably overkill for this particular case given the
purely information nature of SendTargets.

I see and I think it is good here to be consistent as well.

With the current code path it is possible that TargetName is printed but
TargetAddress is not because the payload buffer runs out of space. Is
this okay, or should we change it?

It's possible this could happen, yes. However the initiator receiving a
TargetName without any TargetAddresses should simply ignore this entry.

I see. If you want me to test this or change the code, let me know.

I often have a configuration where I export multiple demo mode LUNs
in different VLANs but I only want initiators from same VLAN access
that LUN. To my knowledge that is currently not possible, is it?

However, splitting up these LUNs across different TargetName
+TargetPortalGroupTag endpoints, and only associating the individual
VLAN interfaces as network portals on these endpoints should provide
the desired effect, no?

I tried it, but it was still discovering all LUNs. Is the config right?

/backstores/fileio create shared-01.v101.campusvl.de /iscsi1/shared-01.v101.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.4
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.5
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/luns create /backstores/fileio/shared-01.v101.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1

/backstores/fileio create shared-01.v102.campusvl.de /iscsi1/shared-01.v102.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ create 2
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.4
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.5
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/luns create /backstores/fileio/shared-01.v102.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ delete 1

When I do a discovery I get both endpoints back.
shared-01.v101.campusvl.iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de and
iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de. Or did I misunderstand
howto set the TargetPortalGroupTag? Or did I miss something else or should it
not do that, if so I would go in the code and search for the problem.

+ &iscsi_tpg_attrib_hide_from_unauthorized.attr,

Extra whitespace here..

I changed all spaces into tabs. I have my vim configured to do
'expandtabs' so everytime I hit 'tab' it puts 8 spaces. I disabled the
expandtabs feature of vim and made sure that the patches only contain
tabs.

+ fabric->tf_ops.tpg_check_hide_from_unauthorized=
+ &lio_tpg_check_hide_from_unauthorized;

Give that only iscsi_target_mod needs to know about this attribute,
there is no reason why it needs to be part of target_core_fabric_ops.
So that said, it's fine to drop this part.

done.

+ int (*tpg_check_hide_from_unauthorized)(struct se_portal_group *);

Ditto here..

done. I also dropped the now unused lio_tpg_check_hide_from_unauthorized.

+ target_name_printed = 0;

Extra white-space here.. All these changes need to be using TAB spacing
btw. ;)

done.

+ && (tpg->tpg_attrib.hide_from_unauthorized == 1)
+ && (! core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,

Kernel style stays that the "&&" operators should be on the preceding
line. Also, drop the extra whitespace between (! core_tpg_...())

done.

+ if (! target_name_printed) {

Ditto here on the extra whitespace.

done.

+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);

Kernel style is to make the following tiqn->tiqn); part here line up
len = sprintf(foo, "%s',
bar);

done.

Post by Nicholas A. Bellinger
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

The patch I was sending you yesterday did _not_ compile because the
prototype for core_tpg_get_initiator_node_acl was missing. I added the
following line to the patch:

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1330045..504b5b5 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -47,6 +47,8 @@
#include "iscsi_target_device.h"
#include "iscsi_target_stat.h"

+#include "../target_core_internal.h"

During testing yesterday I had this line in, however for submitting the patch I
somehow thought it was a relict that I no longer needed so I removed the line
before submitting the patch to you, which was of course wrong. I readded the
line. Please let me know if this line is okay for you or if put the prototype
for core_tpg_get_initiator_node_acl in another header file or want it defined
manually?

Post by Nicholas A. Bellinger
The rest looks reasonable to me. Nice work.

Prefect. Find v2 of the patches. I also tested them on top of the
iscsi-target: percpu_ida tag starvation regression fixes that you send
out earlier today with the usual test setup:

- 12 ESX servers, parallel rescan, 24 VMs deployed in parallel
and powered on in parallel and 24 svMotion in parallel,
unloading the target during operation, watch the slabinfo and
reloading it again.

All working perfectly fine. However I see the following log messages
which I did not see before when I did the 24 concurrent svMotion using
XCOPY:

Oct 4 21:38:42 node-62 kernel: [ 934.369442] Unable to locate ITT: 0x0000b4aa on CID: 0
Oct 4 21:38:42 node-62 kernel: [ 934.374446] Unable to locate RefTaskTag: 0x0000b4aa on CID: 0.

But everything worked afterwards.

I sniffed the discovery. With the default options it looks like:

iSCSI (Text Response)
Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x000008ac
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0xb89e7744
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-03.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-04.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-05.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-06.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-07.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-08.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-09.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-10.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-11.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-12.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1

When disabling demo_mode_discovery I get:

find /sys/kernel/config/target/iscsi -name demo_mode_discovery | while read FILE; do echo 0 > $FILE; done

iSCSI (Text Response)
Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x000002be
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0xa33a9e16
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-12.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
Padding: 0000

Btw. I used the line 'tshark -V -r /tmp/discover6.pcap -R iscsi.keyvalue |
less' to extract the above. Comes in handy.

As you can see I also get the targets from TGPT '2' discovered. Any idea what
I'm doing here wrong?

Nab, what do we do about the userland changes? Should I prepare a patch?
If so where can I find the repository for targetcli/rtsadmin?

Cheers,
Thomas

Thomas Glanzmann

2013-10-04 19:57:16 UTC

Add a new TPG attribute demo_mode_discovery which is enabled by default.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target_configfs.c | 6 ++++++
drivers/target/iscsi/iscsi_target_core.h | 2 ++
drivers/target/iscsi/iscsi_target_tpg.c | 20 ++++++++++++++++++++
drivers/target/iscsi/iscsi_target_tpg.h | 1 +
4 files changed, 29 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index fd14525..300f65c 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1041,6 +1041,11 @@ TPG_ATTR(demo_mode_write_protect, S_IRUGO | S_IWUSR);
*/
DEF_TPG_ATTRIB(prod_mode_write_protect);
TPG_ATTR(prod_mode_write_protect, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_demo_mode_discovery,
+ */
+DEF_TPG_ATTRIB(demo_mode_discovery);
+TPG_ATTR(demo_mode_discovery, S_IRUGO | S_IWUSR);

static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_authentication.attr,
@@ -1051,6 +1056,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
&iscsi_tpg_attrib_cache_dynamic_acls.attr,
&iscsi_tpg_attrib_demo_mode_write_protect.attr,
&iscsi_tpg_attrib_prod_mode_write_protect.attr,
+ &iscsi_tpg_attrib_demo_mode_discovery.attr,
NULL,
};

diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 9a5721b..a03a989 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -58,6 +58,7 @@
#define TA_DEMO_MODE_WRITE_PROTECT 1
/* Disabled by default in production mode w/ explict ACLs */
#define TA_PROD_MODE_WRITE_PROTECT 0
+#define TA_DEMO_MODE_DISCOVERY 1
#define TA_CACHE_CORE_NPS 0

@@ -769,6 +770,7 @@ struct iscsi_tpg_attrib {
u32 default_cmdsn_depth;
u32 demo_mode_write_protect;
u32 prod_mode_write_protect;
+ u32 demo_mode_discovery;
struct iscsi_portal_group *tpg;
};

diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 4faeb47..0cd5e75 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -223,6 +223,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
a->cache_dynamic_acls = TA_CACHE_DYNAMIC_ACLS;
a->demo_mode_write_protect = TA_DEMO_MODE_WRITE_PROTECT;
a->prod_mode_write_protect = TA_PROD_MODE_WRITE_PROTECT;
+ a->demo_mode_discovery = TA_DEMO_MODE_DISCOVERY;
}

int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
@@ -820,3 +821,22 @@ int iscsit_ta_prod_mode_write_protect(

return 0;
}
+
+int iscsit_ta_demo_mode_discovery(
+ struct iscsi_portal_group *tpg,
+ u32 flag)
+{
+ struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
+
+ if ((flag != 0) && (flag != 1)) {
+ pr_err("Illegal value %d\n", flag);
+ return -EINVAL;
+ }
+
+ a->demo_mode_discovery = flag;
+ pr_debug("iSCSI_TPG[%hu] - Demo Mode Discovery bit:"
+ " %s\n", tpg->tpgt, (a->demo_mode_discovery) ?
+ "ON" : "OFF");
+
+ return 0;
+}
diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
index b77693e..3e8ce86 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.h
+++ b/drivers/target/iscsi/iscsi_target_tpg.h
@@ -37,5 +37,6 @@ extern int iscsit_ta_default_cmdsn_depth(struct iscsi_portal_group *, u32);
extern int iscsit_ta_cache_dynamic_acls(struct iscsi_portal_group *, u32);
extern int iscsit_ta_demo_mode_write_protect(struct iscsi_portal_group *, u32);
extern int iscsit_ta_prod_mode_write_protect(struct iscsi_portal_group *, u32);
+extern int iscsit_ta_demo_mode_discovery(struct iscsi_portal_group *, u32);

#endif /* ISCSI_TARGET_TPG_H */

--
1.7.10.4

Thomas Glanzmann

2013-10-04 19:57:46 UTC

If hide_from_unauthorized=0 and generate_node_acls=0 (demo mode dislabed) do
not return TargetName+TargetAddress unless a NodeACL exists.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target.c | 41 +++++++++++++++++++++++++++--------
drivers/target/target_core_tpg.c | 1 +
2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 38e44b9..504b5b5 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -47,6 +47,8 @@
#include "iscsi_target_device.h"
#include "iscsi_target_stat.h"

+#include "../target_core_internal.h"
+
#include <target/iscsi/iscsi_transport.h>

static LIST_HEAD(g_tiqn_list);
@@ -3374,6 +3376,7 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_tiqn *tiqn;
struct iscsi_tpg_np *tpg_np;
int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+ int target_name_printed;
unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
unsigned char *text_in = cmd->text_in_ptr, *text_ptr = NULL;

@@ -3411,19 +3414,23 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
continue;
}

- len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
- len += 1;
-
- if ((len + payload_len) > buffer_len) {
- end_of_buf = 1;
- goto eob;
- }
- memcpy(payload + payload_len, buf, len);
- payload_len += len;
+ target_name_printed = 0;

spin_lock(&tiqn->tiqn_tpg_lock);
list_for_each_entry(tpg, &tiqn->tiqn_tpg_list, tpg_list) {

+ /* If demo_mode_discovery=0 and generate_node_acls=0
+ * (demo mode dislabed) do not return
+ * TargetName+TargetAddress unless a NodeACL exists.
+ */
+
+ if ((tpg->tpg_attrib.generate_node_acls == 0) &&
+ (tpg->tpg_attrib.demo_mode_discovery == 0) &&
+ (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+ cmd->conn->sess->sess_ops->InitiatorName))) {
+ continue;
+ }
+
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
@@ -3438,6 +3445,22 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_np *np = tpg_np->tpg_np;
bool inaddr_any = iscsit_check_inaddr_any(np);

+ if (!target_name_printed) {
+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);
+ len += 1;
+
+ if ((len + payload_len) > buffer_len) {
+ spin_unlock(&tpg->tpg_np_lock);
+ spin_unlock(&tiqn->tiqn_tpg_lock);
+ end_of_buf = 1;
+ goto eob;
+ }
+ memcpy(payload + payload_len, buf, len);
+ payload_len += len;
+ target_name_printed = 1;
+ }
+
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(

return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

/* core_tpg_add_node_to_devs():
*

--
1.7.10.4

Thomas Glanzmann

2013-10-05 06:22:25 UTC

Hello Nab,

Post by Thomas Glanzmann
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

The patch I was sending you yesterday did _not_ compile because the
prototype for core_tpg_get_initiator_node_acl was missing. I added the
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1330045..504b5b5 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -47,6 +47,8 @@
#include "iscsi_target_device.h"
#include "iscsi_target_stat.h"
+#include "../target_core_internal.h"
During testing yesterday I had this line in, however for submitting the patch I
somehow thought it was a relict that I no longer needed so I removed the line
before submitting the patch to you, which was of course wrong. I readded the
line. Please let me know if this line is okay for you or if put the prototype
for core_tpg_get_initiator_node_acl in another header file or want it defined
manually?

I was able to answer that question by myself. I should move the
prototype from drivers/target/target_core_internal.h to
include/target/target_core_fabric.h similiar to
core_tpg_check_initiator_node_acl which is already global.

I did the same find v3 of the patchset below. Moving patch 2 to patch 3
and make core_tpg_get_initiator_node_acl global in patch2. patch1 stays
unmodified.

patch 3 also drops the +#include "../target_core_internal.h" line.

Cheers,
Thomas

Thomas Glanzmann

2013-10-05 06:23:09 UTC

--
1.7.10.4

Thomas Glanzmann

2013-10-05 06:23:41 UTC

Export symbol core_tpg_check_initiator_node_acl and move prototype from the
private drivers/target/target_core_internal.h to the public
include/target/target_core_fabric.h

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/target_core_internal.h | 2 --
drivers/target/target_core_tpg.c | 1 +
include/target/target_core_fabric.h | 2 ++
3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 579128a..889af4d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -75,8 +75,6 @@ extern struct se_device *g_lun0_dev;

struct se_node_acl *__core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
const char *);
-struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
- unsigned char *);
void core_tpg_add_node_to_devs(struct se_node_acl *, struct se_portal_group *);
void core_tpg_wait_for_nacl_pr_ref(struct se_node_acl *);
struct se_lun *core_tpg_pre_addlun(struct se_portal_group *, u32);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(

return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

/* core_tpg_add_node_to_devs():
*
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 882b650e..98eb7d3 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -137,6 +137,8 @@ void transport_generic_request_failure(struct se_cmd *, sense_reason_t);
void __target_execute_cmd(struct se_cmd *);
int transport_lookup_tmr_lun(struct se_cmd *, u32);

+struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
+ unsigned char *);
struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *,
unsigned char *);
void core_tpg_clear_object_luns(struct se_portal_group *);

--
1.7.10.4

Thomas Glanzmann

2013-10-05 06:27:44 UTC

Hello Nab,

Post by Thomas Glanzmann
+struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
+ unsigned char *);
struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *,
unsigned char *);
void core_tpg_clear_object_luns(struct se_portal_group *);

whitespace polution again. Find the patch below with tabs instead of
spaces.

Cheers,
Thomas

Thomas Glanzmann

2013-10-05 06:28:16 UTC

From 7959819a1ce39543dc5411028935de8b31cd79ef Mon Sep 17 00:00:00 2001

From: Thomas Glanzmann <***@glanzmann.de>
Date: Sat, 5 Oct 2013 08:13:21 +0200

Export symbol core_tpg_check_initiator_node_acl and move prototype from the
private drivers/target/target_core_internal.h to the public
include/target/target_core_fabric.h

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/target_core_internal.h | 2 --
drivers/target/target_core_tpg.c | 1 +
include/target/target_core_fabric.h | 2 ++
3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 579128a..889af4d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -75,8 +75,6 @@ extern struct se_device *g_lun0_dev;

struct se_node_acl *__core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
const char *);
-struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
- unsigned char *);
void core_tpg_add_node_to_devs(struct se_node_acl *, struct se_portal_group *);
void core_tpg_wait_for_nacl_pr_ref(struct se_node_acl *);
struct se_lun *core_tpg_pre_addlun(struct se_portal_group *, u32);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(

return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

/* core_tpg_add_node_to_devs():
*
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 882b650e..98eb7d3 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -137,6 +137,8 @@ void transport_generic_request_failure(struct se_cmd *, sense_reason_t);
void __target_execute_cmd(struct se_cmd *);
int transport_lookup_tmr_lun(struct se_cmd *, u32);

+struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
+ unsigned char *);
struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *,
unsigned char *);
void core_tpg_clear_object_luns(struct se_portal_group *);

--
1.7.10.4

Thomas Glanzmann

2013-10-05 06:29:32 UTC

Hello Nab,
one more time without the double mail header. Sorry about that. ;-(

Cheers,
Thomas

Thomas Glanzmann

2013-10-05 06:30:10 UTC

--
1.7.10.4

Thomas Glanzmann

2013-10-05 06:24:25 UTC

If hide_from_unauthorized=0 and generate_node_acls=0 (demo mode dislabed) do
not return TargetName+TargetAddress unless a NodeACL exists.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target.c | 39 +++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 38e44b9..1330045 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3374,6 +3374,7 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_tiqn *tiqn;
struct iscsi_tpg_np *tpg_np;
int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+ int target_name_printed;
unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
unsigned char *text_in = cmd->text_in_ptr, *text_ptr = NULL;

@@ -3411,19 +3412,23 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
continue;
}

- len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
- len += 1;
-
- if ((len + payload_len) > buffer_len) {
- end_of_buf = 1;
- goto eob;
- }
- memcpy(payload + payload_len, buf, len);
- payload_len += len;
+ target_name_printed = 0;

spin_lock(&tiqn->tiqn_tpg_lock);
list_for_each_entry(tpg, &tiqn->tiqn_tpg_list, tpg_list) {

+ /* If demo_mode_discovery=0 and generate_node_acls=0
+ * (demo mode dislabed) do not return
+ * TargetName+TargetAddress unless a NodeACL exists.
+ */
+
+ if ((tpg->tpg_attrib.generate_node_acls == 0) &&
+ (tpg->tpg_attrib.demo_mode_discovery == 0) &&
+ (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+ cmd->conn->sess->sess_ops->InitiatorName))) {
+ continue;
+ }
+
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
@@ -3438,6 +3443,22 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_np *np = tpg_np->tpg_np;
bool inaddr_any = iscsit_check_inaddr_any(np);

+ if (!target_name_printed) {
+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);
+ len += 1;
+
+ if ((len + payload_len) > buffer_len) {
+ spin_unlock(&tpg->tpg_np_lock);
+ spin_unlock(&tiqn->tiqn_tpg_lock);
+ end_of_buf = 1;
+ goto eob;
+ }
+ memcpy(payload + payload_len, buf, len);
+ payload_len += len;
+ target_name_printed = 1;
+ }
+
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?

--
1.7.10.4

Thomas Glanzmann

2013-10-05 11:26:15 UTC

Hello Nab,

Post by Thomas Glanzmann
If hide_from_unauthorized=0 and generate_node_acls=0 (demo mode dislabed) do
not return TargetName+TargetAddress unless a NodeACL exists.

the commit comment was wrong because it is now called
demo_mode_discovery=0. I'm resending this patch.

Cheers,
Thomas

Thomas Glanzmann

2013-10-05 11:27:22 UTC

If demo_mode_discovery=0 and generate_node_acls=0 (demo mode dislabed) do
not return TargetName+TargetAddress unless a NodeACL exists.

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/iscsi/iscsi_target.c | 39 +++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 38e44b9..1330045 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3374,6 +3374,7 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_tiqn *tiqn;
struct iscsi_tpg_np *tpg_np;
int buffer_len, end_of_buf = 0, len = 0, payload_len = 0;
+ int target_name_printed;
unsigned char buf[ISCSI_IQN_LEN+12]; /* iqn + "TargetName=" + \0 */
unsigned char *text_in = cmd->text_in_ptr, *text_ptr = NULL;

@@ -3411,19 +3412,23 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
continue;
}

- len = sprintf(buf, "TargetName=%s", tiqn->tiqn);
- len += 1;
-
- if ((len + payload_len) > buffer_len) {
- end_of_buf = 1;
- goto eob;
- }
- memcpy(payload + payload_len, buf, len);
- payload_len += len;
+ target_name_printed = 0;

spin_lock(&tiqn->tiqn_tpg_lock);
list_for_each_entry(tpg, &tiqn->tiqn_tpg_list, tpg_list) {

+ /* If demo_mode_discovery=0 and generate_node_acls=0
+ * (demo mode dislabed) do not return
+ * TargetName+TargetAddress unless a NodeACL exists.
+ */
+
+ if ((tpg->tpg_attrib.generate_node_acls == 0) &&
+ (tpg->tpg_attrib.demo_mode_discovery == 0) &&
+ (!core_tpg_get_initiator_node_acl(&tpg->tpg_se_tpg,
+ cmd->conn->sess->sess_ops->InitiatorName))) {
+ continue;
+ }
+
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
@@ -3438,6 +3443,22 @@ static int iscsit_build_sendtargets_response(struct iscsi_cmd *cmd)
struct iscsi_np *np = tpg_np->tpg_np;
bool inaddr_any = iscsit_check_inaddr_any(np);

+ if (!target_name_printed) {
+ len = sprintf(buf, "TargetName=%s",
+ tiqn->tiqn);
+ len += 1;
+
+ if ((len + payload_len) > buffer_len) {
+ spin_unlock(&tpg->tpg_np_lock);
+ spin_unlock(&tiqn->tiqn_tpg_lock);
+ end_of_buf = 1;
+ goto eob;
+ }
+ memcpy(payload + payload_len, buf, len);
+ payload_len += len;
+ target_name_printed = 1;
+ }
+
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?

--
1.7.10.4

Nicholas A. Bellinger

2013-10-07 20:43:26 UTC

Post by Thomas Glanzmann
Hello Nab,

Post by Thomas Glanzmann
If hide_from_unauthorized=0 and generate_node_acls=0 (demo mode dislabed) do
not return TargetName+TargetAddress unless a NodeACL exists.

the commit comment was wrong because it is now called
demo_mode_discovery=0. I'm resending this patch.

Hey Thomas,

Please resend the patch series with PATCH-vX in --subject line, eg:

git format-patch --cover-letter -n --subject="PATCH-v3"

Thanks,

--nab

Thomas Glanzmann

2013-10-07 21:10:45 UTC

This patchset makes it possible to only discover targets the disocvering
initiator has an access permission for.

Thomas Glanzmann (3):
iscsi-target: Add new TPG attribute demo_mode_discovery
target: Export symbol core_tpg_check_initiator_node_acl
iscsi-target: Implement demo_mode_discovery logic

drivers/target/iscsi/iscsi_target.c | 39 ++++++++++++++++++++------
drivers/target/iscsi/iscsi_target_configfs.c | 6 ++++
drivers/target/iscsi/iscsi_target_core.h | 2 ++
drivers/target/iscsi/iscsi_target_tpg.c | 20 +++++++++++++
drivers/target/iscsi/iscsi_target_tpg.h | 1 +
drivers/target/target_core_internal.h | 2 --
drivers/target/target_core_tpg.c | 1 +
include/target/target_core_fabric.h | 2 ++
8 files changed, 62 insertions(+), 11 deletions(-)

--
1.7.10.4

Thomas Glanzmann

2013-10-07 21:12:11 UTC

--
1.7.10.4

Thomas Glanzmann

2013-10-07 21:13:02 UTC

Export symbol core_tpg_check_initiator_node_acl and move prototype from the
private drivers/target/target_core_internal.h to the public
include/target/target_core_fabric.h

Signed-off-by: Thomas Glanzmann <***@glanzmann.de>
---
drivers/target/target_core_internal.h | 2 --
drivers/target/target_core_tpg.c | 1 +
include/target/target_core_fabric.h | 2 ++
3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 579128a..889af4d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -75,8 +75,6 @@ extern struct se_device *g_lun0_dev;

struct se_node_acl *__core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
const char *);
-struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
- unsigned char *);
void core_tpg_add_node_to_devs(struct se_node_acl *, struct se_portal_group *);
void core_tpg_wait_for_nacl_pr_ref(struct se_node_acl *);
struct se_lun *core_tpg_pre_addlun(struct se_portal_group *, u32);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b9a6ec0..ec99220 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -116,6 +116,7 @@ struct se_node_acl *core_tpg_get_initiator_node_acl(

return acl;
}
+EXPORT_SYMBOL(core_tpg_get_initiator_node_acl);

/* core_tpg_add_node_to_devs():
*
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 882b650e..4cf4fda 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -137,6 +137,8 @@ void transport_generic_request_failure(struct se_cmd *, sense_reason_t);
void __target_execute_cmd(struct se_cmd *);
int transport_lookup_tmr_lun(struct se_cmd *, u32);

+struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
+ unsigned char *);
struct se_node_acl *core_tpg_check_initiator_node_acl(struct se_portal_group *,
unsigned char *);
void core_tpg_clear_object_luns(struct se_portal_group *);

--
1.7.10.4

Thomas Glanzmann

2013-10-07 21:13:46 UTC

--
1.7.10.4

Benjamin ESTRABAUD

2013-10-16 10:19:04 UTC

Post by Thomas Glanzmann
This patchset makes it possible to only discover targets the disocvering
initiator has an access permission for.

Thank you very much for this great patch! We tested it here a bit and it
works perfectly.

We'll let you know in case we do find an issue with more intensive testing.

Ben.

Post by Thomas Glanzmann
iscsi-target: Add new TPG attribute demo_mode_discovery
target: Export symbol core_tpg_check_initiator_node_acl
iscsi-target: Implement demo_mode_discovery logic
drivers/target/iscsi/iscsi_target.c | 39 ++++++++++++++++++++------
drivers/target/iscsi/iscsi_target_configfs.c | 6 ++++
drivers/target/iscsi/iscsi_target_core.h | 2 ++
drivers/target/iscsi/iscsi_target_tpg.c | 20 +++++++++++++
drivers/target/iscsi/iscsi_target_tpg.h | 1 +
drivers/target/target_core_internal.h | 2 --
drivers/target/target_core_tpg.c | 1 +
include/target/target_core_fabric.h | 2 ++
8 files changed, 62 insertions(+), 11 deletions(-)

Thomas Glanzmann

2013-10-16 10:41:38 UTC

Hello Ben

Post by Benjamin ESTRABAUD

Post by Thomas Glanzmann
This patchset makes it possible to only discover targets the disocvering
initiator has an access permission for.

Thank you very much for this great patch! We tested it here a bit and
it works perfectly. We'll let you know in case we do find an issue
with more intensive testing.

thank you for the feedback. I also did not see any issues with the
patch. I'll probably write another one to limit the visibility of demo
mode LUNs to a portal and post this here.

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-07 20:41:24 UTC

Post by Thomas Glanzmann
Hello Nab,

<SNIP>

Post by Thomas Glanzmann
I often have a configuration where I export multiple demo mode LUNs
in different VLANs but I only want initiators from same VLAN access
that LUN. To my knowledge that is currently not possible, is it?

I tried it, but it was still discovering all LUNs. Is the config right?
/backstores/fileio create shared-01.v101.campusvl.de /iscsi1/shared-01.v101.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.4
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.5
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/luns create /backstores/fileio/shared-01.v101.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1
/backstores/fileio create shared-01.v102.campusvl.de /iscsi1/shared-01.v102.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ create 2
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.4
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.5
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/luns create /backstores/fileio/shared-01.v102.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ delete 1
When I do a discovery I get both endpoints back.
shared-01.v101.campusvl.iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de and
iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de. Or did I misunderstand
howto set the TargetPortalGroupTag? Or did I miss something else or should it
not do that, if so I would go in the code and search for the problem.

So what I was thinking for the above is to configure a vlan device (say
eth0.5), and assign it a specific IP address. From there the vlan
device's IP address can be assigned as a network portal to individual
TargetName+TargetPortalGroupTag endpoints.

Post by Nicholas A. Bellinger
The rest looks reasonable to me. Nice work.

Prefect. Find v2 of the patches. I also tested them on top of the
iscsi-target: percpu_ida tag starvation regression fixes that you send
- 12 ESX servers, parallel rescan, 24 VMs deployed in parallel
and powered on in parallel and 24 svMotion in parallel,
unloading the target during operation, watch the slabinfo and
reloading it again.
All working perfectly fine. However I see the following log messages
which I did not see before when I did the 24 concurrent svMotion using
Oct 4 21:38:42 node-62 kernel: [ 934.369442] Unable to locate ITT: 0x0000b4aa on CID: 0
Oct 4 21:38:42 node-62 kernel: [ 934.374446] Unable to locate RefTaskTag: 0x0000b4aa on CID: 0.
But everything worked afterwards.
iSCSI (Text Response)
Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x000008ac
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0xb89e7744
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-03.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-04.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-05.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-06.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-07.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-08.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-09.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-10.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-11.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-12.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
find /sys/kernel/config/target/iscsi -name demo_mode_discovery | while read FILE; do echo 0 > $FILE; done
iSCSI (Text Response)
Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x000002be
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0xa33a9e16
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:esx-12.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
Padding: 0000
Btw. I used the line 'tshark -V -r /tmp/discover6.pcap -R iscsi.keyvalue |
less' to extract the above. Comes in handy.
As you can see I also get the targets from TGPT '2' discovered. Any idea what
I'm doing here wrong?

I'm a bit confused here. What is the discovery output that is expected
here..?

Post by Thomas Glanzmann
Nab, what do we do about the userland changes? Should I prepare a patch?
If so where can I find the repository for targetcli/rtsadmin?

That's the beauty of configfs + rtslib + targecli. Adding new TPG
attributes requires no changes to userspace. ;)

--nab

Thomas Glanzmann

2013-10-07 21:20:54 UTC

Hello Nab,

Post by Thomas Glanzmann
/backstores/fileio create shared-01.v101.campusvl.de /iscsi1/shared-01.v101.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.4
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.5
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/luns create /backstores/fileio/shared-01.v101.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1
/backstores/fileio create shared-01.v102.campusvl.de /iscsi1/shared-01.v102.campusvl.de size=80G buffered=true
/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ create 2
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.4
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/portals create 10.102.99.5
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/luns create /backstores/fileio/shared-01.v102.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt2/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/ delete 1

I thought I have done that. Haven't I?

VLAN 101:
TargetPortalGroupTag 1
Portal: 10.101.99.4
Portal: 10.101.99.5

VLAN 102:
TargetPortalGroupTag 2
Portal: 10.102.99.4
Portal: 10.102.99.5

When I discover on 10.101.99.4 I still see the demo_mode LUNs from
'TargetPortalGroupTag 2'.

Post by Thomas Glanzmann
iSCSI (Text Response)
Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x000002be
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0xa33a9e16
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.5:3260,2
KeyValue: TargetAddress=10.102.99.4:3260,2
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
Padding: 0000

I'm a bit confused here. What is the discovery output that is expected
here?

I don't want to see
TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de

Post by Nicholas A. Bellinger
That's the beauty of configfs + rtslib + targecli. Adding new TPG
attributes requires no changes to userspace. ;)

Cool.

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-07 23:23:44 UTC

Post by Thomas Glanzmann
Hello Nab,

I thought I have done that. Haven't I?
TargetPortalGroupTag 1
Portal: 10.101.99.4
Portal: 10.101.99.5
TargetPortalGroupTag 2
Portal: 10.102.99.4
Portal: 10.102.99.5
When I discover on 10.101.99.4 I still see the demo_mode LUNs from
'TargetPortalGroupTag 2'.

Mmmm, strange. I'm guessing this has something to do with using the
same TargetName and multiple TargetPortalGroupTags. The one limitation
of doing this is that the Initiator does not explicitly select which
TargetPortalGroupTag it wants to connect with during Login, and has to
rely upon network portal addresses for this.

Can you confirm what logging into 10.101.99.4 returns for key
TargetPortalGroupTag in the first login response payload..?

So separate from the above, I'd recommend using different TargetNames w/
TPGT=1 across the different VLANs, and managing endpoints by TargetNames
instead of TargetPortalGroupTags. This should work the way you expect
with different VLANs.

I'm a bit confused here. What is the discovery output that is expected
here?

I don't want to see
TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de

Please confirm that
iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de is
configured with generate_node_acls=0, and that no active sessions from
the initiator has created a NodeACL with se_node_acl->dynamic_node_acl

--nab

Thomas Glanzmann

2013-10-08 19:51:08 UTC

Hello Nab,

Post by Thomas Glanzmann
Hello Nab,

I'm guessing this has something to do with using the same TargetName
and multiple TargetPortalGroupTags.

I thought that I use two different TargetNames. Am I wrong?

/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
~ ~
The one limitation of doing this is that the Initiator does not
explicitly select which TargetPortalGroupTag it wants to connect with
during Login, and has to rely upon network portal addresses for this.
Can you confirm what logging into 10.101.99.4 returns for key
TargetPortalGroupTag in the first login response payload?
So separate from the above, I'd recommend using different TargetNames
w/ TPGT=1 across the different VLANs, and managing endpoints by
TargetNames instead of TargetPortalGroupTags. This should work the way
you expect with different VLANs.

I though that I did this from the beginning. Just to confirm a
Targetname is created by

/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de

If that is the case than I always had different target names and TPGT
was always 1 because it is the default and I did not change it.

Confirmed. In order to be sure I used the following Config (but I have
also other private LUNs on the system demo_mode_discovery is disabled
for them):

set global auto_cd_after_create=false
/backstores/fileio create shared-01.v101.campusvl.de /iscsi1/shared-01.v101.campusvl.de size=80G buffered=true
/backstores/fileio create shared-02.v101.campusvl.de /iscsi2/shared-02.v101.campusvl.de size=80G buffered=true

/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.4
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/portals create 10.101.99.5
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/luns create /backstores/fileio/shared-01.v101.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1

/iscsi create iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de/tpgt1/portals create 10.101.99.4
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de/tpgt1/portals create 10.101.99.5
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de/tpgt1/luns create /backstores/fileio/shared-02.v101.campusvl.de lun=20
/iscsi/iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1

set global auto_cd_after_create=false
/backstores/fileio create shared-01.v102.campusvl.de /iscsi1/shared-01.v102.campusvl.de size=80G buffered=true
/backstores/fileio create shared-02.v102.campusvl.de /iscsi2/shared-02.v102.campusvl.de size=80G buffered=true

/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt1/portals create 10.102.99.4
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt1/portals create 10.102.99.5
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt1/luns create /backstores/fileio/shared-01.v102.campusvl.de lun=10
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1

/iscsi create iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de/tpgt1/portals create 10.102.99.4
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de/tpgt1/portals create 10.102.99.5
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de/tpgt1/luns create /backstores/fileio/shared-02.v102.campusvl.de lun=20
/iscsi/iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de/tpgt1/ set attribute authentication=0 demo_mode_write_protect=0 generate_node_acls=1 cache_dynamic_acls=1

I isntalled open-iscsi on Debian sid, changed the iqn to make sure that
there are no cached node acls, than I did a discover and sniffed it.
Here is the output:

(miniwheezy64) [~] iscsiadm -m discovery -t sendtargets -p 10.101.99.4
10.101.99.4:3260,1 iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
10.101.99.5:3260,1 iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
10.101.99.4:3260,1 iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
10.101.99.5:3260,1 iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
10.102.99.4:3260,1 iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
10.102.99.5:3260,1 iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
10.102.99.4:3260,1 iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
10.102.99.5:3260,1 iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de

I also sniffed the same from the target. Please note that there are duplicated
packets in the pcap because I did a '-i any' the system has bonding configured.
So I see the packet once for the physical interface and the bond interface.

https://thomas.glanzmann.de/tmp/discovery.pcap

And I can confirm in the sniffer that I got the discovery:

Opcode: Text Response (0x24)
Flags: 0x80
1... .... = F: Final PDU in sequence
.0.. .... = C: Text is complete
TotalAHSLength: 0x00
DataSegmentLength: 0x00000234
LUN: 0000000000000000
InitiatorTaskTag: 0x00000001
TargetTransferTag: 0xffffffff
StatSN: 0x9dee00da
ExpCmdSN: 0x00000002
MaxCmdSN: 0x00000002
Key/Value Pairs
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-01.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v101.storage:shared-02.v101.campusvl.de
KeyValue: TargetAddress=10.101.99.4:3260,1
KeyValue: TargetAddress=10.101.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.4:3260,1
KeyValue: TargetAddress=10.102.99.5:3260,1
KeyValue: TargetName=iqn.2013-03.de.campusvl.v102.storage:shared-02.v102.campusvl.de
KeyValue: TargetAddress=10.102.99.4:3260,1
KeyValue: TargetAddress=10.102.99.5:3260,1

Please let me know what the behaviour should be than I dig into the code and
write a patch. It can't be hard to find.

Cheers,
Thomas

Thomas Glanzmann

2013-10-08 19:58:35 UTC

Hello Nab,

Post by Nicholas A. Bellinger
Please confirm that
iqn.2013-03.de.campusvl.v102.storage:shared-01.v102.campusvl.de is
configured with generate_node_acls=0, and that no active sessions from
the initiator has created a NodeACL with se_node_acl->dynamic_node_acl

I missed the 'generate_node_acls=0' part. When I turn that one of, I do
not discover the public LUNs on the portal itself and the discovery
returns nothing:

(miniwheezy64) [~] iscsiadm -m discovery -t sendtargets -p 10.101.99.4
iscsiadm: No portals found

I'm trying do discover the demo LUNs thata re on that portal but not the ones
which are on the portal in the different subnet.

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-08 21:19:13 UTC

Post by Thomas Glanzmann
Hello Nab,

I missed the 'generate_node_acls=0' part. When I turn that one of, I do
not discover the public LUNs on the portal itself and the discovery
(miniwheezy64) [~] iscsiadm -m discovery -t sendtargets -p 10.101.99.4
iscsiadm: No portals found
I'm trying do discover the demo LUNs thata re on that portal but not the ones
which are on the portal in the different subnet.

Ok, I'm now getting more confused as to what your trying to accomplish
with these patches.

The original discussion was to restrict SendTargets discovery
information for specific TargetName+TargetPortalGroupTag endpoints,
based upon how the endpoint was configured using TPG attributes.

Thus far it has included the following criteria:

- If demo-mode (generate_node_acls=1) is enabled on the TargetName+TargetPortalGroup
endpoint, always return TargetName regardless of other settings
- If demo-mode (generate_node_acls=0) is disabled, only return TargetName if NodeACL
exists that matches the InitiatorName for the discovery session.

So from what I can tell the changes in your PATCH-v3 achieve these
points.

Where I'm getting confused is where portals and subnets are now involved
in the discussion. The above criteria will not restrict SendTargets
discovery based on which portal the initiator is connecting to, but only
restricts SendTargets discovery based upon how each of the endpoints are
configured.

Eg: The initiator will always see the same restricted set of SendTargets
discovery information based on the above criteria, regardless of which
network portal it's actually connecting to.

--nab

Thomas Glanzmann

2013-10-08 21:35:05 UTC

Hello Nab,

Post by Nicholas A. Bellinger
Ok, I'm now getting more confused as to what your trying to accomplish
with these patches.
The original discussion was to restrict SendTargets discovery
information for specific TargetName+TargetPortalGroupTag endpoints,
based upon how the endpoint was configured using TPG attributes.
- If demo-mode (generate_node_acls=1) is enabled on the TargetName+TargetPortalGroup
endpoint, always return TargetName regardless of other settings
- If demo-mode (generate_node_acls=0) is disabled, only return TargetName if NodeACL
exists that matches the InitiatorName for the discovery session.
So from what I can tell the changes in your PATCH-v3 achieve these
points.

I agree with the above. The PATCH-v3 accomplishes that and works
perfectly fine for me. I run it on top of everything else that you have
sent me and I'll report back by the end of the week when I have finished
my class.

(node-62) [~/work/linux-2.6] git shortlog remotes/nab/queue..HEAD
Nicholas Bellinger (6):
iscsi-target: Only perform wait_for_tasks when performing shutdown
iscsi-target: Perform release of acknowledged tags from RX context
iscsi-target; Allow an extra tag_num / 2 number of percpu_ida tags
target: Make target_do_xcopy failures return INVALID_PARAMETER_LIST
target: Allow non zero ListID in EXTENDED_COPY parameter list
target: Reject EXTENDED_COPY when emulate_3pc is disabled

Thomas Glanzmann (3):
iscsi-target: Add new TPG attribute demo_mode_discovery
target: Export symbol core_tpg_check_initiator_node_acl
iscsi-target: Implement demo_mode_discovery logic
Linux node-62 3.12.0-rc3+ #1 SMP Tue Oct 8 22:32:58 CEST 2013 x86_64 GNU/Linux

Post by Nicholas A. Bellinger
Where I'm getting confused is where portals and subnets are now involved
in the discussion. The above criteria will not restrict SendTargets
discovery based on which portal the initiator is connecting to, but only
restricts SendTargets discovery based upon how each of the endpoints are
configured.

While I was working on the patch, I noticed, that I wanted more: I also
wanted to not expose demo mode luns from other portals. This is
currently a pending task since I have not accomplished that yet. As I
see it I have two options here:

- Drop the demo mode and set explicit access permissions (I
don't want that)

- Write a patch that only returns targets which are bound to the
portal

Post by Nicholas A. Bellinger
Eg: The initiator will always see the same restricted set of SendTargets
discovery information based on the above criteria, regardless of which
network portal it's actually connecting to.

I can confirm that. Would you be willing to take a patch that changes
that or do you consider it fundamentally broken?

Do you have an alternative? I have the following scenario:

I have 10 VLANs. They're seperated by a firewall so that one VLAN can
not access the other VLAN. Each VLAN has 12 ESX Servers in it. I have one storage.
This storage is used by all 10 VLANs. On the storage I have two IPs in
every VLAN. I want to be able to provide 'LUNs which can only be
accessed by the ESX server' and I want LUNs which can be accessed by
everyone in the VLAN not taking the iqn into account. Before PATCH-v3
all LUNs were discovered which LED to many unsucessfull logins and long
reboot times, because the ESX initiators tried to log in into every
single target. With PATCH-v3 this is now much better, but the ESX still
try to login to all demo mode LUNs, two they can reach all other fail
because they do not reach them over layer 3 because of the firewall.

Cheers,
Thomas

Thomas Glanzmann

2013-10-09 13:10:39 UTC

Hello Nab,

Post by Thomas Glanzmann
(node-62) [~/work/linux-2.6] git shortlog remotes/nab/queue..HEAD
iscsi-target: Only perform wait_for_tasks when performing shutdown
iscsi-target: Perform release of acknowledged tags from RX context
iscsi-target; Allow an extra tag_num / 2 number of percpu_ida tags
target: Make target_do_xcopy failures return INVALID_PARAMETER_LIST
target: Allow non zero ListID in EXTENDED_COPY parameter list
target: Reject EXTENDED_COPY when emulate_3pc is disabled
iscsi-target: Add new TPG attribute demo_mode_discovery
target: Export symbol core_tpg_check_initiator_node_acl
iscsi-target: Implement demo_mode_discovery logic

we just did 24 concurrent svMotions using 8 ESX server without any
problems whatsoever. I consider the above stable. We also conducted 8
RESCANs in parallel.

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-09 20:53:34 UTC

Post by Thomas Glanzmann
Hello Nab,

we just did 24 concurrent svMotions using 8 ESX server without any
problems whatsoever. I consider the above stable. We also conducted 8
RESCANs in parallel.

Excellent, thanks for the update. FYI, the three EXTENDED_COPY fixes
have been pushed to target-pending/master, and will be headed to
mainline next week for -rc6 code.

Thanks again Thomas!

--nab

Thomas Glanzmann

2013-10-09 21:00:49 UTC

Hello Nab,

Excellent, thanks for the update. FYI, the three EXTENDED_COPY fixes
have been pushed to target-pending/master, and will be headed to
mainline next week for -rc6 code.

perfect. :-)

Thanks again Thomas!

Thank you for getting to the bottom of every single bug I report in a
very timely fashion. I really like the current tip. And having the
demo_mode_discovery option made iscsi target much better for my current
scenario. :-)

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-09 20:51:18 UTC

Post by Thomas Glanzmann
Hello Nab,

Ok, good. Just making sure the objective for the first patch series is
mutually understood. ;)

While I was working on the patch, I noticed, that I wanted more: I also
wanted to not expose demo mode luns from other portals. This is
currently a pending task since I have not accomplished that yet. As I
- Drop the demo mode and set explicit access permissions (I
don't want that)
- Write a patch that only returns targets which are bound to the
portal

I can confirm that. Would you be willing to take a patch that changes
that or do you consider it fundamentally broken?

Mmm, so far I've avoided NodeACLs based on IP address entirely, so
adding this specifically for in-band sendtargets discovery is something
that I'd rather avoid.

Coding this up for your internal use is fine, but it's most likely
something that I'll not be merging.

Post by Thomas Glanzmann
I have 10 VLANs. They're seperated by a firewall so that one VLAN can
not access the other VLAN. Each VLAN has 12 ESX Servers in it. I have one storage.
This storage is used by all 10 VLANs. On the storage I have two IPs in
every VLAN. I want to be able to provide 'LUNs which can only be
accessed by the ESX server' and I want LUNs which can be accessed by
everyone in the VLAN not taking the iqn into account. Before PATCH-v3
all LUNs were discovered which LED to many unsucessfull logins and long
reboot times, because the ESX initiators tried to log in into every
single target. With PATCH-v3 this is now much better, but the ESX still
try to login to all demo mode LUNs, two they can reach all other fail
because they do not reach them over layer 3 because of the firewall.

OK, this exactly what explicit NodeACLs are intended for, and given how
the PATCH-v3 demo_mode_discovery logic now works, correctly hides
TargetNames from Initiators who don't have an explicit NodeACL set.

--nab

Thomas Glanzmann

2013-10-09 20:55:10 UTC

Hello Nab,

Post by Nicholas A. Bellinger
Ok, good. Just making sure the objective for the first patch series
is mutually understood. ;)

yes, let me know if you want to have anything else cleaned up. I
consider it ready for upstream.

Post by Nicholas A. Bellinger
Mmm, so far I've avoided NodeACLs based on IP address entirely, so
adding this specifically for in-band sendtargets discovery is something
that I'd rather avoid.
Coding this up for your internal use is fine, but it's most likely
something that I'll not be merging.

I assumed so.

The thing is the following: This is a lab environment. So people might
play with the initiator iqn. If they do so they would loose access to
the storage. I don't want that because I want to make it as seamless
for the user as possible, so that really is not an option for me. About
the three private LUNs with NodeACLs: They're only needed for two labs,
so I don't care that much about this. But making the shared storage
unavailable because someone screwed up the iqn of the initiator, would
be a no go. However I see your point. You consider that broken and I
think we had that discussion a while back ago. So maybe I'll make
something ready and use it and post it here. If you like it or someone
else, he can pick it up, otherwise, git makes it easy to do vendor
tracking while maintaing a private patch or two. I do the same for
screen, mutt, you name it. :-)

I really like the current tip. If you want me to stress test anything
else let me know. It is shame that I currently don't have FC or 10 GBit.
I would like to try the FCOE and FC code as well.

Cheers,
Thomas

Nicholas A. Bellinger

2013-10-03 22:14:05 UTC

This post might be inappropriate. Click to display it.

Benjamin ESTRABAUD

2013-10-04 16:02:54 UTC

Hi Thomas,

Thank you very much for this patch, we'll be testing it soon here too
and we'll let you know if we find any issues with it.

Regards,
Ben.

Post by Thomas Glanzmann
Hello Nab,

done. Please let me know if you like the attribute name
hide_from_unauthorized or if I should change it?

done.

I thought about this several hours, but need advice from you. I thought
about cleaning up this function the following ways but discarded it for
- Normal
- IFC_SENDTARGETS_SINGLE
- hide_from_unauthorized
I discarded this option because it results in a lot of
duplicated code.
- Putting if (cmd->cmd_flags & IFC_SENDTARGETS_SINGLE) {
text_ptr = strchr(text_in, '=');
...
}
in a seperate function. And put the sprintf ...
payload_len += len; parts in a seperate function.
Make the function shorter but not much nicer.
- Now that I put my thoughts in writing I should have probably
done both.
Please give me advice on howto pretty up the existing code.

Is the spinlock necessary? I think it is not because the tpg_state is an
enum that is mapped to a byte, int or long int. The access to it is
atomic. So I don't think we need it.

With the current code path it is possible that TargetName is printed but
TargetAddress is not because the payload buffer runs out of space. Is
this okay, or should we change it?
I often have a configuration where I export multiple demo mode LUNs
in different VLANs but I only want initiators from same VLAN access that
LUN. To my knowledge that is currently not possible, is it? Would you
accept another patch which only exposes demo mode LUNs if a portal
exists for that LUN for the same ip address as the discovery is
targeted or another patch which gives me same functionality but maybe
does it another way?
Cheers,
Thomas

Thomas Glanzmann

2013-10-04 16:35:42 UTC

Hello Ben,

Post by Benjamin ESTRABAUD
Thank you very much for this patch, we'll be testing it soon here too
and we'll let you know if we find any issues with it.

this patch does not work. There is one line is missing. I'm currently
conducting a stress test and will post v2 of the patch soon, when I have
testd it throughoughly, but it works, that is for sure. I confirmed
using tcpdump and wireshark. The filter for wireshark is:
iscsi.keyvalues.

Cheers,
Thomas

Benjamin ESTRABAUD

2013-09-16 11:41:20 UTC