Re: [PATCH 10/16] nvme-fabrics: parse options 'keyring' and 'tls_key'

[Hannes Reinecke <hare@suse.de>]

On 8/9/23 11:56, Sagi Grimberg wrote:
> 
> 
> On 8/8/23 19:53, Hannes Reinecke wrote:
>> Parse the fabrics options 'keyring' and 'tls_key' and store the
>> referenced keys in the options structure.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/fabrics.c | 71 ++++++++++++++++++++++++++++++++++++-
>>   drivers/nvme/host/fabrics.h |  6 ++++
>>   drivers/nvme/host/tcp.c     | 11 ++++--
>>   3 files changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index ddad482c3537..df986a7641a3 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -622,6 +622,41 @@ static struct nvmf_transport_ops 
>> *nvmf_lookup_transport(
>>       return NULL;
>>   }
>> +static int parse_key(struct nvmf_ctrl_options *opts, int key_id,
>> +             bool is_key)
>> +{
>> +    struct key *key = NULL;
>> +
>> +    if (key_id < 0) {
>> +        pr_err("Invalid %s id %d\n",
>> +               is_key ? "key" : "keyring", key_id);
>> +        return -EINVAL;
>> +    }
>> +    if (key_id) {
>> +        key = key_lookup(key_id);
>> +        if (!key) {
>> +            pr_err("%s id %08x not found\n",
>> +                   is_key ? "Key" : "Keyring", key_id);
>> +            return -ENOKEY;
>> +        }
>> +    } else {
>> +        if (is_key)
>> +            pr_debug("Using 'best' PSK\n");
>> +        else
>> +            pr_debug("Using default keyring\n");
>> +        key = NULL;
> 
> Not sure I understand the else case here.
> Why do we allow key_id = 0 if it does not represent
> a valid existing key_id?
> 
No, you are right. While there is a default keyring, there is no
point in allowing '0' as a value here; if the user wants to use
the default keyring he shoudn't have set the '--keyring' option.

>> +    }
>> +    if (is_key) {
>> +        key_put(opts->tls_key);
>> +        opts->tls_key = key;
>> +    } else {
>> +        key_put(opts->keyring);
>> +        opts->keyring = key;
>> +    }
>> +
> 
> Perhaps instead of is_key, just return the key and assign outside.

Could be. Lemme see.

Cheers,

Hannes