| Message ID | 20210409074402.7342-1-anaidu.gollu@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based | expand |
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() ?
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?
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 ;) ?
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.
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>
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?
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.
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;
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(-)