[PATCH v2 0/4] Add controllers to nvmet configfs

[PATCH v2 0/4] Add controllers to nvmet configfs

[Israel Rukshin]

This patch series expands nvmet configfs.
It adds to nvmet two main features
 - Close an active connection from target side.
 - Show all connections from the target point of view.

Those functionalities have some benefits:
 - Give the administrator more control and information
 - Enforce new ACL while there are active connections
 - Disconnect from unreachable Hosts

Changes from v1:
 - Split commit "Fix error flow in nvmet_alloc_ctrl()" to two
 - Embed some port info (traddr and trsvcid) in nvmet_ctrl struct

Israel Rukshin (4):
  nvmet: Fix error flow in nvmet_alloc_ctrl()
  nvmet: Rearrange nvmet_ctrl_free()
  nvmet: Add controllers to configfs
  nvmet: Add feature close connection from target side

 drivers/nvme/target/configfs.c | 86 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     | 19 ++++++++--
 drivers/nvme/target/nvmet.h    | 12 ++++++
 3 files changed, 113 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] nvmet: Fix error flow in nvmet_alloc_ctrl()

[Israel Rukshin]

Remove the allocated id on error.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 22a2a2b..5699c9a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -821,7 +821,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		/* Don't accept keep-alive timeout for discovery controllers */
 		if (kato) {
 			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-			goto out_free_sqs;
+			goto out_remove_ida;
 		}
 
 		/*
@@ -851,6 +851,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	*ctrlp = ctrl;
 	return 0;
 
+out_remove_ida:
+	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 out_free_sqs:
 	kfree(ctrl->sqs);
 out_free_cqs:
-- 
1.8.3.1

[PATCH 1/4] nvmet: Fix error flow in nvmet_alloc_ctrl()

[Christoph Hellwig]

Thanks,

applied to nvme-4.16.

[PATCH 2/4] nvmet: Rearrange nvmet_ctrl_free()

[Israel Rukshin]

Make it symmetric to nvmet_alloc_ctrl().

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 5699c9a..995976e 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -870,21 +870,22 @@ static void nvmet_ctrl_free(struct kref *ref)
 	struct nvmet_ctrl *ctrl = container_of(ref, struct nvmet_ctrl, ref);
 	struct nvmet_subsys *subsys = ctrl->subsys;
 
-	nvmet_stop_keep_alive_timer(ctrl);
-
 	mutex_lock(&subsys->lock);
 	list_del(&ctrl->subsys_entry);
 	mutex_unlock(&subsys->lock);
 
+	nvmet_stop_keep_alive_timer(ctrl);
+
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fatal_err_work);
 
 	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
-	nvmet_subsys_put(subsys);
 
 	kfree(ctrl->sqs);
 	kfree(ctrl->cqs);
 	kfree(ctrl);
+
+	nvmet_subsys_put(subsys);
 }
 
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl)
-- 
1.8.3.1

[PATCH 2/4] nvmet: Rearrange nvmet_ctrl_free()

[Christoph Hellwig]

Thanks,

applied to nvme-4.16.

[PATCH 3/4] nvmet: Add controllers to configfs

[Israel Rukshin]

The commit show all the controllers and some info about them.
This will allow the user to monitor the created controllers from target
point of view.
The "controllers" folder was added per subsystem under:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/

<CTRL_ID> folder consists of:
 - hostnqn: Host NQN
 - port_traddr: Port Transport Address
 - port_trsvcid: Port Transport Service ID

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/configfs.c | 72 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  8 +++++
 drivers/nvme/target/nvmet.h    | 12 +++++++
 3 files changed, 92 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b6aeb1d..2413df6 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -469,6 +469,49 @@ static struct config_group *nvmet_ns_make(struct config_group *group,
 	.ct_owner		= THIS_MODULE,
 };
 
+static ssize_t nvmet_ctrl_hostnqn_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->hostnqn);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, hostnqn);
+
+static ssize_t nvmet_ctrl_traddr_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->traddr);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, traddr);
+
+static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", ctrl->trsvcid);
+}
+
+CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
+
+static struct configfs_attribute *nvmet_ctrl_attrs[] = {
+	&nvmet_ctrl_attr_hostnqn,
+	&nvmet_ctrl_attr_traddr,
+	&nvmet_ctrl_attr_trsvcid,
+	NULL,
+};
+
+static struct config_item_type nvmet_ctrl_type = {
+	.ct_attrs		= nvmet_ctrl_attrs,
+	.ct_owner		= THIS_MODULE,
+};
+
+static struct config_item_type nvmet_controllers_type = {
+	.ct_owner		= THIS_MODULE,
+};
+
 static int nvmet_port_subsys_allow_link(struct config_item *parent,
 		struct config_item *target)
 {
@@ -760,6 +803,10 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 	configfs_add_default_group(&subsys->allowed_hosts_group,
 			&subsys->group);
 
+	config_group_init_type_name(&subsys->controllers_group,
+			"controllers", &nvmet_controllers_type);
+	configfs_add_default_group(&subsys->controllers_group, &subsys->group);
+
 	return &subsys->group;
 }
 
@@ -983,6 +1030,31 @@ static struct config_group *nvmet_hosts_make_group(struct config_group *group,
 	},
 };
 
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
+{
+	if (d_inode(ctrl->group.cg_item.ci_dentry))
+		configfs_unregister_group(&ctrl->group);
+}
+
+int nvmet_ctrl_configfs_create(struct nvmet_ctrl *ctrl)
+{
+	int res = 0;
+	char name[CONFIGFS_ITEM_NAME_LEN];
+
+	sprintf(name, "%d", ctrl->cntlid);
+	pr_debug("Adding controller %s to configfs\n", name);
+
+	config_group_init_type_name(&ctrl->group, name, &nvmet_ctrl_type);
+
+	res = configfs_register_group(&ctrl->subsys->controllers_group,
+				      &ctrl->group);
+	if (res)
+		pr_err("failed to register configfs group for controller %s\n",
+		       name);
+
+	return res;
+}
+
 int __init nvmet_init_configfs(void)
 {
 	int ret;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 995976e..631c0c5 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -842,6 +842,12 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		/* keep-alive timeout in seconds */
 		ctrl->kato = DIV_ROUND_UP(kato, 1000);
 	}
