[PATCH] nvme: add sysfs attribute 'tls_keyring'

[Sagi Grimberg]

On 08/03/2024 16:06, Hannes Reinecke wrote:
> 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?

WARN_ON is better I guess as this is really unexpected.

>
>>> +    }
>>> +    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.

Still I don't see a sane use-case where this would be used.

>
>>> +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.

OK, not a must. Can be added later...