[PATCH 0/4] nvme: Fixes for deleting a ctrl before it was created

[PATCH 0/4] nvme: Fixes for deleting a ctrl before it was created

[Israel Rukshin]

The patchset starts with warning fixes on this scenario, continue with
small cleanup and ends with the actual fix.
Calling nvme_sysfs_delete() when the controller is in the middle of
creation may cause several bugs. If the controller is in NEW state we
remove delete_controller file and don't delete the controller. The user
will not be able to use nvme disconnect command on that controller again,
although the controller may be active. Other bugs may happen if the
controller is in the middle of create_ctrl callback and
nvme_do_delete_ctrl() starts. For example, freeing I/O tagset at
nvme_do_delete_ctrl() before it was created or controller use after free
at create_ctrl callback.

Israel Rukshin (4):
  nvme-rdma: Fix warning at nvme_rdma_setup_ctrl
  nvme-tcp: Fix warning at nvme_tcp_setup_ctrl
  nvme: Remove unused return code from nvme_delete_ctrl_sync
  nvme: Fix controller use after free at create_ctrl callback

 drivers/nvme/host/core.c    | 13 ++++++-------
 drivers/nvme/host/fabrics.c |  1 +
 drivers/nvme/host/nvme.h    |  1 +
 drivers/nvme/host/rdma.c    |  8 ++++++--
 drivers/nvme/host/tcp.c     |  8 ++++++--
 5 files changed, 20 insertions(+), 11 deletions(-)

-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Israel Rukshin]

The transition to LIVE state should not fail in case of a new controller.
Moving to DELETING state before nvme_rdma_create_ctrl() takes a refcount
on the controller may leads to controller use after free.

Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/host/rdma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d6022fa..57f9031 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
 	if (!changed) {
-		/* state change failure is ok if we're in DELETING state */
-		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
+		/*
+		 * state change failure is ok if we're in DELETING state,
+		 * unless we're during creation of a new controller to
+		 * avoid use after free (ctrl refcount is not taken yet).
+		 */
+		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
 		ret = -EINVAL;
 		goto destroy_io;
 	}
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Sagi Grimberg]

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index d6022fa..57f9031 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>   
>   	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>   	if (!changed) {
> -		/* state change failure is ok if we're in DELETING state */
> -		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
> +		/*
> +		 * state change failure is ok if we're in DELETING state,
> +		 * unless we're during creation of a new controller to
> +		 * avoid use after free (ctrl refcount is not taken yet).
> +		 */
> +		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);

What state are we in if not DELETING in this case?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Israel Rukshin]

On 3/12/2020 8:37 AM, Sagi Grimberg wrote:
>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index d6022fa..57f9031 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>> nvme_rdma_ctrl *ctrl, bool new)
>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>       if (!changed) {
>> -        /* state change failure is ok if we're in DELETING state */
>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>> +        /*
>> +         * state change failure is ok if we're in DELETING state,
>> +         * unless we're during creation of a new controller to
>> +         * avoid use after free (ctrl refcount is not taken yet).
>> +         */
>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
>
> What state are we in if not DELETING in this case?

We are in DELETING state.

With this change any state failure triggers a warning (including 
DELETING) if new is true.

In my test I was in DELETING state and with new == true

Trying to change state from DELETING to LIVE is not allowed at the state 
machine.


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index d6022fa..57f9031 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>>> nvme_rdma_ctrl *ctrl, bool new)
>>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>>       if (!changed) {
>>> -        /* state change failure is ok if we're in DELETING state */
>>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>>> +        /*
>>> +         * state change failure is ok if we're in DELETING state,
>>> +         * unless we're during creation of a new controller to
>>> +         * avoid use after free (ctrl refcount is not taken yet).
>>> +         */
>>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
>>
>> What state are we in if not DELETING in this case?
> 
> We are in DELETING state.
> 
> With this change any state failure triggers a warning (including 
> DELETING) if new is true.
> 
> In my test I was in DELETING state and with new == true
> 
> Trying to change state from DELETING to LIVE is not allowed at the state 
> machine.

Why do we need a warning on that?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Israel Rukshin]