+	memcpy(ctrl->traddr, req->port->disc_addr.traddr, NVMF_TRADDR_SIZE);
+	memcpy(ctrl->trsvcid, req->port->disc_addr.trsvcid, NVMF_TRSVCID_SIZE);
+	ret = nvmet_ctrl_configfs_create(ctrl);
+	if (ret)
+		goto out_remove_ida;
+
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);
@@ -876,6 +882,8 @@ static void nvmet_ctrl_free(struct kref *ref)
 
 	nvmet_stop_keep_alive_timer(ctrl);
 
+	nvmet_ctrl_configfs_del(ctrl);
+
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fatal_err_work);
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 194ebff..c8fa32d 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -134,8 +134,17 @@ struct nvmet_ctrl {
 
 	char			subsysnqn[NVMF_NQN_FIELD_LEN];
 	char			hostnqn[NVMF_NQN_FIELD_LEN];
+	char			traddr[NVMF_TRADDR_SIZE];
+	char			trsvcid[NVMF_TRSVCID_SIZE];
+
+	struct config_group	group;
 };
 
+static inline struct nvmet_ctrl *to_nvmet_ctrl(struct config_item *item)
+{
+	return container_of(to_config_group(item), struct nvmet_ctrl, group);
+}
+
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 
@@ -160,6 +169,7 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+	struct config_group	controllers_group;
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
@@ -288,6 +298,8 @@ u16 nvmet_ctrl_find_get(const char *subsysnqn, const char *hostnqn, u16 cntlid,
 		struct nvmet_req *req, struct nvmet_ctrl **ret);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd);
+int nvmet_ctrl_configfs_create(struct nvmet_ctrl *ctrl);
+void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl);
 
 struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		enum nvme_subsys_type type);
-- 
1.8.3.1

