| Message ID | 20210914100314.492-4-d.bogdanov@yadro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: iscsi: control authentication per ACL | expand |
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.
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
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.
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
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;
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(-)