[PATCH] nvme: add sysfs attribute 'tls_keyring'

[Hannes Reinecke]

On 3/8/24 15:01, Sagi Grimberg wrote:
> 
> 
> On 08/03/2024 14:10, Hannes Reinecke wrote:
>> Add a sysfs attribute 'tls_keyring' to hold the contents for the
>> 'keyring' fabrics option. This option is only meaningful if the
>> 'tls' option has been specified; if the 'tls_key' option is specified
>> the key is looked up directly and the keyring setting is ignored.
>> So blank out the keyring value when 'tls_key' is specified.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/nvme/host/sysfs.c | 55 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 6c7f1d5c056f..7b65ff7426c9 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -5,11 +5,12 @@
>>    * Copyright (c) 2011-2014, Intel Corporation.
>>    */
>> -#include <linux/nvme-auth.h>
>> -
>>   #include "nvme.h"
>>   #include "fabrics.h"
>> +#include <linux/nvme-auth.h>
>> +#include <linux/nvme-keyring.h>
>> +
>>   static ssize_t nvme_sysfs_reset(struct device *dev,
>>                   struct device_attribute *attr, const char *buf,
>>                   size_t count)
>> @@ -677,6 +678,52 @@ static ssize_t tls_key_show(struct device *dev,
>>       return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
>>   }
>>   static DEVICE_ATTR_RO(tls_key);
>> +
>> +static ssize_t tls_keyring_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> +    struct key *keyring;
>> +
>> +    if (!ctrl->tls_key)
>> +        return 0;
>> +    /* Do not display the keyring for user-selected keys */
>> +    if (ctrl->opts->tls_key)
>> +        return 0;
>> +    keyring = ctrl->opts->keyring;
>> +    if (!keyring) {
>> +        keyring = key_lookup(nvme_keyring_id());
>> +        if (!keyring)
>> +            return -EINVAL;
> 
> Weird if this failed...
> 
Yeah. But feels even weirder to _not_ check it.
WARN_ON?

>> +    }
>> +    return sysfs_emit(buf, "%s", keyring->description);
>> +}
>> +
>> +static ssize_t tls_keyring_store(struct device *dev,
>> +        struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> +    int err;
>> +    u32 key_id;
>> +    struct key *keyring;
>> +
>> +    if (!strlen(buf))
>> +        return -EINVAL;
>> +    err = kstrtou32(buf, 0, &key_id);
>> +    if (err)
>> +        return err;
>> +    keyring = key_lookup(key_id);
>> +    if (IS_ERR(keyring)) {
>> +        pr_err("key %08x not found\n", key_id);
>> +        return -EINVAL;
>> +    }
>> +    if (ctrl->opts->keyring)
>> +        key_put(ctrl->opts->keyring);
>> +    ctrl->opts->keyring = keyring;
>> +    return 0;
>> +}
> 
> Why should there be a _store() for this?
> 
Technically one can change the keyring. But I can drop it for now,
and revisit the issue once I get around to fully test key revokation
and key rotation.

>> +static DEVICE_ATTR(tls_keyring, S_IRUGO | S_IWUSR,
>> +        tls_keyring_show, tls_keyring_store);
>>   #endif
>>   static struct attribute *nvme_dev_attrs[] = {
>> @@ -708,6 +755,7 @@ static struct attribute *nvme_dev_attrs[] = {
>>   #endif
>>   #ifdef CONFIG_NVME_TCP_TLS
>>       &dev_attr_tls_key.attr,
>> +    &dev_attr_tls_keyring.attr,
>>   #endif
>>       &dev_attr_adm_passthru_err_log_enabled.attr,
>>       NULL
>> @@ -743,6 +791,9 @@ static umode_t nvme_dev_attrs_are_visible(struct 
>> kobject *kobj,
>>       if (a == &dev_attr_tls_key.attr &&
>>           (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>>           return 0;
>> +    if (a == &dev_attr_tls_keyring.attr &&
>> +        (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>> +        return 0;
> 
> Wouldn't it make better sense to expose it only if tls was requested?

Sort of. But we never did for the 'tls_key' attribute, too, which would
have the same restriction. So I didn't do it here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich