[PATCH 0/3] target: iscsi: control authentication per ACL

[PATCH 0/3] target: iscsi: control authentication per ACL

[Dmitry Bogdanov]

Add acls/{ACL}/attrib/authentication attribute that controls authentication
for the particular ACL. By default, this attribute inherits a value of
authentication attribute of the target port group to keep a backward
compatibility.

authentication attribute has 3 states:
"0" - authentication is turned off for this ACL
"1" - authentication is required for this ACL
"" - authentication is inherited from TPG

This patchset is intended for scsi-queue.

Dmitry Bogdanov (3):
  scsi: target: iscsi: Add upcast helpers
  scsi: target: iscsi: extract auth functions
  target: iscsi: control authentication per ACL

 drivers/target/iscsi/iscsi_target_configfs.c  | 126 ++++++++-------
 drivers/target/iscsi/iscsi_target_nego.c      | 148 ++++++++++++------
 .../target/iscsi/iscsi_target_nodeattrib.c    |   1 +
 drivers/target/iscsi/iscsi_target_tpg.c       |   3 +-
 include/target/iscsi/iscsi_target_core.h      |  14 ++
 5 files changed, 186 insertions(+), 106 deletions(-)

-- 
2.25.1

[PATCH 1/3] scsi: target: iscsi: Add upcast helpers

[Dmitry Bogdanov]

iscsi target is cluttered with open-coded container_of conversions from
se_nacl to iscsi_node_acl. The code could be cleaned by introducing a
helper - to_iscsi_nacl() (similar to other helpers in target core).

While at it, make another iscsi conversion helper consistent
and rename iscsi_tpg() helper to to_iscsi_tpg().

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/iscsi/iscsi_target_configfs.c | 85 +++++++-------------
 drivers/target/iscsi/iscsi_target_nego.c     | 14 ++--
 drivers/target/iscsi/iscsi_target_tpg.c      |  3 +-
 include/target/iscsi/iscsi_target_core.h     | 12 +++
 4 files changed, 50 insertions(+), 64 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index f4a24fa5058e..e3750b64cc0c 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -210,7 +210,7 @@ static struct se_tpg_np *lio_target_call_addnptotpg(
 		return ERR_PTR(ret);
 	}
 
-	tpg = container_of(se_tpg, struct iscsi_portal_group, tpg_se_tpg);
+	tpg = to_iscsi_tpg(se_tpg);
 	ret = iscsit_get_tpg(tpg);
 	if (ret < 0)
 		return ERR_PTR(-EINVAL);
@@ -281,9 +281,7 @@ static ssize_t iscsi_nacl_attrib_##name##_show(struct config_item *item,\
 		char *page)						\
 {									\
 	struct se_node_acl *se_nacl = attrib_to_nacl(item);		\
-	struct iscsi_node_acl *nacl = container_of(se_nacl, struct iscsi_node_acl, \
-					se_node_acl);			\
-									\
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);		\
 	return sprintf(page, "%u\n", nacl->node_attrib.name);		\
 }									\
 									\
@@ -291,8 +289,7 @@ static ssize_t iscsi_nacl_attrib_##name##_store(struct config_item *item,\
 		const char *page, size_t count)				\
 {									\
 	struct se_node_acl *se_nacl = attrib_to_nacl(item);		\
-	struct iscsi_node_acl *nacl = container_of(se_nacl, struct iscsi_node_acl, \
-					se_node_acl);			\
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);		\
 	u32 val;							\
 	int ret;							\
 									\
@@ -377,15 +374,14 @@ static ssize_t iscsi_nacl_auth_##name##_show(struct config_item *item,	\
 		char *page)						\
 {									\
 	struct se_node_acl *nacl = auth_to_nacl(item);			\
-	return __iscsi_nacl_auth_##name##_show(container_of(nacl,	\
-			struct iscsi_node_acl, se_node_acl), page);	\
+	return __iscsi_nacl_auth_##name##_show(to_iscsi_nacl(nacl), page);	\
 }									\
 static ssize_t iscsi_nacl_auth_##name##_store(struct config_item *item,	\
 		const char *page, size_t count)				\
 {									\
 	struct se_node_acl *nacl = auth_to_nacl(item);			\
-	return __iscsi_nacl_auth_##name##_store(container_of(nacl,	\
-			struct iscsi_node_acl, se_node_acl), page, count); \
+	return __iscsi_nacl_auth_##name##_store(to_iscsi_nacl(nacl),	\
+						page, count); \
 }									\
 									\
 CONFIGFS_ATTR(iscsi_nacl_auth_, name)
@@ -417,8 +413,7 @@ static ssize_t iscsi_nacl_auth_##name##_show(struct config_item *item,	\
 		char *page)						\
 {									\
 	struct se_node_acl *nacl = auth_to_nacl(item);			\
-	return __iscsi_nacl_auth_##name##_show(container_of(nacl,	\
-			struct iscsi_node_acl, se_node_acl), page);	\
+	return __iscsi_nacl_auth_##name##_show(to_iscsi_nacl(nacl), page);	\
 }									\
 									\
 CONFIGFS_ATTR_RO(iscsi_nacl_auth_, name)
