[PATCHv4] nvmet: implement unique discovery NQN

[PATCHv4] nvmet: implement unique discovery NQN

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

Unique discovery NQNs allow to differentiate between discovery
services from (typically physically separate) NVMe-oF subsystems.
This is required for establishing secured connections as otherwise
the credentials won't be unique and the integrity of the connection
cannot be guaranteed.
This patch adda a configfs attribute 'discovery_nqn' in the 'nvmet'
configfs directory to specify the unique discovery NQN.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c | 46 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  7 ++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 77a6e817b315..e8ead4ab35aa 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1613,6 +1613,11 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (sysfs_streq(name, nvmet_disc_subsys->subsysnqn)) {
+		pr_err("can't create subsystem using unique discovery NQN\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
 	if (IS_ERR(subsys))
 		return ERR_CAST(subsys);
@@ -2159,7 +2164,48 @@ static const struct config_item_type nvmet_hosts_type = {
 
 static struct config_group nvmet_hosts_group;
 
+static ssize_t nvmet_root_discovery_nqn_show(struct config_item *item,
+					     char *page)
+{
+	return sprintf(page, "%s\n", nvmet_disc_subsys->subsysnqn);
+}
+
+static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct list_head *entry;
+	size_t len;
+
+	len = strcspn(page, "\n");
+	if (!len || len > NVMF_NQN_FIELD_LEN - 1)
+		return -EINVAL;
+
+	down_write(&nvmet_config_sem);
+	list_for_each(entry, &nvmet_subsystems_group.cg_children) {
+		struct config_item *item =
+			container_of(entry, struct config_item, ci_entry);
+		if (!strncmp(config_item_name(item), page, len)) {
+			pr_err("duplicate NQN %s\n", config_item_name(item));
+			up_write(&nvmet_config_sem);
+			return -EINVAL;
+		}
+	}
+	memset(nvmet_disc_subsys->subsysnqn, 0, NVMF_NQN_FIELD_LEN);
+	memcpy(nvmet_disc_subsys->subsysnqn, page, len);
+	up_write(&nvmet_config_sem);
+
+	return len;
+}
+
+CONFIGFS_ATTR(nvmet_root_, discovery_nqn);
+
+static struct configfs_attribute *nvmet_root_attrs[] = {
+	&nvmet_root_attr_discovery_nqn,
+	NULL,
+};
+
 static const struct config_item_type nvmet_root_type = {
+	.ct_attrs		= nvmet_root_attrs,
 	.ct_owner		= THIS_MODULE,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 694e91cdfc8d..2ae6d70f47a2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1552,6 +1552,13 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	}
 
 	down_read(&nvmet_config_sem);
+	if (!strncmp(nvmet_disc_subsys->subsysnqn, subsysnqn,
+				NVMF_NQN_SIZE)) {
+		if (kref_get_unless_zero(&nvmet_disc_subsys->ref)) {
+			up_read(&nvmet_config_sem);
+			return nvmet_disc_subsys;
+		}
+	}
 	list_for_each_entry(p, &port->subsystems, entry) {
 		if (!strncmp(p->subsys->subsysnqn, subsysnqn,
 				NVMF_NQN_SIZE)) {
-- 
2.35.3

Re: [PATCHv4] nvmet: implement unique discovery NQN

[Chaitanya Kulkarni]

On 3/22/2024 12:09 AM, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Unique discovery NQNs allow to differentiate between discovery
> services from (typically physically separate) NVMe-oF subsystems.
> This is required for establishing secured connections as otherwise
> the credentials won't be unique and the integrity of the connection
> cannot be guaranteed.
> This patch adda a configfs attribute 'discovery_nqn' in the 'nvmet'
> configfs directory to specify the unique discovery NQN.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---

umm change log ?

>   drivers/nvme/target/configfs.c | 46 ++++++++++++++++++++++++++++++++++
>   drivers/nvme/target/core.c     |  7 ++++++
>   2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 77a6e817b315..e8ead4ab35aa 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1613,6 +1613,11 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
>   		return ERR_PTR(-EINVAL);
>   	}
>   
> +	if (sysfs_streq(name, nvmet_disc_subsys->subsysnqn)) {
> +		pr_err("can't create subsystem using unique discovery NQN\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
>   	if (IS_ERR(subsys))
>   		return ERR_CAST(subsys);
> @@ -2159,7 +2164,48 @@ static const struct config_item_type nvmet_hosts_type = {
>   
>   static struct config_group nvmet_hosts_group;
>   
> +static ssize_t nvmet_root_discovery_nqn_show(struct config_item *item,
> +					     char *page)
> +{
> +	return sprintf(page, "%s\n", nvmet_disc_subsys->subsysnqn);
> +}
> +

snprintf() with PAGE_SIZE ?

> +static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
> +		const char *page, size_t count)
> +{
> +	struct list_head *entry;
> +	size_t len;
> +
> +	len = strcspn(page, "\n");
> +	if (!len || len > NVMF_NQN_FIELD_LEN - 1)
> +		return -EINVAL;
> +
> +	down_write(&nvmet_config_sem);
> +	list_for_each(entry, &nvmet_subsystems_group.cg_children) {
> +		struct config_item *item =
> +			container_of(entry, struct config_item, ci_entry);

newline here ?

> +		if (!strncmp(config_item_name(item), page, len)) {
> +			pr_err("duplicate NQN %s\n", config_item_name(item));
> +			up_write(&nvmet_config_sem);
> +			return -EINVAL;
> +		}
> +	}
> +	memset(nvmet_disc_subsys->subsysnqn, 0, NVMF_NQN_FIELD_LEN);
> +	memcpy(nvmet_disc_subsys->subsysnqn, page, len);
> +	up_write(&nvmet_config_sem);
> +
> +	return len;
> +}


-ck

Re: [PATCHv4] nvmet: implement unique discovery NQN

[Hannes Reinecke]

On 3/22/24 10:43, Chaitanya Kulkarni wrote:
> On 3/22/2024 12:09 AM, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Unique discovery NQNs allow to differentiate between discovery
>> services from (typically physically separate) NVMe-oF subsystems.
>> This is required for establishing secured connections as otherwise
>> the credentials won't be unique and the integrity of the connection
>> cannot be guaranteed.
>> This patch adda a configfs attribute 'discovery_nqn' in the 'nvmet'
>> configfs directory to specify the unique discovery NQN.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
> 
> umm change log ?
> 
Just a rebase to latest head and modified the commit message slightly.
Not sure if that warranted a change log.

>>    drivers/nvme/target/configfs.c | 46 ++++++++++++++++++++++++++++++++++
>>    drivers/nvme/target/core.c     |  7 ++++++
>>    2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>> index 77a6e817b315..e8ead4ab35aa 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -1613,6 +1613,11 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
>>    		return ERR_PTR(-EINVAL);
>>    	}
>>    
>> +	if (sysfs_streq(name, nvmet_disc_subsys->subsysnqn)) {
>> +		pr_err("can't create subsystem using unique discovery NQN\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>>    	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
>>    	if (IS_ERR(subsys))
>>    		return ERR_CAST(subsys);
>> @@ -2159,7 +2164,48 @@ static const struct config_item_type nvmet_hosts_type = {
>>    
>>    static struct config_group nvmet_hosts_group;
>>    
>> +static ssize_t nvmet_root_discovery_nqn_show(struct config_item *item,
>> +					     char *page)
>> +{
>> +	return sprintf(page, "%s\n", nvmet_disc_subsys->subsysnqn);
>> +}
>> +
> 
> snprintf() with PAGE_SIZE ?
> 
Ok.

>> +static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
>> +		const char *page, size_t count)
>> +{
>> +	struct list_head *entry;
>> +	size_t len;
>> +
>> +	len = strcspn(page, "\n");
>> +	if (!len || len > NVMF_NQN_FIELD_LEN - 1)
>> +		return -EINVAL;
>> +
>> +	down_write(&nvmet_config_sem);
>> +	list_for_each(entry, &nvmet_subsystems_group.cg_children) {
>> +		struct config_item *item =
>> +			container_of(entry, struct config_item, ci_entry);
> 
> newline here ?
> 
Blasted configfs always eats into the line length limitation.
I see what I can do.

Thanks for the review.

Cheers,

Hannes

Re: [PATCHv4] nvmet: implement unique discovery NQN

[Sagi Grimberg]

I think I gave you a review back in the last iteration, but if not
this looks good to me, outside of the review by Chaitanya

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>