[PATCH 3/4] nvmet: Add controllers to configfs

[Sagi Grimberg]

> +void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
> +{
> +	if (d_inode(ctrl->group.cg_item.ci_dentry))
> +		configfs_unregister_group(&ctrl->group);

I'm not sure I understand the conditional here, care to explain?

[PATCH 3/4] nvmet: Add controllers to configfs

[Israel Rukshin]

On 11/13/2017 10:18 PM, Sagi Grimberg wrote:
>
>> +void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
>> +{
>> +    if (d_inode(ctrl->group.cg_item.ci_dentry))
>> +        configfs_unregister_group(&ctrl->group);
>
> I'm not sure I understand the conditional here, care to explain?

Since I created the controller folder with configfs_register_group(), it 
creates this folder as a default folder.
Default folders are removed when their parent folder is removed.

The new filesystem hierarchy:
<SUBSYSTEM>/controllers/<CTRL_ID>/

Both controllers and controller (CTRL_ID) are default folders.
This condition protect us from a case that the user remove the subsystem 
folder with active connections.
When subsystem folder is removed by the user it first remove the 
controllers folder
and then it removes all the controller (CTRL_ID)  folders.
In this case there is no need to remove again the controller (CTRL_ID)  
folders when
the actual controller object is going to be freed.

This check simply asks if controller (CTRL_ID) folders was already removed.

[PATCH 3/4] nvmet: Add controllers to configfs

[Christoph Hellwig]

On Mon, Nov 13, 2017@12:29:42PM +0000, Israel Rukshin wrote:
> The commit show all the controllers and some info about them.
> This will allow the user to monitor the created controllers from target
> point of view.
> The "controllers" folder was added per subsystem under:
> /config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/
> 
> <CTRL_ID> folder consists of:
>  - hostnqn: Host NQN

This makes sense.

>  - port_traddr: Port Transport Address
>  - port_trsvcid: Port Transport Service ID

These are port properties.  You probably want to create a symlink
to the port from the controller directory instead.

> +void nvmet_ctrl_configfs_del(struct nvmet_ctrl *ctrl)
> +{
> +	if (d_inode(ctrl->group.cg_item.ci_dentry))
> +		configfs_unregister_group(&ctrl->group);
> +}

Are we even guaranteed that the denty is still around?  What happens
if you do a drop of all caches after the manual removal and then
?xecute this path later?
> +	int res = 0;
> +	char name[CONFIGFS_ITEM_NAME_LEN];
> +
> +	sprintf(name, "%d", ctrl->cntlid);
> +	pr_debug("Adding controller %s to configfs\n", name);
> +
> +	config_group_init_type_name(&ctrl->group, name, &nvmet_ctrl_type);
> +

Hmm - we probably should either add varargs to config_group_init_type_name
or just open code it.  config_item_set_name takes varargs, so having
this static buffer is a little silly.  I'll take a look at the configfs
side.

[PATCH 4/4] nvmet: Add feature close connection from target side

[Israel Rukshin]

This allows the user to close any connection from target side
by writing to the file "force_close" at the controller folder.
Full path:
/config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/configfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 2413df6..90e5925 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
 
 CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
 
+static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
+
+	ctrl->ops->delete_ctrl(ctrl);
+
+	return count;
+}
+
+CONFIGFS_ATTR_WO(nvmet_ctrl_, force_close);
+
 static struct configfs_attribute *nvmet_ctrl_attrs[] = {
 	&nvmet_ctrl_attr_hostnqn,
 	&nvmet_ctrl_attr_traddr,
 	&nvmet_ctrl_attr_trsvcid,
+	&nvmet_ctrl_attr_force_close,
 	NULL,
 };
 
-- 
1.8.3.1

[PATCH 4/4] nvmet: Add feature close connection from target side

[Sagi Grimberg]

Hi Israel,

> This allows the user to close any connection from target side
> by writing to the file "force_close" at the controller folder.
> Full path:
> /config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close
> 
> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>   drivers/nvme/target/configfs.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 2413df6..90e5925 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct config_item *item, char *page)
>   
>   CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
>   
> +static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
> +		const char *page, size_t count)