@@ -623,8 +618,7 @@ static ssize_t lio_target_nacl_cmdsn_depth_store(struct config_item *item,
 {
 	struct se_node_acl *se_nacl = acl_to_nacl(item);
 	struct se_portal_group *se_tpg = se_nacl->se_tpg;
-	struct iscsi_portal_group *tpg = container_of(se_tpg,
-			struct iscsi_portal_group, tpg_se_tpg);
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);
 	struct config_item *acl_ci, *tpg_ci, *wwn_ci;
 	u32 cmdsn_depth = 0;
 	int ret;
@@ -700,8 +694,7 @@ static struct configfs_attribute *lio_target_initiator_attrs[] = {
 static int lio_target_init_nodeacl(struct se_node_acl *se_nacl,
 		const char *name)
 {
-	struct iscsi_node_acl *acl =
-		container_of(se_nacl, struct iscsi_node_acl, se_node_acl);
+	struct iscsi_node_acl *acl = to_iscsi_nacl(se_nacl);
 
 	config_group_init_type_name(&acl->node_stat_grps.iscsi_sess_stats_group,
 			"iscsi_sess_stats", &iscsi_stat_sess_cit);
@@ -720,8 +713,7 @@ static ssize_t iscsi_tpg_attrib_##name##_show(struct config_item *item,	\
 		char *page)						\
 {									\
 	struct se_portal_group *se_tpg = attrib_to_tpg(item);		\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,		\
-			struct iscsi_portal_group, tpg_se_tpg);	\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);		\
 	ssize_t rb;							\
 									\
 	if (iscsit_get_tpg(tpg) < 0)					\
@@ -736,8 +728,7 @@ static ssize_t iscsi_tpg_attrib_##name##_store(struct config_item *item,\
 		const char *page, size_t count)				\
 {									\
 	struct se_portal_group *se_tpg = attrib_to_tpg(item);		\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,		\
-			struct iscsi_portal_group, tpg_se_tpg);	\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);		\
 	u32 val;							\
 	int ret;							\
 									\
@@ -800,8 +791,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
 static ssize_t __iscsi_##prefix##_##name##_show(struct se_portal_group *se_tpg,	\
 		char *page)							\
 {										\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,			\
-				struct iscsi_portal_group, tpg_se_tpg);		\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);			\
 	struct iscsi_node_auth *auth = &tpg->tpg_demo_auth;			\
 										\
 	if (!capable(CAP_SYS_ADMIN))						\
@@ -813,8 +803,7 @@ static ssize_t __iscsi_##prefix##_##name##_show(struct se_portal_group *se_tpg,
 static ssize_t __iscsi_##prefix##_##name##_store(struct se_portal_group *se_tpg,\
 		const char *page, size_t count)					\
 {										\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,			\
-				struct iscsi_portal_group, tpg_se_tpg);		\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);			\
 	struct iscsi_node_auth *auth = &tpg->tpg_demo_auth;			\
 										\
 	if (!capable(CAP_SYS_ADMIN))						\
@@ -861,8 +850,7 @@ DEF_TPG_AUTH_STR(password_mutual, NAF_PASSWORD_IN_SET);
 static ssize_t __iscsi_##prefix##_##name##_show(struct se_portal_group *se_tpg,	\
 		char *page)								\
 {										\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,			\
-				struct iscsi_portal_group, tpg_se_tpg);		\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);			\
 	struct iscsi_node_auth *auth = &tpg->tpg_demo_auth;			\
 										\
 	if (!capable(CAP_SYS_ADMIN))						\
@@ -900,8 +888,7 @@ static ssize_t iscsi_tpg_param_##name##_show(struct config_item *item,	\
 		char *page)						\
 {									\
 	struct se_portal_group *se_tpg = param_to_tpg(item);		\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,		\
-			struct iscsi_portal_group, tpg_se_tpg);		\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);		\
 	struct iscsi_param *param;					\
 	ssize_t rb;							\
 									\
@@ -923,8 +910,7 @@ static ssize_t iscsi_tpg_param_##name##_store(struct config_item *item, \
 		const char *page, size_t count)				\
 {									\
 	struct se_portal_group *se_tpg = param_to_tpg(item);		\
-	struct iscsi_portal_group *tpg = container_of(se_tpg,		\
-			struct iscsi_portal_group, tpg_se_tpg);		\
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);		\
 	char *buf;							\
 	int ret, len;							\
 									\
@@ -1008,8 +994,7 @@ static struct configfs_attribute *lio_target_tpg_param_attrs[] = {
 static ssize_t lio_target_tpg_enable_show(struct config_item *item, char *page)
 {
 	struct se_portal_group *se_tpg = to_tpg(item);
-	struct iscsi_portal_group *tpg = container_of(se_tpg,
-			struct iscsi_portal_group, tpg_se_tpg);
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);
 	ssize_t len;
 
 	spin_lock(&tpg->tpg_state_lock);
@@ -1024,8 +1009,7 @@ static ssize_t lio_target_tpg_enable_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct se_portal_group *se_tpg = to_tpg(item);
-	struct iscsi_portal_group *tpg = container_of(se_tpg,
-			struct iscsi_portal_group, tpg_se_tpg);
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);
 	u32 op;
 	int ret;
 
@@ -1134,7 +1118,7 @@ static void lio_target_tiqn_deltpg(struct se_portal_group *se_tpg)
 	struct iscsi_portal_group *tpg;
 	struct iscsi_tiqn *tiqn;
 