On 3/13/2020 1:08 AM, Sagi Grimberg wrote:
>
>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>> index d6022fa..57f9031 100644
>>>> --- a/drivers/nvme/host/rdma.c
>>>> +++ b/drivers/nvme/host/rdma.c
>>>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>>>       if (!changed) {
>>>> -        /* state change failure is ok if we're in DELETING state */
>>>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>>>> +        /*
>>>> +         * state change failure is ok if we're in DELETING state,
>>>> +         * unless we're during creation of a new controller to
>>>> +         * avoid use after free (ctrl refcount is not taken yet).
>>>> +         */
>>>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
>>>
>>> What state are we in if not DELETING in this case?
>>
>> We are in DELETING state.
>>
>> With this change any state failure triggers a warning (including 
>> DELETING) if new is true.
>>
>> In my test I was in DELETING state and with new == true
>>
>> Trying to change state from DELETING to LIVE is not allowed at the 
>> state machine.
>
> Why do we need a warning on that?

First, the comment "state change failure is ok if we're in DELETING 
state" is wrong.

State change failure should not happen when creating a new controller, 
because it may lead to controller use after free.

Second, the warning already exists so I am only fixing it.


Regards,

Israel


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Sagi Grimberg]

>>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>>> index d6022fa..57f9031 100644
>>>>> --- a/drivers/nvme/host/rdma.c
>>>>> +++ b/drivers/nvme/host/rdma.c
>>>>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>>>>       if (!changed) {
>>>>> -        /* state change failure is ok if we're in DELETING state */
>>>>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>>>>> +        /*
>>>>> +         * state change failure is ok if we're in DELETING state,
>>>>> +         * unless we're during creation of a new controller to
>>>>> +         * avoid use after free (ctrl refcount is not taken yet).
>>>>> +         */
>>>>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);
>>>>
>>>> What state are we in if not DELETING in this case?
>>>
>>> We are in DELETING state.
>>>
>>> With this change any state failure triggers a warning (including 
>>> DELETING) if new is true.
>>>
>>> In my test I was in DELETING state and with new == true
>>>
>>> Trying to change state from DELETING to LIVE is not allowed at the 
>>> state machine.
>>
>> Why do we need a warning on that?
> 
> First, the comment "state change failure is ok if we're in DELETING 
> state" is wrong.

The WARN_ON_ONCE is designed to indicate that we failed to transition
the state but we don't understand why. Is that the case here?

> State change failure should not happen when creating a new controller, 
> because it may lead to controller use after free.

So we need to fix that, but the warning should be printed only when
we get an unexpected behavior from the flow perspective.

> Second, the warning already exists so I am only fixing it.

I don't understand what this is fixing.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Max Gurtovoy]

On 3/16/2020 6:43 PM, Sagi Grimberg wrote:
>
>>>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>>>> index d6022fa..57f9031 100644
>>>>>> --- a/drivers/nvme/host/rdma.c
>>>>>> +++ b/drivers/nvme/host/rdma.c
>>>>>> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>>         changed = nvme_change_ctrl_state(&ctrl->ctrl, 
>>>>>> NVME_CTRL_LIVE);
>>>>>>       if (!changed) {
>>>>>> -        /* state change failure is ok if we're in DELETING state */
>>>>>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
>>>>>> +        /*
>>>>>> +         * state change failure is ok if we're in DELETING state,
>>>>>> +         * unless we're during creation of a new controller to
>>>>>> +         * avoid use after free (ctrl refcount is not taken yet).
>>>>>> +         */
>>>>>> +        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || 
>>>>>> new);
>>>>>
>>>>> What state are we in if not DELETING in this case?
>>>>
>>>> We are in DELETING state.
>>>>
>>>> With this change any state failure triggers a warning (including 
>>>> DELETING) if new is true.
>>>>
>>>> In my test I was in DELETING state and with new == true
>>>>
>>>> Trying to change state from DELETING to LIVE is not allowed at the 
>>>> state machine.
>>>
>>> Why do we need a warning on that?
>>
>> First, the comment "state change failure is ok if we're in DELETING 
>> state" is wrong.
>
> The WARN_ON_ONCE is designed to indicate that we failed to transition
> the state but we don't understand why. Is that the case here?
>
>> State change failure should not happen when creating a new 
>> controller, because it may lead to controller use after free.
>
> So we need to fix that, but the warning should be printed only when
> we get an unexpected behavior from the flow perspective.
>
>> Second, the warning already exists so I am only fixing it.
>
> I don't understand what this is fixing.

