[PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Gollu Appalanaidu]

NSZE is the total size of the namespace in logical blocks. So the max
addressable logical block is NLB minus 1. So your starting logical
block is equal to NSZE it is a out of range.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 953ec64729..be9edb1158 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
             uint64_t slba = le64_to_cpu(range[i].slba);
             uint32_t nlb = le32_to_cpu(range[i].nlb);
 
-            if (nvme_check_bounds(ns, slba, nlb)) {
+            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
                 trace_pci_nvme_err_invalid_lba_range(slba, nlb,
                                                      ns->id_ns.nsze);
                 continue;
-- 
2.17.1

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Minwoo Im]

On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> NSZE is the total size of the namespace in logical blocks. So the max
> addressable logical block is NLB minus 1. So your starting logical
> block is equal to NSZE it is a out of range.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 953ec64729..be9edb1158 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>              uint64_t slba = le64_to_cpu(range[i].slba);
>              uint32_t nlb = le32_to_cpu(range[i].nlb);
>  
> -            if (nvme_check_bounds(ns, slba, nlb)) {
> +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {

This patch also looks like check the boundary about slba.  Should it be
also checked inside of nvme_check_bounds() ?

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On Apr  9 20:05, Minwoo Im wrote:
>On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
>> NSZE is the total size of the namespace in logical blocks. So the max
>> addressable logical block is NLB minus 1. So your starting logical
>> block is equal to NSZE it is a out of range.
>>
>> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> ---
>>  hw/block/nvme.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 953ec64729..be9edb1158 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>>              uint64_t slba = le64_to_cpu(range[i].slba);
>>              uint32_t nlb = le32_to_cpu(range[i].nlb);
>>
>> -            if (nvme_check_bounds(ns, slba, nlb)) {
>> +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
>
>This patch also looks like check the boundary about slba.  Should it be
>also checked inside of nvme_check_bounds() ?

The catch here is that DSM is like the only command where the number of 
logical blocks is a 1s-based value. Otherwise we always have nlb > 0, 
which means that nvme_check_bounds() will always "do the right thing".

My main gripe here is that (in my mind), by definition, a "zero length 
range" does not reference any LBAs at all. So how can it result in LBA 
Out of Range?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Minwoo Im]

On 21-04-09 13:55:01, Klaus Jensen wrote:
> On Apr  9 20:05, Minwoo Im wrote:
> > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > > NSZE is the total size of the namespace in logical blocks. So the max
> > > addressable logical block is NLB minus 1. So your starting logical
> > > block is equal to NSZE it is a out of range.
> > > 
> > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > > ---
> > >  hw/block/nvme.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 953ec64729..be9edb1158 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
> > >              uint64_t slba = le64_to_cpu(range[i].slba);
> > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
> > > 
> > > -            if (nvme_check_bounds(ns, slba, nlb)) {
> > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
> > 
> > This patch also looks like check the boundary about slba.  Should it be
> > also checked inside of nvme_check_bounds() ?
> 
> The catch here is that DSM is like the only command where the number of
> logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
> means that nvme_check_bounds() will always "do the right thing".
> 
> My main gripe here is that (in my mind), by definition, a "zero length
> range" does not reference any LBAs at all. So how can it result in LBA Out
> of Range?

Even if this is not the LBA out of range case which is currently what
nvme_check_bounds() checking, but I thought the function checks the
bounds so that we can add one more check inside of that function like:
(If SLBA is 0-based or not, slba should not be nsze, isn't it ?)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7244534a89e9..25a7db5ecbd8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1415,6 +1415,10 @@ static inline uint16_t nvme_check_bounds(NvmeNamespace *ns, uint64_t slba,
 {
     uint64_t nsze = le64_to_cpu(ns->id_ns.nsze);
 
+    if (slba == nsze) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
     if (unlikely(UINT64_MAX - slba < nlb || slba + nlb > nsze)) {
         return NVME_LBA_RANGE | NVME_DNR;
     }

Or am I missing something here ;) ?

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Apr  9 21:31, Minwoo Im wrote:
>On 21-04-09 13:55:01, Klaus Jensen wrote:
>> On Apr  9 20:05, Minwoo Im wrote:
>> > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
>> > > NSZE is the total size of the namespace in logical blocks. So the max
>> > > addressable logical block is NLB minus 1. So your starting logical
>> > > block is equal to NSZE it is a out of range.
>> > >
>> > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> > > ---
>> > >  hw/block/nvme.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> > > index 953ec64729..be9edb1158 100644
>> > > --- a/hw/block/nvme.c
>> > > +++ b/hw/block/nvme.c
>> > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>> > >              uint64_t slba = le64_to_cpu(range[i].slba);
>> > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
>> > >
>> > > -            if (nvme_check_bounds(ns, slba, nlb)) {
>> > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
>> >
>> > This patch also looks like check the boundary about slba.  Should it be
>> > also checked inside of nvme_check_bounds() ?
>>
>> The catch here is that DSM is like the only command where the number of
>> logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
>> means that nvme_check_bounds() will always "do the right thing".
>>
>> My main gripe here is that (in my mind), by definition, a "zero length
>> range" does not reference any LBAs at all. So how can it result in LBA Out
>> of Range?
>
>Even if this is not the LBA out of range case which is currently what
>nvme_check_bounds() checking, but I thought the function checks the
>bounds so that we can add one more check inside of that function like:
>(If SLBA is 0-based or not, slba should not be nsze, isn't it ?)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 7244534a89e9..25a7db5ecbd8 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -1415,6 +1415,10 @@ static inline uint16_t nvme_check_bounds(NvmeNamespace *ns, uint64_t slba,
> {
>     uint64_t nsze = le64_to_cpu(ns->id_ns.nsze);
>
>+    if (slba == nsze) {
>+        return NVME_INVALID_FIELD | NVME_DNR;
>+    }
>+
>     if (unlikely(UINT64_MAX - slba < nlb || slba + nlb > nsze)) {
>         return NVME_LBA_RANGE | NVME_DNR;
>     }
>
>Or am I missing something here ;) ?

No, not at all, it's just that this additional check is never needed for 
any other command than DSM since, as far as I remember, DSM is the only 
command with the 1s-based NLB value fuckup.

This means that nlb will always be at least 1, so slba + 1 > nsze will 
be false if slba == nsze.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Minwoo Im]

On 21-04-09 14:36:19, Klaus Jensen wrote:
> On Apr  9 21:31, Minwoo Im wrote:
> > On 21-04-09 13:55:01, Klaus Jensen wrote:
> > > On Apr  9 20:05, Minwoo Im wrote:
> > > > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > > > > NSZE is the total size of the namespace in logical blocks. So the max
> > > > > addressable logical block is NLB minus 1. So your starting logical
> > > > > block is equal to NSZE it is a out of range.
> > > > >
> > > > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > > > > ---
> > > > >  hw/block/nvme.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > > index 953ec64729..be9edb1158 100644
> > > > > --- a/hw/block/nvme.c
> > > > > +++ b/hw/block/nvme.c
> > > > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
> > > > >              uint64_t slba = le64_to_cpu(range[i].slba);
> > > > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
> > > > >
> > > > > -            if (nvme_check_bounds(ns, slba, nlb)) {
> > > > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
> > > >
> > > > This patch also looks like check the boundary about slba.  Should it be
> > > > also checked inside of nvme_check_bounds() ?
> > > 
> > > The catch here is that DSM is like the only command where the number of
> > > logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
> > > means that nvme_check_bounds() will always "do the right thing".
> > > 
> > > My main gripe here is that (in my mind), by definition, a "zero length
> > > range" does not reference any LBAs at all. So how can it result in LBA Out
> > > of Range?
> > 
> > Even if this is not the LBA out of range case which is currently what
> > nvme_check_bounds() checking, but I thought the function checks the
> > bounds so that we can add one more check inside of that function like:
> > (If SLBA is 0-based or not, slba should not be nsze, isn't it ?)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7244534a89e9..25a7db5ecbd8 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1415,6 +1415,10 @@ static inline uint16_t nvme_check_bounds(NvmeNamespace *ns, uint64_t slba,
> > {
> >     uint64_t nsze = le64_to_cpu(ns->id_ns.nsze);
> > 
> > +    if (slba == nsze) {
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> >     if (unlikely(UINT64_MAX - slba < nlb || slba + nlb > nsze)) {
> >         return NVME_LBA_RANGE | NVME_DNR;
> >     }
> > 
> > Or am I missing something here ;) ?
> 
> No, not at all, it's just that this additional check is never needed for any
> other command than DSM since, as far as I remember, DSM is the only command
> with the 1s-based NLB value fuckup.
> 
> This means that nlb will always be at least 1, so slba + 1 > nsze will be
> false if slba == nsze.

Understood :)

Please have:

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Keith Busch]

On Fri, Apr 09, 2021 at 01:55:01PM +0200, Klaus Jensen wrote:
> On Apr  9 20:05, Minwoo Im wrote:
> > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > > NSZE is the total size of the namespace in logical blocks. So the max
> > > addressable logical block is NLB minus 1. So your starting logical
> > > block is equal to NSZE it is a out of range.
> > > 
> > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > > ---
> > >  hw/block/nvme.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 953ec64729..be9edb1158 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
> > >              uint64_t slba = le64_to_cpu(range[i].slba);
> > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
> > > 
> > > -            if (nvme_check_bounds(ns, slba, nlb)) {
> > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
> > 
> > This patch also looks like check the boundary about slba.  Should it be
> > also checked inside of nvme_check_bounds() ?
> 
> The catch here is that DSM is like the only command where the number of
> logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
> means that nvme_check_bounds() will always "do the right thing".
> 
> My main gripe here is that (in my mind), by definition, a "zero length
> range" does not reference any LBAs at all. So how can it result in LBA Out
> of Range?

So what's the problem? If the request is to discard 0 blocks starting
from the last block, then that's valid. Is this patch actually fixing
anything?

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

On Apr 10 00:30, Keith Busch wrote:
>On Fri, Apr 09, 2021 at 01:55:01PM +0200, Klaus Jensen wrote:
>> On Apr  9 20:05, Minwoo Im wrote:
>> > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
>> > > NSZE is the total size of the namespace in logical blocks. So the max
>> > > addressable logical block is NLB minus 1. So your starting logical
>> > > block is equal to NSZE it is a out of range.
>> > >
>> > > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> > > ---
>> > >  hw/block/nvme.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> > > index 953ec64729..be9edb1158 100644
>> > > --- a/hw/block/nvme.c
>> > > +++ b/hw/block/nvme.c
>> > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>> > >              uint64_t slba = le64_to_cpu(range[i].slba);
>> > >              uint32_t nlb = le32_to_cpu(range[i].nlb);
>> > >
>> > > -            if (nvme_check_bounds(ns, slba, nlb)) {
>> > > +            if (nvme_check_bounds(ns, slba, nlb) || slba == ns->id_ns.nsze) {
>> >
>> > This patch also looks like check the boundary about slba.  Should it be
>> > also checked inside of nvme_check_bounds() ?
>>
>> The catch here is that DSM is like the only command where the number of
>> logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
>> means that nvme_check_bounds() will always "do the right thing".
>>
>> My main gripe here is that (in my mind), by definition, a "zero length
>> range" does not reference any LBAs at all. So how can it result in LBA Out
>> of Range?
>
>So what's the problem? If the request is to discard 0 blocks starting
>from the last block, then that's valid. Is this patch actually fixing
>anything?
>

If SLBA == NSZE we are out of bounds since the last addressable block is 
NSZE-1. But, I don't consider the current behavior buggy or wrong, the 
devices correctly handles the zero length range by just not discarding 
anything anywhere.

The spec is pretty unclear on how invalid ranges in DSM are handled. My 
interpretation is that the advisory nature of DSM allows it to do best 
effort, but as Gollu is suggesting here, a device could just as well 
decide to validate the ranges and return an appropriate status code if 
it wanted to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]