[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: |
Peter Maydell |
Subject: |
Re: [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts |
Date: |
Fri, 12 Mar 2021 13:07:53 +0000 |
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
-- PMM
- [PULL v2 08/38] hw/block/nvme: refactor zone resource management, (continued)
- [PULL v2 08/38] hw/block/nvme: refactor zone resource management, Klaus Jensen, 2021/03/09
- [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
- Re: [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts,
Peter Maydell <=
- [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