maybe we should re-order the patches.

Israel fixed the use after free bug in this series.

In case of "new" we can't fail moving to NVME_CTRL_LIVE now at all 
(patch 4/4).

We don't allow disconnect during initial connection establishment.



_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-rdma: Fix warning at nvme_rdma_setup_ctrl

[Christoph Hellwig]

On Wed, Mar 11, 2020 at 05:00:46PM +0200, Israel Rukshin wrote:
> The transition to LIVE state should not fail in case of a new controller.
> Moving to DELETING state before nvme_rdma_create_ctrl() takes a refcount
> on the controller may leads to controller use after free.
> 
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>  drivers/nvme/host/rdma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index d6022fa..57f9031 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1084,8 +1084,12 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>  
>  	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>  	if (!changed) {
> -		/* state change failure is ok if we're in DELETING state */
> -		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING);
> +		/*
> +		 * state change failure is ok if we're in DELETING state,
> +		 * unless we're during creation of a new controller to
> +		 * avoid use after free (ctrl refcount is not taken yet).
> +		 */
> +		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING || new);

Please split this into two WARN_ON_ONCE so that the trace shows which
condition triggered.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/4] nvme-tcp: Fix warning at nvme_tcp_setup_ctrl

[Israel Rukshin]

The transition to LIVE state should not fail in case of a new controller.
Moving to DELETING state before nvme_tcp_create_ctrl() takes a refcount
on the controller may leads to controller use after free.

Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/host/tcp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2af7e8c..145b96c 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1877,8 +1877,12 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 	}
 
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE)) {
-		/* state change failure is ok if we're in DELETING state */
-		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING);
+		/*
+		 * state change failure is ok if we're in DELETING state,
+		 * unless we're during creation of a new controller to
+		 * avoid use after free (ctrl refcount is not taken yet).
+		 */
+		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING || new);
 		ret = -EINVAL;
 		goto destroy_io;
 	}
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/4] nvme: Remove unused return code from nvme_delete_ctrl_sync

[Israel Rukshin]

The return code of nvme_delete_ctrl_sync is never used, so change it to
void.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 17cd39b..c0d9b19 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -192,21 +192,16 @@ int nvme_delete_ctrl(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_delete_ctrl);
 
-static int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
+static void nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 {
-	int ret = 0;
-
 	/*
 	 * Keep a reference until nvme_do_delete_ctrl() complete,
 	 * since ->delete_ctrl can free the controller.
 	 */
 	nvme_get_ctrl(ctrl);
-	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
-		ret = -EBUSY;
-	if (!ret)
+	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING))
 		nvme_do_delete_ctrl(ctrl);
 	nvme_put_ctrl(ctrl);
-	return ret;
 }
 
 static blk_status_t nvme_error_status(u16 status)
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/4] nvme: Remove unused return code from nvme_delete_ctrl_sync

[Sagi Grimberg]

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

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/4] nvme: Remove unused return code from nvme_delete_ctrl_sync

[Christoph Hellwig]

On Wed, Mar 11, 2020 at 05:00:48PM +0200, Israel Rukshin wrote:
> The return code of nvme_delete_ctrl_sync is never used, so change it to
> void.
> 
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Israel Rukshin]

Calling nvme_sysfs_delete() when the controller is in the middle of
creation may cause several bugs. If the controller is in NEW state we
remove delete_controller file and don't delete the controller. The user
will not be able to use nvme disconnect command on that controller again,
although the controller may be active. Other bugs may happen if the
controller is in the middle of create_ctrl callback and
nvme_do_delete_ctrl() starts. For example, freeing I/O tagset at
nvme_do_delete_ctrl() before it was created or controller use after free
at create_ctrl callback.

