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

[Klaus Jensen <its@irrelevant.dk>]

[-- 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 --]