-	tpg = container_of(se_tpg, struct iscsi_portal_group, tpg_se_tpg);
+	tpg = to_iscsi_tpg(se_tpg);
 	tiqn = tpg->tpg_tiqn;
 	/*
 	 * iscsit_tpg_del_portal_group() assumes force=1
@@ -1408,46 +1392,41 @@ static void lio_aborted_task(struct se_cmd *se_cmd)
 	cmd->conn->conn_transport->iscsit_aborted_task(cmd->conn, cmd);
 }
 
-static inline struct iscsi_portal_group *iscsi_tpg(struct se_portal_group *se_tpg)
-{
-	return container_of(se_tpg, struct iscsi_portal_group, tpg_se_tpg);
-}
-
 static char *lio_tpg_get_endpoint_wwn(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_tiqn->tiqn;
+	return to_iscsi_tpg(se_tpg)->tpg_tiqn->tiqn;
 }
 
 static u16 lio_tpg_get_tag(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpgt;
+	return to_iscsi_tpg(se_tpg)->tpgt;
 }
 
 static u32 lio_tpg_get_default_depth(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_attrib.default_cmdsn_depth;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.default_cmdsn_depth;
 }
 
 static int lio_tpg_check_demo_mode(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_attrib.generate_node_acls;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.generate_node_acls;
 }
 
 static int lio_tpg_check_demo_mode_cache(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_attrib.cache_dynamic_acls;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.cache_dynamic_acls;
 }
 
 static int lio_tpg_check_demo_mode_write_protect(
 	struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_attrib.demo_mode_write_protect;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.demo_mode_write_protect;
 }
 
 static int lio_tpg_check_prod_mode_write_protect(
 	struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_attrib.prod_mode_write_protect;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.prod_mode_write_protect;
 }
 
 static int lio_tpg_check_prot_fabric_only(
@@ -1457,9 +1436,9 @@ static int lio_tpg_check_prot_fabric_only(
 	 * Only report fabric_prot_type if t10_pi has also been enabled
 	 * for incoming ib_isert sessions.
 	 */
-	if (!iscsi_tpg(se_tpg)->tpg_attrib.t10_pi)
+	if (!to_iscsi_tpg(se_tpg)->tpg_attrib.t10_pi)
 		return 0;
