[PATCH] target: remove the auth_type field from iscsi_session

[PATCH] target: remove the auth_type field from iscsi_session

[Maurizio Lombardi]

This field is not used anymore so we can remove it.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 5 -----
 include/target/iscsi/iscsi_target_core.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 151e2949bb75..36341ffaffbf 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -144,11 +144,6 @@ static u32 iscsi_handle_authentication(
 		auth = &iscsit_global->discovery_acl.node_auth;
 	}
 
-	if (strstr("CHAP", authtype))
-		strcpy(conn->sess->auth_type, "CHAP");
-	else
-		strcpy(conn->sess->auth_type, NONE);
-
 	if (strstr("None", authtype))
 		return 1;
 	else if (strstr("CHAP", authtype))
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..f0495515ca6a 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -647,7 +647,6 @@ struct iscsi_session {
 
 	/* LIO specific session ID */
 	u32			sid;
-	char			auth_type[8];
 	/* unique within the target */
 	int			session_index;
 	/* Used for session reference counting */
-- 
Maurizio Lombardi

Re: [PATCH] target: remove the auth_type field from iscsi_session

[Roman Bolshakov]

On Tue, Jun 08, 2021 at 06:40:47PM +0200, Maurizio Lombardi wrote:
> This field is not used anymore so we can remove it.
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/iscsi/iscsi_target_nego.c | 5 -----
>  include/target/iscsi/iscsi_target_core.h | 1 -
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index 151e2949bb75..36341ffaffbf 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -144,11 +144,6 @@ static u32 iscsi_handle_authentication(
>  		auth = &iscsit_global->discovery_acl.node_auth;
>  	}
>  
> -	if (strstr("CHAP", authtype))
> -		strcpy(conn->sess->auth_type, "CHAP");
> -	else
> -		strcpy(conn->sess->auth_type, NONE);
> -

Hi Maurizio,

It might still be useful to carry the meaning of "effective auth_type"
in case of complex auth configuration. Otherwise there's no way to check
what auth settings took effect for a particular session/I_T nexus.

I think we should rather print auth_type value someplace in configfs
than delete the field altogether.

Regards,
Roman

>  	if (strstr("None", authtype))
>  		return 1;
>  	else if (strstr("CHAP", authtype))
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 1eccb2ac7d02..f0495515ca6a 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -647,7 +647,6 @@ struct iscsi_session {
>  
>  	/* LIO specific session ID */
>  	u32			sid;
> -	char			auth_type[8];
>  	/* unique within the target */
>  	int			session_index;
>  	/* Used for session reference counting */
> -- 
> Maurizio Lombardi
> 

Re: [PATCH] target: remove the auth_type field from iscsi_session

[Maurizio Lombardi]

st 9. 6. 2021 v 1:14 odesílatel Roman Bolshakov <r.bolshakov@yadro.com> napsal:
>
> Hi Maurizio,
>
> It might still be useful to carry the meaning of "effective auth_type"
> in case of complex auth configuration. Otherwise there's no way to check
> what auth settings took effect for a particular session/I_T nexus.
>
> I think we should rather print auth_type value someplace in configfs
> than delete the field altogether.

Ok I see what you mean.

If acls are used, identifying the CHAP-protected sessions is
trivial... you just have
to look under configfs /tptg_1/acls/.../auth and tptg_1/acls/.../info

If dynamic sessions are allowed and the tgt parameter AuthMethod  is
"CHAP,None",
you could end up having some initiators using CHAP and some not...
AFAIK, in this case, there is currently no way to find out if a
particular session used CHAP or not.

If it could really be useful to know that, then one possible solution
is to add this
information to the "dynamic_sessions" list in configfs,
but I'm not really sure this is acceptable because it could break the
user applications
that rely on this list.

Another solution that comes to my mind is to create a new configfs
node "sessions_info"
that contains a list of all connected initiators, their iqns,
authentication method etc.
but if the list is too long it could be truncated (attribute's max
size is PAGE_SIZE).

Or we could create a new configfs directory "sessions" and each
session would have it's own
entry there.

Maurizio



>
> Regards,
> Roman
>
> >       if (strstr("None", authtype))
> >               return 1;
> >       else if (strstr("CHAP", authtype))
> > diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> > index 1eccb2ac7d02..f0495515ca6a 100644
> > --- a/include/target/iscsi/iscsi_target_core.h
> > +++ b/include/target/iscsi/iscsi_target_core.h
> > @@ -647,7 +647,6 @@ struct iscsi_session {
> >
> >       /* LIO specific session ID */
> >       u32                     sid;
> > -     char                    auth_type[8];
> >       /* unique within the target */
> >       int                     session_index;
> >       /* Used for session reference counting */
> > --
> > Maurizio Lombardi
> >
>

Re: [PATCH] target: remove the auth_type field from iscsi_session

[Roman Bolshakov]

On Wed, Jun 09, 2021 at 12:22:36PM +0200, Maurizio Lombardi wrote:
> st 9. 6. 2021 v 1:14 odesílatel Roman Bolshakov <r.bolshakov@yadro.com> napsal:
> >
> > It might still be useful to carry the meaning of "effective auth_type"
> > in case of complex auth configuration. Otherwise there's no way to check
> > what auth settings took effect for a particular session/I_T nexus.
> >
> > I think we should rather print auth_type value someplace in configfs
> > than delete the field altogether.
> 
> Ok I see what you mean.
> 
> If acls are used, identifying the CHAP-protected sessions is
> trivial... you just have
> to look under configfs /tptg_1/acls/.../auth and tptg_1/acls/.../info
> 
> If dynamic sessions are allowed and the tgt parameter AuthMethod  is
> "CHAP,None",
> you could end up having some initiators using CHAP and some not...
> AFAIK, in this case, there is currently no way to find out if a
> particular session used CHAP or not.
> 
> If it could really be useful to know that, then one possible solution
> is to add this
> information to the "dynamic_sessions" list in configfs,
> but I'm not really sure this is acceptable because it could break the
> user applications
> that rely on this list.
> 
> Another solution that comes to my mind is to create a new configfs
> node "sessions_info"
> that contains a list of all connected initiators, their iqns,
> authentication method etc.
> but if the list is too long it could be truncated (attribute's max
> size is PAGE_SIZE).
> 
> Or we could create a new configfs directory "sessions" and each
> session would have it's own
> entry there.
> 

Hi Maurizio,

Yes per-session file would work. I think about sessions directory in
debugfs (/sys/kernel/debug/target/sessions/<session-name>) with the
following contents:

  * "info" file that contains aggregated information about a
    session

  * per-command directories named after command tag where we can inspect
    every command in se_sess_map of the session

    * "info" file for the command that would allow to read exact command
      state including transport-specific details and probably command
      addresses.

The reason why debugfs is preferred over configfs is the following.
There could be cases where configfs entries couldn't be destroyed due to
a command stuck in the session. In the case we can't read configfs and
have no fallback way to figure out the reason of the TCM lock-up. All
commands to access the session might hang.

If we create debugfs sessions entry right after session init and destroy
it just before session is freed. Then we can read it while the session
as long as it's alive.

Reading from "<session>/<tag>/info" needs to be consistent but shouldn't
have an impact on the hot path. We might need to introduce tag/se_cmd
generation for each command in the session. The generation is increased
on every get_tag()/command allocation. That'd allow printing routine to
detect that content of the debugfs is stale and read should be
retried/EAGAIN or aborted.

It's very raw thoughts but hopefully I'll come up with something that
assists in session debugging.

Regards,
Roman

Re: [PATCH] target: remove the auth_type field from iscsi_session

[michael.christie]

On 6/9/21 5:22 AM, Maurizio Lombardi wrote:
> st 9. 6. 2021 v 1:14 odesílatel Roman Bolshakov <r.bolshakov@yadro.com> napsal:
>>
>> Hi Maurizio,
>>
>> It might still be useful to carry the meaning of "effective auth_type"
>> in case of complex auth configuration. Otherwise there's no way to check
>> what auth settings took effect for a particular session/I_T nexus.
>>
>> I think we should rather print auth_type value someplace in configfs
>> than delete the field altogether.
> 
> Ok I see what you mean.
> 
> If acls are used, identifying the CHAP-protected sessions is
> trivial... you just have
> to look under configfs /tptg_1/acls/.../auth and tptg_1/acls/.../info
> 
> If dynamic sessions are allowed and the tgt parameter AuthMethod  is
> "CHAP,None",
> you could end up having some initiators using CHAP and some not...
> AFAIK, in this case, there is currently no way to find out if a
> particular session used CHAP or not.
> 
> If it could really be useful to know that, then one possible solution
> is to add this
> information to the "dynamic_sessions" list in configfs,
> but I'm not really sure this is acceptable because it could break the
> user applications
> that rely on this list.
> 
> Another solution that comes to my mind is to create a new configfs
> node "sessions_info"
> that contains a list of all connected initiators, their iqns,
> authentication method etc.
> but if the list is too long it could be truncated (attribute's max
> size is PAGE_SIZE).
> 
> Or we could create a new configfs directory "sessions" and each
> session would have it's own
> entry there.

I did that here if you guys want it:

https://www.spinics.net/lists/linux-scsi/msg143621.html