To fix all this races don't allow the user to delete the controller
before it was fully created.

Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/host/core.c    | 4 ++++
 drivers/nvme/host/fabrics.c | 1 +
 drivers/nvme/host/nvme.h    | 1 +
 3 files changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c0d9b19..7976955 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3224,6 +3224,10 @@ static ssize_t nvme_sysfs_delete(struct device *dev,
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 
+	/* Can't delete non-created controllers */
+	if (!ctrl->created)
+		return -EBUSY;
+
 	if (device_remove_file_self(dev, attr))
 		nvme_delete_ctrl_sync(ctrl);
 	return count;
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index c09230e..d6e3d67 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -1055,6 +1055,7 @@ void nvmf_free_options(struct nvmf_ctrl_options *opts)
 		ret = PTR_ERR(ctrl);
 		goto out_module_put;
 	}
+	ctrl->created = true;
 
 	module_put(ops->module);
 	return ctrl;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d0bfa2b..e7623d2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -294,6 +294,7 @@ struct nvme_ctrl {
 	u16 maxcmd;
 	int nr_reconnects;
 	struct nvmf_ctrl_options *opts;
+	bool created;
 
 	struct page *discard_page;
 	unsigned long discard_page_busy;
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Sagi Grimberg]

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c0d9b19..7976955 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3224,6 +3224,10 @@ static ssize_t nvme_sysfs_delete(struct device *dev,
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>   
> +	/* Can't delete non-created controllers */
> +	if (!ctrl->created)
> +		return -EBUSY;
> +

Not ideal that core checks attribute that fabrics is setting. Maybe
move this to nvme_start_ctrl?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Israel Rukshin]