-	return iscsi_tpg(se_tpg)->tpg_attrib.fabric_prot_type;
+	return to_iscsi_tpg(se_tpg)->tpg_attrib.fabric_prot_type;
 }
 
 /*
@@ -1496,16 +1475,14 @@ static void lio_tpg_close_session(struct se_session *se_sess)
 
 static u32 lio_tpg_get_inst_index(struct se_portal_group *se_tpg)
 {
-	return iscsi_tpg(se_tpg)->tpg_tiqn->tiqn_index;
+	return to_iscsi_tpg(se_tpg)->tpg_tiqn->tiqn_index;
 }
 
 static void lio_set_default_node_attributes(struct se_node_acl *se_acl)
 {
-	struct iscsi_node_acl *acl = container_of(se_acl, struct iscsi_node_acl,
-				se_node_acl);
+	struct iscsi_node_acl *acl = to_iscsi_nacl(se_acl);
 	struct se_portal_group *se_tpg = se_acl->se_tpg;
-	struct iscsi_portal_group *tpg = container_of(se_tpg,
-				struct iscsi_portal_group, tpg_se_tpg);
+	struct iscsi_portal_group *tpg = to_iscsi_tpg(se_tpg);
 
 	acl->node_attrib.nacl = acl;
 	iscsit_set_default_node_attribues(acl, tpg);
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index c0ed6f8e5c5b..f0769708e4fb 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -104,8 +104,8 @@ static u32 iscsi_handle_authentication(
 {
 	struct iscsi_session *sess = conn->sess;
 	struct iscsi_node_auth *auth;
-	struct iscsi_node_acl *iscsi_nacl;
-	struct iscsi_portal_group *iscsi_tpg;
+	struct iscsi_node_acl *nacl;
+	struct iscsi_portal_group *tpg;
 	struct se_node_acl *se_nacl;
 
 	if (!sess->sess_ops->SessionType) {
@@ -120,15 +120,13 @@ static u32 iscsi_handle_authentication(
 		}
 
 		if (se_nacl->dynamic_node_acl) {
-			iscsi_tpg = container_of(se_nacl->se_tpg,
-					struct iscsi_portal_group, tpg_se_tpg);
+			tpg = to_iscsi_tpg(se_nacl->se_tpg);
 
-			auth = &iscsi_tpg->tpg_demo_auth;
+			auth = &tpg->tpg_demo_auth;
 		} else {
-			iscsi_nacl = container_of(se_nacl, struct iscsi_node_acl,
-						  se_node_acl);
+			nacl = to_iscsi_nacl(se_nacl);
 
-			auth = &iscsi_nacl->node_auth;
+			auth = &nacl->node_auth;
 		}
 	} else {
 		/*
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index 8075f60fd02c..7410387d52e1 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -394,8 +394,7 @@ struct iscsi_node_attrib *iscsit_tpg_get_node_attrib(
 {
 	struct se_session *se_sess = sess->se_sess;
 	struct se_node_acl *se_nacl = se_sess->se_node_acl;
-	struct iscsi_node_acl *acl = container_of(se_nacl, struct iscsi_node_acl,
-					se_node_acl);
+	struct iscsi_node_acl *acl = to_iscsi_nacl(se_nacl);
 
 	return &acl->node_attrib;
 }
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..21c1aaa6dae2 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -757,6 +757,12 @@ struct iscsi_node_acl {
 	struct iscsi_node_stat_grps node_stat_grps;
 };
 
+static inline struct iscsi_node_acl *
+to_iscsi_nacl(struct se_node_acl *se_nacl)
+{
+	return container_of(se_nacl, struct iscsi_node_acl, se_node_acl);
+}
+
 struct iscsi_tpg_attrib {
 	u32			authentication;
 	u32			login_timeout;
@@ -838,6 +844,12 @@ struct iscsi_portal_group {
 	struct list_head	tpg_list;
 } ____cacheline_aligned;
 
+static inline struct iscsi_portal_group *
+to_iscsi_tpg(struct se_portal_group *se_tpg)
+{
+	return container_of(se_tpg, struct iscsi_portal_group, tpg_se_tpg);
+}
+
 struct iscsi_wwn_stat_grps {
 	struct config_group	iscsi_stat_group;
 	struct config_group	iscsi_instance_group;
-- 
2.25.1

[PATCH 2/3] scsi: target: iscsi: extract auth functions

[Dmitry Bogdanov]

Create functions that answers simple questions:
whether authentication is required, what credentials, whether
connection is autenticated.

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 140 +++++++++++++++--------
 1 file changed, 92 insertions(+), 48 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index f0769708e4fb..006fa679517a 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -94,6 +94,31 @@ int extract_param(
 	return 0;
 }
 
+static struct iscsi_node_auth *iscsi_get_node_auth(struct iscsi_conn *conn)
+{
+	struct iscsi_portal_group *tpg;
+	struct iscsi_node_acl *nacl;
+	struct se_node_acl *se_nacl;
+
+	if (conn->sess->sess_ops->SessionType)
+		return &iscsit_global->discovery_acl.node_auth;
+
+	se_nacl = conn->sess->se_sess->se_node_acl;
+	if (!se_nacl) {
+		pr_err("Unable to locate struct se_node_acl for CHAP auth\n");
+		return NULL;
+	}
+
+	if (se_nacl->dynamic_node_acl) {
+		tpg = to_iscsi_tpg(se_nacl->se_tpg);
+		return &tpg->tpg_demo_auth;
+	}
+
+	nacl = to_iscsi_nacl(se_nacl);
+
+	return &nacl->node_auth;
+}
+
 static u32 iscsi_handle_authentication(
 	struct iscsi_conn *conn,
 	char *in_buf,
@@ -102,38 +127,11 @@ static u32 iscsi_handle_authentication(
 	int *out_length,
 	unsigned char *authtype)
 {
-	struct iscsi_session *sess = conn->sess;
 	struct iscsi_node_auth *auth;
-	struct iscsi_node_acl *nacl;
-	struct iscsi_portal_group *tpg;
-	struct se_node_acl *se_nacl;
-
-	if (!sess->sess_ops->SessionType) {
-		/*
-		 * For SessionType=Normal
-		 */
-		se_nacl = conn->sess->se_sess->se_node_acl;
-		if (!se_nacl) {
-			pr_err("Unable to locate struct se_node_acl for"
-					" CHAP auth\n");
-			return -1;
-		}
-
-		if (se_nacl->dynamic_node_acl) {
-			tpg = to_iscsi_tpg(se_nacl->se_tpg);
-
-			auth = &tpg->tpg_demo_auth;
-		} else {
-			nacl = to_iscsi_nacl(se_nacl);
 
-			auth = &nacl->node_auth;
-		}
-	} else {
-		/*
-		 * For SessionType=Discovery
-		 */
-		auth = &iscsit_global->discovery_acl.node_auth;
-	}
+	auth = iscsi_get_node_auth(conn);
+	if (!auth)
+		return -1;
 
 	if (strstr("CHAP", authtype))
 		strcpy(conn->sess->auth_type, "CHAP");
@@ -813,6 +811,37 @@ static int iscsi_target_do_authentication(
 	return 0;
 }
 
+bool iscsi_conn_auth_required(struct iscsi_conn *conn)
+{
+	struct se_node_acl *se_nacl;
+
+	if (conn->sess->sess_ops->SessionType) {
+		/*
+		 * For SessionType=Discovery
+		 */
+		return conn->tpg->tpg_attrib.authentication;
+	}
+	/*
+	 * For SessionType=Normal
+	 */
+	se_nacl = conn->sess->se_sess->se_node_acl;
+	if (!se_nacl) {
+		pr_debug("Unknown ACL %s is trying to connect\n",
+			 se_nacl->initiatorname);
+		return true;
+	}
+
+	if (se_nacl->dynamic_node_acl) {
+		pr_debug("Dynamic ACL %s is trying to connect\n",
+			 se_nacl->initiatorname);
+		return conn->tpg->tpg_attrib.authentication;
+	}
+
+	pr_debug("Known ACL %s is trying to connect\n",
+		 se_nacl->initiatorname);
+	return conn->tpg->tpg_attrib.authentication;
+}
+
 static int iscsi_target_handle_csg_zero(
 	struct iscsi_conn *conn,
 	struct iscsi_login *login)
