[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Klaus Jensen
Subject: Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based
Date: Fri, 9 Apr 2021 18:57:58 +0200

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

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: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]