[PATCH] nvme: ensure reset state check ordering

[PATCH] nvme: ensure reset state check ordering

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

A different CPU may be setting the ctrl->state value while flushing the
reset work, so ensure proper barriers to prevent reading a stale state.
Reading the wrong state can report unexpected ENETRESET errors when
everything was successful.

Fixes: 8000d1fdb07e36 ("nvme-rdma: fix sysfs invoked reset_ctrl error flow")
Reported-by: Minh Hoang <mh2022@meta.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 62612f87aafa2..9ec7ce8cc802e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -201,7 +201,7 @@ int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl)
 	ret = nvme_reset_ctrl(ctrl);
 	if (!ret) {
 		flush_work(&ctrl->reset_work);
-		if (ctrl->state != NVME_CTRL_LIVE)
+		if (smp_load_acquire(&ctrl->state) != NVME_CTRL_LIVE)
 			ret = -ENETRESET;
 	}
 
-- 
2.34.1

Re: [PATCH] nvme: ensure reset state check ordering

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 11:07:52AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> A different CPU may be setting the ctrl->state value while flushing the
> reset work, so ensure proper barriers to prevent reading a stale state.
> Reading the wrong state can report unexpected ENETRESET errors when
> everything was successful.

smp_load_acquire needs to be paired with a smp_store_release.

We should either always hold the lock or use WRITE_ONCE/READ_ONCE

Re: [PATCH] nvme: ensure reset state check ordering

[Sagi Grimberg]

>> From: Keith Busch <kbusch@kernel.org>
>>
>> A different CPU may be setting the ctrl->state value while flushing the
>> reset work, so ensure proper barriers to prevent reading a stale state.
>> Reading the wrong state can report unexpected ENETRESET errors when
>> everything was successful.
> 
> smp_load_acquire needs to be paired with a smp_store_release.
> 
> We should either always hold the lock or use WRITE_ONCE/READ_ONCE

which lock?

Re: [PATCH] nvme: ensure reset state check ordering

[Christoph Hellwig]

On Mon, Nov 20, 2023 at 03:53:59PM +0200, Sagi Grimberg wrote:
>
>>> From: Keith Busch <kbusch@kernel.org>
>>>
>>> A different CPU may be setting the ctrl->state value while flushing the
>>> reset work, so ensure proper barriers to prevent reading a stale state.
>>> Reading the wrong state can report unexpected ENETRESET errors when
>>> everything was successful.
>>
>> smp_load_acquire needs to be paired with a smp_store_release.
>>
>> We should either always hold the lock or use WRITE_ONCE/READ_ONCE
>
> which lock?

in this case: ctrl->lock.

Re: [PATCH] nvme: ensure reset state check ordering

[Sagi Grimberg]

>>>> From: Keith Busch <kbusch@kernel.org>
>>>>
>>>> A different CPU may be setting the ctrl->state value while flushing the
>>>> reset work, so ensure proper barriers to prevent reading a stale state.
>>>> Reading the wrong state can report unexpected ENETRESET errors when
>>>> everything was successful.
>>>
>>> smp_load_acquire needs to be paired with a smp_store_release.
>>>
>>> We should either always hold the lock or use WRITE_ONCE/READ_ONCE
>>
>> which lock?
> 
> in this case: ctrl->lock.

Yes, we can do either. READ/WRITE_ONCE seems more appropriate. Worth
a comment because we don't use it in other places where it is advisory.