[PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[Keisuke Nishimura]

The xa_store() may fail due to memory allocation failure because there
is no guarantee that the index csi is already used. This code allocates
the memory in advance to ensure the xa_store() succeeds before
nvme_get_log().

Fixes: 1cf7a12e09aa ("nvme: use an xarray to lookup the Commands Supported and Effects log")
Signed-off-by: Keisuke Nishimura <keisuke.nishimura@inria.fr>
---
 drivers/nvme/host/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a8d32a4a5c3..758de89b47b4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3098,6 +3098,12 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 	if (!cel)
 		return -ENOMEM;
 
+	ret = xa_reserve(&ctrl->cels, csi, GFP_KERNEL);
+	if (ret) {
+		kfree(cel);
+		return ret;
+	}
+
 	ret = nvme_get_log(ctrl, 0x00, NVME_LOG_CMD_EFFECTS, 0, csi,
 			cel, sizeof(*cel), 0);
 	if (ret) {
-- 
2.34.1

Re: [PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[Christoph Hellwig]

On Mon, Dec 02, 2024 at 01:42:40PM +0100, Keisuke Nishimura wrote:
> +	ret = xa_reserve(&ctrl->cels, csi, GFP_KERNEL);
> +	if (ret) {
> +		kfree(cel);
> +		return ret;
> +	}
> +
>  	ret = nvme_get_log(ctrl, 0x00, NVME_LOG_CMD_EFFECTS, 0, csi,
>  			cel, sizeof(*cel), 0);

There isn't really any point in doing a xa_reserve here vs simply
handling the xa_store error a bit below.

Re: [PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[Keisuke Nishimura]

Hello,

On 12/12/2024 07:27, Christoph Hellwig wrote:
> On Mon, Dec 02, 2024 at 01:42:40PM +0100, Keisuke Nishimura wrote:
>> +	ret = xa_reserve(&ctrl->cels, csi, GFP_KERNEL);
>> +	if (ret) {
>> +		kfree(cel);
>> +		return ret;
>> +	}
>> +
>>   	ret = nvme_get_log(ctrl, 0x00, NVME_LOG_CMD_EFFECTS, 0, csi,
>>   			cel, sizeof(*cel), 0);
> 
> There isn't really any point in doing a xa_reserve here vs simply
> handling the xa_store error a bit below.
> 

Thank you for your reply.

Without this xa_reserve(), if xa_store() fails, we cannot store cel after 
calling nvme_get_log() which can have side effects. My intention is to ensure 
xa_store() succeeds first and, if not, return early before invoking nvme_get_log().

But I am not very familiar with this code. If it is okay to check after the 
nvme_get_log(), I will send the patch soon.

best,
Keisuke

Re: [PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 02:06:50PM +0100, Keisuke Nishimura wrote:
> Without this xa_reserve(), if xa_store() fails, we cannot store cel after 
> calling nvme_get_log() which can have side effects. My intention is to 
> ensure xa_store() succeeds first and, if not, return early before invoking 
> nvme_get_log().

While reading various log pages can have side effects there shouldn't be
any for the Command Supported and Effects log page.

> But I am not very familiar with this code. If it is okay to check after the 
> nvme_get_log(), I will send the patch soon.

I think it should be fine, but I'm happy to wait for more opinions.

Re: [PATCH] nvme: Allocate memory for xa_store in advance in nvme_get_effects_log

[Keisuke Nishimura]

Hello,


On 16/12/2024 16:52, Christoph Hellwig wrote:
> On Mon, Dec 16, 2024 at 02:06:50PM +0100, Keisuke Nishimura wrote:
>> Without this xa_reserve(), if xa_store() fails, we cannot store cel after
>> calling nvme_get_log() which can have side effects. My intention is to
>> ensure xa_store() succeeds first and, if not, return early before invoking
>> nvme_get_log().
> 
> While reading various log pages can have side effects there shouldn't be
> any for the Command Supported and Effects log page.
> 
>> But I am not very familiar with this code. If it is okay to check after the
>> nvme_get_log(), I will send the patch soon.
> 
> I think it should be fine, but I'm happy to wait for more opinions.
> 

Okay. I do not have any other opinions on this:) I am happy to follow your 
suggestion and leave any further decisions to the maintainers of this subsystem 
if needed. I will send the patch to directly check xa_store() soon.

Thank you.
Keisuke

[PATCH] nvme: Add error check for xa_store in nvme_get_effects_log

[Keisuke Nishimura]

The xa_store() may fail due to memory allocation failure because there
is no guarantee that the index csi is already used. This fix adds an
error check of the return value of xa_store() in nvme_get_effects_log().

Fixes: 1cf7a12e09aa ("nvme: use an xarray to lookup the Commands Supported and Effects log")
Signed-off-by: Keisuke Nishimura <keisuke.nishimura@inria.fr>
---
 drivers/nvme/host/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a8d32a4a5c3..d4028ce5ad8d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3088,7 +3088,7 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, u8 csi,
 static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 				struct nvme_effects_log **log)
 {
-	struct nvme_effects_log	*cel = xa_load(&ctrl->cels, csi);
+	struct nvme_effects_log *old, *cel = xa_load(&ctrl->cels, csi);
 	int ret;
 
 	if (cel)
@@ -3105,7 +3105,11 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 		return ret;
 	}
 
-	xa_store(&ctrl->cels, csi, cel, GFP_KERNEL);
+	old = xa_store(&ctrl->cels, csi, cel, GFP_KERNEL);
+	if (xa_is_err(old)) {
+		kfree(cel);
+		return xa_err(old);
+	}
 out:
 	*log = cel;
 	return 0;
-- 
2.34.1

Re: [PATCH] nvme: Add error check for xa_store in nvme_get_effects_log

[Sagi Grimberg]

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

Re: [PATCH] nvme: Add error check for xa_store in nvme_get_effects_log

[Christoph Hellwig]

Looks good:

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

Re: [PATCH] nvme: Add error check for xa_store in nvme_get_effects_log

[Keith Busch]

On Fri, Dec 20, 2024 at 01:00:47PM +0100, Keisuke Nishimura wrote:
> The xa_store() may fail due to memory allocation failure because there
> is no guarantee that the index csi is already used. This fix adds an
> error check of the return value of xa_store() in nvme_get_effects_log().

Thanks, applied to nvme-6.14.