[PATCH] nvme: add sysfs attribute 'tls_keyring'

[Sagi Grimberg]

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

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

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