On Mon, 6 Mar 2023 at 14:34, Klaus Jensen <i...@irrelevant.dk> wrote:
>
> From: Jesper Devantier <j.devant...@samsung.com>
>
> Add emulation of TP4146 ("Flexible Data Placement").
>
> Reviewed-by: Keith Busch <kbu...@kernel.org>
> Signed-off-by: Jesper Devantier <j.devant...@samsung.com>
> Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
Hi; Coverity points out what looks like a memory leak
in this function (CID 1507979):
> +static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp)
> +{
> + NvmeEnduranceGroup *endgrp = ns->endgrp;
> + NvmeRuHandle *ruh;
> + uint8_t lbafi = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
> + unsigned int *ruhid, *ruhids;
> + char *r, *p, *token;
> + uint16_t *ph;
> +
> + if (!ns->params.fdp.ruhs) {
> + ns->fdp.nphs = 1;
> + ph = ns->fdp.phs = g_new(uint16_t, 1);
> +
> + ruh = nvme_find_ruh_by_attr(endgrp, NVME_RUHA_CTRL, ph);
> + if (!ruh) {
> + ruh = nvme_find_ruh_by_attr(endgrp, NVME_RUHA_UNUSED, ph);
> + if (!ruh) {
> + error_setg(errp, "no unused reclaim unit handles left");
> + return false;
> + }
> +
> + ruh->ruha = NVME_RUHA_CTRL;
> + ruh->lbafi = lbafi;
> + ruh->ruamw = endgrp->fdp.runs >> ns->lbaf.ds;
> +
> + for (uint16_t rg = 0; rg < endgrp->fdp.nrg; rg++) {
> + ruh->rus[rg].ruamw = ruh->ruamw;
> + }
> + } else if (ruh->lbafi != lbafi) {
> + error_setg(errp, "lba format index of controller assigned "
> + "reclaim unit handle does not match namespace lba "
> + "format index");
> + return false;
> + }
> +
> + return true;
> + }
> +
> + ruhid = ruhids = g_new0(unsigned int, endgrp->fdp.nruh);
Here we allocate memory...
> + r = p = strdup(ns->params.fdp.ruhs);
> +
> + /* parse the placement handle identifiers */
> + while ((token = qemu_strsep(&p, ";")) != NULL) {
> + ns->fdp.nphs += 1;
> + if (ns->fdp.nphs > NVME_FDP_MAXPIDS ||
> + ns->fdp.nphs == endgrp->fdp.nruh) {
> + error_setg(errp, "too many placement handles");
> + free(r);
> + return false;
> + }
> +
> + if (qemu_strtoui(token, NULL, 0, ruhid++) < 0) {
> + error_setg(errp, "cannot parse reclaim unit handle identifier");
> + free(r);
> + return false;
> + }
...but in error-exit paths like these we don't free that memory...
> + }
> +
> + free(r);
> +
> + ph = ns->fdp.phs = g_new(uint16_t, ns->fdp.nphs);
> +
> + ruhid = ruhids;
> +
> + /* verify the identifiers */
> + for (unsigned int i = 0; i < ns->fdp.nphs; i++, ruhid++, ph++) {
> + if (*ruhid >= endgrp->fdp.nruh) {
> + error_setg(errp, "invalid reclaim unit handle identifier");
> + return false;
> + }
> +
> + ruh = &endgrp->fdp.ruhs[*ruhid];
> +
> + switch (ruh->ruha) {
> + case NVME_RUHA_UNUSED:
> + ruh->ruha = NVME_RUHA_HOST;
> + ruh->lbafi = lbafi;
> + ruh->ruamw = endgrp->fdp.runs >> ns->lbaf.ds;
> +
> + for (uint16_t rg = 0; rg < endgrp->fdp.nrg; rg++) {
> + ruh->rus[rg].ruamw = ruh->ruamw;
> + }
> +
> + break;
> +
> + case NVME_RUHA_HOST:
> + if (ruh->lbafi != lbafi) {
> + error_setg(errp, "lba format index of host assigned"
> + "reclaim unit handle does not match namespace "
> + "lba format index");
> + return false;
> + }
> +
> + break;
> +
> + case NVME_RUHA_CTRL:
> + error_setg(errp, "reclaim unit handle is controller assigned");
> + return false;
> +
> + default:
> + abort();
> + }
> +
> + *ph = *ruhid;
> + }
> +
> + return true;
...and it doesn't look like we free it in the happy-path either.
> +}
If this is just working memory that isn't needed once the
function exits then using g_autofree is probably neater than
adding a 'free' on every path that returns from the function.
thanks
-- PMM