Maybe simply 'delete' is a better attribute name?

> +{
> +	struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
> +
> +	ctrl->ops->delete_ctrl(ctrl);
> +

What guarantees that calling delete_ctrl is safe here? Controllers can
be freed under various conditions (client disconnect, fatal_error,
device removal). What protects from user initiated force_close hitting
a use-after-free condition?

[PATCH 4/4] nvmet: Add feature close connection from target side

[Israel Rukshin]

Hi Sagi,

On 11/13/2017 10:21 PM, Sagi Grimberg wrote:
> Hi Israel,
>
>> This allows the user to close any connection from target side
>> by writing to the file "force_close" at the controller folder.
>> Full path:
>> /config/nvmet/subsystems/<SUBSYSTEM_NAME>/controllers/<CTRL_ID>/force_close 
>>
>>
>> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
>> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
>> ---
>>   drivers/nvme/target/configfs.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/nvme/target/configfs.c 
>> b/drivers/nvme/target/configfs.c
>> index 2413df6..90e5925 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -496,10 +496,23 @@ static ssize_t nvmet_ctrl_trsvcid_show(struct 
>> config_item *item, char *page)
>>     CONFIGFS_ATTR_RO(nvmet_ctrl_, trsvcid);
>>   +static ssize_t nvmet_ctrl_force_close_store(struct config_item *item,
>> +        const char *page, size_t count)
>
> Maybe simply 'delete' is a better attribute name?
You probably right.
>
>> +{
>> +    struct nvmet_ctrl *ctrl = to_nvmet_ctrl(item);
>> +
>> +    ctrl->ops->delete_ctrl(ctrl);
>> +
>
> What guarantees that calling delete_ctrl is safe here? Controllers can
> be freed under various conditions (client disconnect, fatal_error,
> device removal). What protects from user initiated force_close hitting
> a use-after-free condition?

The fact that I call nvmet_ctrl_configfs_del() at nvmet_ctrl_free() 
before freeing the controller guarantees
that the user will not see the controller at the filesystem after this 
point.
The second thing is that configfs_unregister_group() should wait for all 
the user calls on the controller to finish.
I don't see a difference between nvmet_ctrl_trsvcid_show() and 
nvmet_ctrl_force_close_store() (both are synchronous functions).

Regarding the races that you mentioned with client disconnect, 
fatal_error ...
__nvmet_rdma_queue_disconnect() is protected with a state lock.

[PATCH 4/4] nvmet: Add feature close connection from target side

[Sagi Grimberg]

>> What guarantees that calling delete_ctrl is safe here? Controllers can
>> be freed under various conditions (client disconnect, fatal_error,
>> device removal). What protects from user initiated force_close hitting
>> a use-after-free condition?
> 
> The fact that I call nvmet_ctrl_configfs_del() at nvmet_ctrl_free() 
> before freeing the controller guarantees
> that the user will not see the controller at the filesystem after this 
> point.
> The second thing is that configfs_unregister_group() should wait for all 
> the user calls on the controller to finish.
> I don't see a difference between nvmet_ctrl_trsvcid_show() and 
> nvmet_ctrl_force_close_store() (both are synchronous functions).
> 
> Regarding the races that you mentioned with client disconnect, 
> fatal_error ...
> __nvmet_rdma_queue_disconnect() is protected with a state lock.

The point is that ->delete_ctrl() is not designed to be safe for
calling it multiple times (although it might be the case for rdma) and
the code assumes that (which is wrong).

You need to call it conditionally. See what is done in
nvmet_ctrl_fatal_error()

[PATCH 4/4] nvmet: Add feature close connection from target side

[Christoph Hellwig]

> The point is that ->delete_ctrl() is not designed to be safe for
> calling it multiple times (although it might be the case for rdma) and
> the code assumes that (which is wrong).
>
> You need to call it conditionally. See what is done in
> nvmet_ctrl_fatal_error()

Yes.  In fact he should probably just call nvmet_ctrl_fatal_error.