[PATCH] nvmet: fix mar and mor off-by-one errors

[PATCH] nvmet: fix mar and mor off-by-one errors

[Dennis Maisenbacher]

From: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>

Maximum Active Resources (MAR) and Maximum Open Resources (MOR) are 0's
based vales where a value of 0xffffffff indicates that there is no limit.

Cast the unsigned int values that are returned by bdev_max_open_zones and
bdev_max_active_zones into u32 vales which need to be decremented as the
returned values of the block layer helpers are not 0's based.
The cast to u32 is necessary because the size of unsigned int is
architecture dependent and a 0 reported by the block layer helpers
indicates no limit, thus it needs to be converted to 0xffffffff which
happens by underflowing the u32.

Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
Signed-off-by: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
---
 drivers/nvme/target/zns.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index c7ef69f29fe4..606f21ee21bf 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -100,6 +100,7 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 	struct nvme_id_ns_zns *id_zns;
 	u64 zsze;
 	u16 status;
+	u32 mar, mor;
 
 	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
 		req->error_loc = offsetof(struct nvme_identify, nsid);
@@ -130,8 +131,10 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
 					req->ns->blksize_shift;
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
-	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
-	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
+	mor = bdev_max_open_zones(req->ns->bdev);
+	id_zns->mor = cpu_to_le32(--mor);
+	mar = bdev_max_active_zones(req->ns->bdev);
+	id_zns->mar = cpu_to_le32(--mar);
 
 done:
 	status = nvmet_copy_to_sgl(req, 0, id_zns, sizeof(*id_zns));
-- 
2.25.1

Re: [PATCH] nvmet: fix mar and mor off-by-one errors

[Niklas Cassel]

On Mon, Sep 05, 2022 at 02:21:16PM +0200, Dennis Maisenbacher wrote:
> From: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
> 
> Maximum Active Resources (MAR) and Maximum Open Resources (MOR) are 0's
> based vales where a value of 0xffffffff indicates that there is no limit.
> 
> Cast the unsigned int values that are returned by bdev_max_open_zones and
> bdev_max_active_zones into u32 vales which need to be decremented as the
> returned values of the block layer helpers are not 0's based.
> The cast to u32 is necessary because the size of unsigned int is
> architecture dependent and a 0 reported by the block layer helpers
> indicates no limit, thus it needs to be converted to 0xffffffff which
> happens by underflowing the u32.
> 
> Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
> Signed-off-by: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
> ---
>  drivers/nvme/target/zns.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index c7ef69f29fe4..606f21ee21bf 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c
> @@ -100,6 +100,7 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>  	struct nvme_id_ns_zns *id_zns;
>  	u64 zsze;
>  	u16 status;
> +	u32 mar, mor;
>  
>  	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>  		req->error_loc = offsetof(struct nvme_identify, nsid);
> @@ -130,8 +131,10 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>  	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>  					req->ns->blksize_shift;
>  	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
> -	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
> -	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
> +	mor = bdev_max_open_zones(req->ns->bdev);
> +	id_zns->mor = cpu_to_le32(--mor);
> +	mar = bdev_max_active_zones(req->ns->bdev);
> +	id_zns->mar = cpu_to_le32(--mar);
>  
>  done:
>  	status = nvmet_copy_to_sgl(req, 0, id_zns, sizeof(*id_zns));
> -- 
> 2.25.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH] nvmet: fix mar and mor off-by-one errors

[Christoph Hellwig]

On Mon, Sep 05, 2022 at 02:21:16PM +0200, Dennis Maisenbacher wrote:
> Cast the unsigned int values that are returned by bdev_max_open_zones and
> bdev_max_active_zones into u32 vales which need to be decremented as the
> returned values of the block layer helpers are not 0's based.
> The cast to u32 is necessary because the size of unsigned int is
> architecture dependent and a 0 reported by the block layer helpers
> indicates no limit, thus it needs to be converted to 0xffffffff which
> happens by underflowing the u32.

unsigned int (in Linux, not the C standard) is not architecture
dependent but always a 32-bit unsigned integer type.

But I'd much rather see an explicit check for 0 and conversion to
0xffffffff anyway.  Yes, unsigned integer underflow is well defined,
but having the explicit check explains much better to the read that it
is intentional.

Can you also add a Fixes tag, please?

[PATCH] nvmet: fix mar and mor off-by-one errors

[Dennis Maisenbacher]

>> Cast the unsigned int values that are returned by bdev_max_open_zones and
>> bdev_max_active_zones into u32 vales which need to be decremented as the
>> returned values of the block layer helpers are not 0's based.
>> The cast to u32 is necessary because the size of unsigned int is
>> architecture dependent and a 0 reported by the block layer helpers
>> indicates no limit, thus it needs to be converted to 0xffffffff which
>> happens by underflowing the u32.
>
>unsigned int (in Linux, not the C standard) is not architecture
>dependent but always a 32-bit unsigned integer type.

Thanks for clarifying!

>But I'd much rather see an explicit check for 0 and conversion to
>0xffffffff anyway.  Yes, unsigned integer underflow is well defined,
>but having the explicit check explains much better to the read that it
>is intentional.
>
>Can you also add a Fixes tag, please?

Sure, makes sense. I will address your comments in v2.

Dennis