Re: [PULL 5/5] hw/nvme: flexible data placement emulation

[Peter Maydell]

On Mon, 6 Mar 2023 at 14:34, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Jesper Devantier <j.devantier@samsung.com>
>
> Add emulation of TP4146 ("Flexible Data Placement").
>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Jesper Devantier <j.devantier@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@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