On 22.03.21 07:19, Klaus Jensen wrote:
From: Klaus Jensen <k.jen...@samsung.com>
In nvme_format_ns(), if the namespace is of zero size (which might be
useless, but not invalid), the `count` variable will leak. Fix this by
returning early in that case.
When looking at the Coverity report, something else caught my eye: As
far as I’m aware, blk_aio_pwrite_zeroes() may invoke the CB before
returning (if blk_do_pwritev_part() returns without yielding). I don’t
think that will happen with real hardware (who knows, though), but it
should be possible to see with the null-co block driver.
nvme_format_ns() doesn’t quite look like it takes that into account.
For example, because *count starts at 1 and is decremented after the
while (len) loop, all nvme_aio_format_cb() invocations (if they are
invoked before their blk_aio_pwrite_zeroes() returns) will see
*count == 2, and thus not free it, or call nvme_enqueue_req_completion().
I don’t know whether the latter is problematic, but not freeing `count`
doesn’t seem right. Perhaps this could be addressed by adding a
condition to the `(*count)--` to see whether `(*count)-- == 1` (or
rather `--(*count) == 0`), which would indicate that there are no AIO
functions still in flight?
Max
Reported-by: Coverity (CID 1451082)
Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
---
hw/block/nvme.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6842b01ab58b..dad275971a84 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4984,6 +4984,11 @@ static uint16_t nvme_format_ns(NvmeCtrl *n,
NvmeNamespace *ns, uint8_t lbaf,
ns->status = NVME_FORMAT_IN_PROGRESS;
len = ns->size;
+
+ if (!len) {
+ return NVME_SUCCESS;
+ }
+
offset = 0;
count = g_new(int, 1);