[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts
From: |
Klaus Jensen |
Subject: |
Re: [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts |
Date: |
Fri, 12 Mar 2021 16:11:29 +0100 |
On Mar 12 13:07, Peter Maydell wrote:
> On Tue, 9 Mar 2021 at 11:45, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > ZASL (Zone Append Size Limit) is defined exactly like MDTS (Maximum Data
> > Transfer Size), that is, it is a value in units of the minimum memory
> > page size (CAP.MPSMIN) and is reported as a power of two.
> >
> > The 'mdts' nvme device parameter is specified as in the spec, but the
> > 'zoned.append_size_limit' parameter is specified in bytes. This is
> > suboptimal for a number of reasons:
> >
> > 1. It is just plain confusing wrt. the definition of mdts.
> > 2. There is a lot of complexity involved in validating the value; it
> > must be a power of two, it should be larger than 4k, if it is zero
> > we set it internally to mdts, but still report it as zero.
> > 3. While "hw/block/nvme: improve invalid zasl value reporting"
> > slightly improved the handling of the parameter, the validation is
> > still wrong; it does not depend on CC.MPS, it depends on
> > CAP.MPSMIN. And we are not even checking that it is actually less
> > than or equal to MDTS, which is kinda the *one* condition it must
> > satisfy.
> >
> > Fix this by defining zasl exactly like mdts and checking the one thing
> > that it must satisfy (that it is less than or equal to mdts). Also,
> > change the default value from 128KiB to 0 (aka, whatever mdts is).
>
> > @@ -2144,10 +2142,9 @@ static uint16_t nvme_do_write(NvmeCtrl *n,
> > NvmeRequest *req, bool append,
> > goto invalid;
> > }
> >
> > - if (nvme_l2b(ns, nlb) > (n->page_size << n->zasl)) {
> > - trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
> > - status = NVME_INVALID_FIELD;
> > - goto invalid;
> > + if (n->params.zasl && data_size > n->page_size <<
> > n->params.zasl) {
> > + trace_pci_nvme_err_zasl(data_size);
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > slba = zone->w_ptr;
>
> Hi; Coverity points out a possible overflow here (CID 1450756):
> n->page_size is a uint32_t, and n->params.zasl is a uint8_t, so
> the "n->page_size << n->params.zasl" will be done as 32-bit arithmetic;
> but it is then compared against a uint64_t data_size.
>
> Is this an overflow that can never happen (ie a false positive), or
> should the RHS of the comparison be done as 64-bit arithmetic by
> adding a cast ?
>
Thanks!
I think a cast is in order. I will get a fix out.
signature.asc
Description: PGP signature
- [PULL v2 10/38] nvme: updated shared header for copy command, (continued)
- [PULL v2 10/38] nvme: updated shared header for copy command, Klaus Jensen, 2021/03/09
- [PULL v2 11/38] hw/block/nvme: add simple copy command, Klaus Jensen, 2021/03/09
- [PULL v2 12/38] hw/block/nvme: fix Close Zone, Klaus Jensen, 2021/03/09
- [PULL v2 13/38] hw/block/nvme: add missing mor/mar constraint checks, Klaus Jensen, 2021/03/09
- [PULL v2 15/38] hw/block/nvme: use locally assigned QEMU IEEE OUI, Klaus Jensen, 2021/03/09
- [PULL v2 14/38] hw/block/nvme: improve invalid zasl value reporting, Klaus Jensen, 2021/03/09
- [PULL v2 16/38] hw/block/nvme: add broadcast nsid support flush command, Klaus Jensen, 2021/03/09
- [PULL v2 18/38] hw/block/nvme: deduplicate bad mdts trace event, Klaus Jensen, 2021/03/09
- [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts, Klaus Jensen, 2021/03/09
- [PULL v2 20/38] hw/block/nvme: remove unnecessary endian conversion, Klaus Jensen, 2021/03/09
- [PULL v2 17/38] hw/block/nvme: document 'mdts' nvme device parameter, Klaus Jensen, 2021/03/09
- [PULL v2 29/38] hw/block/nvme: remove the req dependency in map functions, Klaus Jensen, 2021/03/09
- [PULL v2 28/38] hw/block/nvme: try to deal with the iov/qsg duality, Klaus Jensen, 2021/03/09
- [PULL v2 30/38] hw/block/nvme: refactor nvme_dma, Klaus Jensen, 2021/03/09
- [PULL v2 21/38] hw/block/nvme: add identify trace event, Klaus Jensen, 2021/03/09
- [PULL v2 22/38] hw/block/nvme: fix potential compilation error, Klaus Jensen, 2021/03/09
- [PULL v2 24/38] hw/block/nvme: report non-mdts command size limit for dsm, Klaus Jensen, 2021/03/09
- [PULL v2 23/38] hw/block/nvme: add trace event for zone read check, Klaus Jensen, 2021/03/09
- [PULL v2 26/38] hw/block/nvme: remove block accounting for write zeroes, Klaus Jensen, 2021/03/09