On 3/12/2020 8:45 AM, Sagi Grimberg wrote:
>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index c0d9b19..7976955 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3224,6 +3224,10 @@ static ssize_t nvme_sysfs_delete(struct device 
>> *dev,
>>   {
>>       struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>   +    /* Can't delete non-created controllers */
>> +    if (!ctrl->created)
>> +        return -EBUSY;
>> +
>
> Not ideal that core checks attribute that fabrics is setting. Maybe
> move this to nvme_start_ctrl?

I can't move this to nvme_start_ctrl(), because rdma/tcp have not taken 
ctrl refcount yet.

Beside that, nvme_sysfs_delete function is only used by fabrics modules.

PCI module doesn't set delete_ctrl ops.


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index c0d9b19..7976955 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -3224,6 +3224,10 @@ static ssize_t nvme_sysfs_delete(struct device 
>>> *dev,
>>>   {
>>>       struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>>   +    /* Can't delete non-created controllers */
>>> +    if (!ctrl->created)
>>> +        return -EBUSY;
>>> +
>>
>> Not ideal that core checks attribute that fabrics is setting. Maybe
>> move this to nvme_start_ctrl?
> 
> I can't move this to nvme_start_ctrl(), because rdma/tcp have not taken 
> ctrl refcount yet.

Can you explain what is the issue with the ref counting for setting this
flag?

> Beside that, nvme_sysfs_delete function is only used by fabrics modules.
> 
> PCI module doesn't set delete_ctrl ops.

That can change though.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Israel Rukshin]

On 3/16/2020 6:47 PM, Sagi Grimberg wrote:
>
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index c0d9b19..7976955 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -3224,6 +3224,10 @@ static ssize_t nvme_sysfs_delete(struct 
>>>> device *dev,
>>>>   {
>>>>       struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>>>   +    /* Can't delete non-created controllers */
>>>> +    if (!ctrl->created)
>>>> +        return -EBUSY;
>>>> +
>>>
>>> Not ideal that core checks attribute that fabrics is setting. Maybe
>>> move this to nvme_start_ctrl?
>>
>> I can't move this to nvme_start_ctrl(), because rdma/tcp have not 
>> taken ctrl refcount yet.
>
> Can you explain what is the issue with the ref counting for setting this
> flag?
>
Yes, for example you can see that nvme_rdma_create_ctrl() calls 
nvme_rdma_setup_ctrl() which calls to nvme_start_ctrl().

After calling nvme_rdma_setup_ctrl() we take the ref count on the ctrl 
by calling nvme_get_ctrl().

In case nvme_sysfs_delete() is called by the user before calling 
nvme_get_ctrl() the controller ref count

reach to zero and nvme_free_ctrl() is called.

After that I got:

[Tue Mar 17 12:23:19 2020] nvme 
\xffffffa0zw\xffffffed2\xffffff8f\xffffffff\xffffffff.rodata: new ctrl: 
NQN "testsubsystem2", addr 11.209.40.61:4420
[Tue Mar 17 12:23:19 2020] ------------[ cut here ]------------
[Tue Mar 17 12:23:19 2020] refcount_t: addition on 0; use-after-free.
[Tue Mar 17 12:23:19 2020] Call Trace:
[Tue Mar 17 12:23:19 2020]  kobject_get+0x5f/0x70
[Tue Mar 17 12:23:19 2020]  nvme_rdma_create_ctrl+0x38c/0x3e4 [nvme_rdma]
[Tue Mar 17 12:23:19 2020]  nvmf_dev_write+0xa79/0xd00 [nvme_fabrics]
[Tue Mar 17 12:23:19 2020]  vfs_write+0xad/0x1b0
[Tue Mar 17 12:23:19 2020]  ksys_write+0x55/0xd0
[Tue Mar 17 12:23:19 2020]  do_syscall_64+0x55/0x1b0
[Tue Mar 17 12:23:19 2020] entry_SYSCALL_64_after_hwframe+0x44/0xa9


We can fix this by taking the ref count on earlier stage.

For example we can take a ref count at nvme_start_ctrl(), but it affects 
also pci module (I need to check that),

or we can take it before calling nvme_start_ctrl() at rdma/tcp. The ref 
count should be taken only if  "new" is true.

>> Beside that, nvme_sysfs_delete function is only used by fabrics modules.
>>
>> PCI module doesn't set delete_ctrl ops.
>
> That can change though.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Christoph Hellwig]

On Tue, Mar 17, 2020 at 01:49:43PM +0200, Israel Rukshin wrote:
>>
> Yes, for example you can see that nvme_rdma_create_ctrl() calls 
> nvme_rdma_setup_ctrl() which calls to nvme_start_ctrl().
>
> After calling nvme_rdma_setup_ctrl() we take the ref count on the ctrl by 
> calling nvme_get_ctrl().
>
> In case nvme_sysfs_delete() is called by the user before calling 
> nvme_get_ctrl() the controller ref count
>
> reach to zero and nvme_free_ctrl() is called.

> We can fix this by taking the ref count on earlier stage.

Why don't we do that?

> For example we can take a ref count at nvme_start_ctrl(), but it affects 
> also pci module (I need to check that),
>
> or we can take it before calling nvme_start_ctrl() at rdma/tcp. The ref 
> count should be taken only if  "new" is true.

I think we need the reference as soon as the controller is externally
visible in any way, which AFAICS is done by cdev_device_add in
nvme_init_ctrl.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme: Fix controller use after free at create_ctrl callback

[Israel Rukshin]

On 3/17/2020 2:56 PM, Christoph Hellwig wrote:
> On Tue, Mar 17, 2020 at 01:49:43PM +0200, Israel Rukshin wrote:
>> Yes, for example you can see that nvme_rdma_create_ctrl() calls
>> nvme_rdma_setup_ctrl() which calls to nvme_start_ctrl().
>>
>> After calling nvme_rdma_setup_ctrl() we take the ref count on the ctrl by
>> calling nvme_get_ctrl().
>>
>> In case nvme_sysfs_delete() is called by the user before calling
>> nvme_get_ctrl() the controller ref count
>>
>> reach to zero and nvme_free_ctrl() is called.
>> We can fix this by taking the ref count on earlier stage.
> Why don't we do that?

I will do it at V2 :)

>
>> For example we can take a ref count at nvme_start_ctrl(), but it affects
>> also pci module (I need to check that),
>>
>> or we can take it before calling nvme_start_ctrl() at rdma/tcp. The ref
>> count should be taken only if  "new" is true.
> I think we need the reference as soon as the controller is externally
> visible in any way, which AFAICS is done by cdev_device_add in
> nvme_init_ctrl.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme