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

[Mike Christie <michael.christie@oracle.com>]

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.