| Message ID | 20210608164047.128763-1-mlombard@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: remove the auth_type field from iscsi_session | expand |
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 >
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 > > >
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
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
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 */
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(-)