@@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
 		return -1;
 
 	if (!iscsi_check_negotiated_keys(conn->param_list)) {
-		if (conn->tpg->tpg_attrib.authentication &&
-		    !strncmp(param->value, NONE, 4)) {
-			pr_err("Initiator sent AuthMethod=None but"
-				" Target is enforcing iSCSI Authentication,"
-					" login failed.\n");
-			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
-					ISCSI_LOGIN_STATUS_AUTH_FAILED);
-			return -1;
-		}
+		bool auth_required = iscsi_conn_auth_required(conn);
+
+		if (auth_required) {
+			if (!strncmp(param->value, NONE, 4)) {
+				pr_err("Initiator sent AuthMethod=None but"
+				       " Target is enforcing iSCSI Authentication,"
+				       " login failed.\n");
+				iscsit_tx_login_rsp(conn,
+						ISCSI_STATUS_CLS_INITIATOR_ERR,
+						ISCSI_LOGIN_STATUS_AUTH_FAILED);
+				return -1;
+			}
 
-		if (conn->tpg->tpg_attrib.authentication &&
-		    !login->auth_complete)
-			return 0;
+			if (!login->auth_complete)
+				return 0;
 
-		if (strncmp(param->value, NONE, 4) && !login->auth_complete)
-			return 0;
+			if (strncmp(param->value, NONE, 4) &&
+			    !login->auth_complete)
+				return 0;
+		}
 
 		if ((login_req->flags & ISCSI_FLAG_LOGIN_NEXT_STAGE1) &&
 		    (login_req->flags & ISCSI_FLAG_LOGIN_TRANSIT)) {
@@ -904,6 +937,18 @@ static int iscsi_target_handle_csg_zero(
 	return iscsi_target_do_authentication(conn, login);
 }
 
+static bool iscsi_conn_authenticated(struct iscsi_conn *conn,
+				     struct iscsi_login *login)
+{
+	if (!iscsi_conn_auth_required(conn))
+		return true;
+
+	if (login->auth_complete)
+		return true;
+
+	return false;
+}
+
 static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_login *login)
 {
 	int ret;
@@ -947,11 +992,10 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn *conn, struct iscsi_log
 		return -1;
 	}
 
-	if (!login->auth_complete &&
-	     conn->tpg->tpg_attrib.authentication) {
+	if (!iscsi_conn_authenticated(conn, login)) {
 		pr_err("Initiator is requesting CSG: 1, has not been"
-			 " successfully authenticated, and the Target is"
-			" enforcing iSCSI Authentication, login failed.\n");
+		       " successfully authenticated, and the Target is"
+		       " enforcing iSCSI Authentication, login failed.\n");
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
 				ISCSI_LOGIN_STATUS_AUTH_FAILED);
 		return -1;
-- 
2.25.1

Re: [PATCH 2/3] scsi: target: iscsi: extract auth functions

[Mike Christie]

On 9/14/21 5:03 AM, Dmitry Bogdanov wrote:
> +bool iscsi_conn_auth_required(struct iscsi_conn *conn)
> +{
> +	struct se_node_acl *se_nacl;
> +
> +	if (conn->sess->sess_ops->SessionType) {
> +		/*
> +		 * For SessionType=Discovery
> +		 */
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +	/*
> +	 * For SessionType=Normal
> +	 */
> +	se_nacl = conn->sess->se_sess->se_node_acl;
> +	if (!se_nacl) {
> +		pr_debug("Unknown ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return true;

Before the patch, if we didn't have an ACL we would go by
conn->tpg->tpg_attrib.authentication. But with the patch, if
we don't have an ACL, then it looks like we always require authentication
which I don't think is right.

Is the code above supposed to return the value of 
conn->tpg->tpg_attrib.authentication?


> +	}
> +
> +	if (se_nacl->dynamic_node_acl) {
> +		pr_debug("Dynamic ACL %s is trying to connect\n",
> +			 se_nacl->initiatorname);
> +		return conn->tpg->tpg_attrib.authentication;
> +	}
> +
> +	pr_debug("Known ACL %s is trying to connect\n",
> +		 se_nacl->initiatorname);
> +	return conn->tpg->tpg_attrib.authentication;
> +}
> +
>  static int iscsi_target_handle_csg_zero(
>  	struct iscsi_conn *conn,
>  	struct iscsi_login *login)
> @@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
>  		return -1;
>  
>  	if (!iscsi_check_negotiated_keys(conn->param_list)) {
> -		if (conn->tpg->tpg_attrib.authentication &&
> -		    !strncmp(param->value, NONE, 4)) {
> -			pr_err("Initiator sent AuthMethod=None but"
> -				" Target is enforcing iSCSI Authentication,"
> -					" login failed.\n");
> -			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
> -					ISCSI_LOGIN_STATUS_AUTH_FAILED);
> -			return -1;
> -		}
> +		bool auth_required = iscsi_conn_auth_required(conn);
> +

In __iscsi_target_login_thread we have:

        if (conn->sess)
                conn->sess->se_sess->sup_prot_ops =
                        conn->conn_transport->iscsit_get_sup_prot_ops(conn);

before we call:

iscsi_target_start_negotiation -> iscsi_target_do_login- > iscsi_target_handle_csg_zero
and hit the code above.

If conn->sess can be NULL then iscsi_conn_auth_required will crash.

However, I can't tell how conn->sess can be NULL in that code path. Is the conn->sess
check in __iscsi_target_login_thread not needed?

RE: [PATCH 2/3] scsi: target: iscsi: extract auth functions

[Dmitriy Bogdanov]

Hi Mike,

> > +{
> > +	struct se_node_acl *se_nacl;
> > +
> > +	if (conn->sess->sess_ops->SessionType) {
> > +		/*
> > +		 * For SessionType=Discovery
> > +		 */
> > +		return conn->tpg->tpg_attrib.authentication;
> > +	}
> > +	/*
> > +	 * For SessionType=Normal
> > +	 */
> > +	se_nacl = conn->sess->se_sess->se_node_acl;
> > +	if (!se_nacl) {
> > +		pr_debug("Unknown ACL %s is trying to connect\n",
> > +			 se_nacl->initiatorname);
> > +		return true;
>
> Before the patch, if we didn't have an ACL we would go by
> conn->tpg->tpg_attrib.authentication. But with the patch, if
> we don't have an ACL, then it looks like we always require authentication
> which I don't think is right.
>
> Is the code above supposed to return the value of 
> conn->tpg->tpg_attrib.authentication?

No, no. This piece of code is the same as it was.
An absence of ACL is some erroneous situation because the login must be
rejected earlier in __iscsi_target_login_thread -> iscsi_target_locate_portal

> > +	}
> > +
> > +	if (se_nacl->dynamic_node_acl) {
> > +		pr_debug("Dynamic ACL %s is trying to connect\n",
> > +			 se_nacl->initiatorname);
> > +		return conn->tpg->tpg_attrib.authentication;
> > +	}
> > +
> > +	pr_debug("Known ACL %s is trying to connect\n",
> > +		 se_nacl->initiatorname);
> > +	return conn->tpg->tpg_attrib.authentication;
> > +}
> > +
> >  static int iscsi_target_handle_csg_zero(
> >  	struct iscsi_conn *conn,
> >  	struct iscsi_login *login)
> > @@ -874,22 +903,26 @@ static int iscsi_target_handle_csg_zero(
> >  		return -1;
> >  
> >  	if (!iscsi_check_negotiated_keys(conn->param_list)) {
> > -		if (conn->tpg->tpg_attrib.authentication &&
> > -		    !strncmp(param->value, NONE, 4)) {
> > -			pr_err("Initiator sent AuthMethod=None but"
> > -				" Target is enforcing iSCSI Authentication,"
> > -					" login failed.\n");
> > -			iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
> > -					ISCSI_LOGIN_STATUS_AUTH_FAILED);
> > -			return -1;
> > -		}
> > +		bool auth_required = iscsi_conn_auth_required(conn);
> > +
>
> In __iscsi_target_login_thread we have:
>
>         if (conn->sess)
>                 conn->sess->se_sess->sup_prot_ops =
>                         conn->conn_transport->iscsit_get_sup_prot_ops(conn);
>
> before we call:
>
> iscsi_target_start_negotiation -> iscsi_target_do_login- > iscsi_target_handle_csg_zero
> and hit the code above.
>
> If conn->sess can be NULL then iscsi_conn_auth_required will crash.
>
> However, I can't tell how conn->sess can be NULL in that code path. Is the conn->sess
> check in __iscsi_target_login_thread not needed?

conn->sess is set to NULL in iscsi_login_non_zero_tsih_s1 and  new session is created
in iscsi_login_non_zero_tsih_s2 which is before iscsi_target_start_negotiation, so
we are safe.

BR,
 Dmitry

[PATCH 3/3] target: iscsi: control authentication per ACL

[Dmitry Bogdanov]

Add acls/{ACL}/attrib/authentication attribute that controls authentication
for the particular ACL. By default, this attribute inherits a value of
authentication attribute of the target port group to keep backward
compatibility.

authentication attribute has 3 states:
 "0" - authentication is turned off for this ACL
 "1" - authentication is required for this ACL
 "" - authentication is inherited from TPG

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/iscsi/iscsi_target_configfs.c  | 41 +++++++++++++++++++
 drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
 .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
 include/target/iscsi/iscsi_target_core.h      |  2 +
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index e3750b64cc0c..2d70de342408 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
 ISCSI_NACL_ATTR(random_datain_seq_offsets);
 ISCSI_NACL_ATTR(random_r2t_offsets);
 
+static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
+		char *page)
+{
+	struct se_node_acl *se_nacl = attrib_to_nacl(item);
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
+
+	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
+		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
+
+		return sprintf(page, "%u (inherited)\n",
+				tpg->tpg_attrib.authentication);
+	}
+	return sprintf(page, "%u\n", nacl->node_attrib.authentication);
+}
+
+static ssize_t iscsi_nacl_attrib_authentication_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct se_node_acl *se_nacl = attrib_to_nacl(item);
+	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
+	s32 val;
+	int ret;
+
+	if (sysfs_streq(page, "")) {
+		val = NA_AUTHENTICATION_INHERITED;
+	} else {
+		ret = kstrtos32(page, 0, &val);
+		if (ret)
+			return ret;
+		if (val != 0 && val != 1)
+			return -EINVAL;
+	}
+
+	nacl->node_attrib.authentication = val;
+
+	return count;
+}
+
+CONFIGFS_ATTR(iscsi_nacl_attrib_, authentication);
+
 static struct configfs_attribute *lio_target_nacl_attrib_attrs[] = {
 	&iscsi_nacl_attrib_attr_dataout_timeout,
 	&iscsi_nacl_attrib_attr_dataout_timeout_retries,
@@ -323,6 +363,7 @@ static struct configfs_attribute *lio_target_nacl_attrib_attrs[] = {
 	&iscsi_nacl_attrib_attr_random_datain_pdu_offsets,
 	&iscsi_nacl_attrib_attr_random_datain_seq_offsets,
 	&iscsi_nacl_attrib_attr_random_r2t_offsets,
+	&iscsi_nacl_attrib_attr_authentication,
 	NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 006fa679517a..9873c5e34206 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -813,6 +813,7 @@ static int iscsi_target_do_authentication(
 
 bool iscsi_conn_auth_required(struct iscsi_conn *conn)
 {
+	struct iscsi_node_acl *nacl;
 	struct se_node_acl *se_nacl;
 
 	if (conn->sess->sess_ops->SessionType) {
@@ -839,7 +840,12 @@ bool iscsi_conn_auth_required(struct iscsi_conn *conn)
 
 	pr_debug("Known ACL %s is trying to connect\n",
 		 se_nacl->initiatorname);
-	return conn->tpg->tpg_attrib.authentication;
+
+	nacl = to_iscsi_nacl(se_nacl);
+	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED)
+		return conn->tpg->tpg_attrib.authentication;
+
+	return nacl->node_attrib.authentication;
 }
 
 static int iscsi_target_handle_csg_zero(
diff --git a/drivers/target/iscsi/iscsi_target_nodeattrib.c b/drivers/target/iscsi/iscsi_target_nodeattrib.c
index e3ac247bffe8..baf1c93fa1e3 100644
--- a/drivers/target/iscsi/iscsi_target_nodeattrib.c
+++ b/drivers/target/iscsi/iscsi_target_nodeattrib.c
@@ -30,6 +30,7 @@ void iscsit_set_default_node_attribues(
 {
 	struct iscsi_node_attrib *a = &acl->node_attrib;
 
+	a->authentication = NA_AUTHENTICATION_INHERITED;
 	a->dataout_timeout = NA_DATAOUT_TIMEOUT;
 	a->dataout_timeout_retries = NA_DATAOUT_TIMEOUT_RETRIES;
 	a->nopin_timeout = NA_NOPIN_TIMEOUT;
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 21c1aaa6dae2..0913909fa765 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -26,6 +26,7 @@ struct sock;
 #define ISCSI_RX_THREAD_NAME		"iscsi_trx"
 #define ISCSI_TX_THREAD_NAME		"iscsi_ttx"
 #define ISCSI_IQN_LEN			224
+#define NA_AUTHENTICATION_INHERITED	-1
 
 /* struct iscsi_node_attrib sanity values */
 #define NA_DATAOUT_TIMEOUT		3
@@ -714,6 +715,7 @@ struct iscsi_login {
 } ____cacheline_aligned;
 
 struct iscsi_node_attrib {
+	s32			authentication;
 	u32			dataout_timeout;
 	u32			dataout_timeout_retries;
 	u32			default_erl;
-- 
2.25.1

Re: [PATCH 3/3] target: iscsi: control authentication per ACL

[Mike Christie]

On 9/14/21 5:03 AM, Dmitry Bogdanov wrote:
> Add acls/{ACL}/attrib/authentication attribute that controls authentication
> for the particular ACL. By default, this attribute inherits a value of
> authentication attribute of the target port group to keep backward
> compatibility.
> 
> authentication attribute has 3 states:
>  "0" - authentication is turned off for this ACL
>  "1" - authentication is required for this ACL
>  "" - authentication is inherited from TPG


Why the empty string for this value? Maybe 2 or -1?


> 
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> ---
>  drivers/target/iscsi/iscsi_target_configfs.c  | 41 +++++++++++++++++++
>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>  include/target/iscsi/iscsi_target_core.h      |  2 +
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index e3750b64cc0c..2d70de342408 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>  ISCSI_NACL_ATTR(random_r2t_offsets);
>  
> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
> +		char *page)
> +{
> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
> +
> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
> +
> +		return sprintf(page, "%u (inherited)\n",
> +				tpg->tpg_attrib.authentication);


I think we want a value of -1 or 2 for inherited then here it should print
that value.

RE: [PATCH 3/3] target: iscsi: control authentication per ACL

[Dmitriy Bogdanov]

Hi Mike,

> > Add acls/{ACL}/attrib/authentication attribute that controls authentication
> > for the particular ACL. By default, this attribute inherits a value of
> > authentication attribute of the target port group to keep backward
> > compatibility.
> > 
> > authentication attribute has 3 states:
> >  "0" - authentication is turned off for this ACL
> >  "1" - authentication is required for this ACL
> >  "" - authentication is inherited from TPG
>
>
> Why the empty string for this value? Maybe 2 or -1?
That was design decision by logic that since that attribute has a precedence 
to clear that precedence we must clear the attribute, i.e. set to the empty value.

> > 
> > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> > ---
> >  drivers/target/iscsi/iscsi_target_configfs.c  | 41 +++++++++++++++++++
> >  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
> >  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
> >  include/target/iscsi/iscsi_target_core.h      |  2 +
> >  4 files changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> > index e3750b64cc0c..2d70de342408 100644
> > --- a/drivers/target/iscsi/iscsi_target_configfs.c
> > +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> > @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
> >  ISCSI_NACL_ATTR(random_datain_seq_offsets);
> >  ISCSI_NACL_ATTR(random_r2t_offsets);
> >  
> > +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
> > +		char *page)
> > +{
> > +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
> > +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
> > +
> > +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
> > +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
> > +
> > +		return sprintf(page, "%u (inherited)\n",
> > +				tpg->tpg_attrib.authentication);
>
>
> I think we want a value of -1 or 2 for inherited then here it should print
> that value.

We decided to hide the internal value from userspace and represent it similar to
tpg.authentication to have the same handling there.

BR,
 Dmitry

Re: [PATCH 3/3] target: iscsi: control authentication per ACL

[Mike Christie]

On 10/4/21 2:56 AM, Dmitriy Bogdanov wrote:
> Hi Mike,
> 
>>> Add acls/{ACL}/attrib/authentication attribute that controls authentication
>>> for the particular ACL. By default, this attribute inherits a value of
>>> authentication attribute of the target port group to keep backward
>>> compatibility.
>>>
>>> authentication attribute has 3 states:
>>>  "0" - authentication is turned off for this ACL
>>>  "1" - authentication is required for this ACL
>>>  "" - authentication is inherited from TPG
>>
>>
>> Why the empty string for this value? Maybe 2 or -1?
> That was design decision by logic that since that attribute has a precedence 
> to clear that precedence we must clear the attribute, i.e. set to the empty value.
> 
>>>
>>> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
>>> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
>>> ---
>>>  drivers/target/iscsi/iscsi_target_configfs.c  | 41 +++++++++++++++++++
>>>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>>>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>>>  include/target/iscsi/iscsi_target_core.h      |  2 +
>>>  4 files changed, 51 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>>> index e3750b64cc0c..2d70de342408 100644
>>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>>> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>>>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>>>  ISCSI_NACL_ATTR(random_r2t_offsets);
>>>  
>>> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
>>> +		char *page)
>>> +{
>>> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
>>> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
>>> +
>>> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
>>> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
>>> +
>>> +		return sprintf(page, "%u (inherited)\n",
>>> +				tpg->tpg_attrib.authentication);
>>
>>
>> I think we want a value of -1 or 2 for inherited then here it should print
>> that value.
> 
> We decided to hide the internal value from userspace and represent it similar to
> tpg.authentication to have the same handling there.

I'm not sure what you meant by representing it similar to tpg.authentication. That
attrib, and I think every attrib, prints 1 value.

The problem with above is that this works by accident for rtslib based apps which
read in the attribs, stores them, then on restore writes them to the kernel. On the
read/save stage we get "0 (inherited)". Then on the restore stage we try to write
that back to the kernel and get an error. rtslib/targetcli just spits out an error
and ignores it, so it still works since the kernel used the default. We don't
really want the error spit out and I don't think we want it to work by accident like
this.

RE: [PATCH 3/3] target: iscsi: control authentication per ACL

[Dmitriy Bogdanov]

Hi Mike,

>>>> Add acls/{ACL}/attrib/authentication attribute that controls authentication
>>>> for the particular ACL. By default, this attribute inherits a value of
>>>> authentication attribute of the target port group to keep backward
>>>> compatibility.
>>>>
>>>> authentication attribute has 3 states:
>>>>  "0" - authentication is turned off for this ACL
>>>>  "1" - authentication is required for this ACL
>>>>  "" - authentication is inherited from TPG
>>>
>>>
>>> Why the empty string for this value? Maybe 2 or -1?
>> That was design decision by logic that since that attribute has a precedence 
>> to clear that precedence we must clear the attribute, i.e. set to the empty value.
>> 
>>>>
>>>> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>>> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
>>>> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
>>>> ---
>>>>  drivers/target/iscsi/iscsi_target_configfs.c  | 41 +++++++++++++++++++
>>>>  drivers/target/iscsi/iscsi_target_nego.c      |  8 +++-
>>>>  .../target/iscsi/iscsi_target_nodeattrib.c    |  1 +
>>>>  include/target/iscsi/iscsi_target_core.h      |  2 +
>>>>  4 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> index e3750b64cc0c..2d70de342408 100644
>>>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>>>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>>>>  ISCSI_NACL_ATTR(random_datain_seq_offsets);
>>>>  ISCSI_NACL_ATTR(random_r2t_offsets);
>>>>  
>>>> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
>>>> +		char *page)
>>>> +{
>>>> +	struct se_node_acl *se_nacl = attrib_to_nacl(item);
>>>> +	struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
>>>> +
>>>> +	if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
>>>> +		struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
>>>> +
>>>> +		return sprintf(page, "%u (inherited)\n",
>>>> +				tpg->tpg_attrib.authentication);
>>>
>>>
>>> I think we want a value of -1 or 2 for inherited then here it should print
>>> that value.
>> 
>> We decided to hide the internal value from userspace and represent it similar to
>> tpg.authentication to have the same handling there.
>
> I'm not sure what you meant by representing it similar to tpg.authentication. That
> attrib, and I think every attrib, prints 1 value.
>
> The problem with above is that this works by accident for rtslib based apps which
> read in the attribs, stores them, then on restore writes them to the kernel. On the
> read/save stage we get "0 (inherited)". Then on the restore stage we try to write
> that back to the kernel and get an error. rtslib/targetcli just spits out an error
> and ignores it, so it still works since the kernel used the default. We don't
> really want the error spit out and I don't think we want it to work by accident like
> this.

I missed that fact that rtslib saves/restores all attributes. You are right then, I need to
report some effective value (I think '-1'). Will send new patchset soon.

BR,
 Dmitry