Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20211204205504.6550-1-amhamza.mgc@gmail.com>
References: <163864977875.3153335.18099399866051099554@Monstersaurus> <20211204205504.6550-1-amhamza.mgc@gmail.com>
Subject: Re: [PATCH v2] media: venus: vdec: fixed possible memory leak issue
From:   Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, amhamza.mgc@gmail.com
To:     Ameer Hamza <amhamza.mgc@gmail.com>, agross@kernel.org,
        bjorn.andersson@linaro.org, mchehab@kernel.org,
        stanimir.varbanov@linaro.org
Date:   Mon, 06 Dec 2021 09:55:54 +0000
Message-ID: <163878455464.147210.11589283989656931183@Monstersaurus>
User-Agent: alot/0.10
Precedence: bulk

Hi Ameer,

Thank you for investigating the alternative suggestion I made.

Quoting Ameer Hamza (2021-12-04 20:55:04)
> Fixed coverity warning by freeing the allocated memory before return

We could probably say that fixing the coverity warning isn't so much the
target of the patch as fixing the memory leak. It's just helpful that
coverity spotted it for us.


I'd write:

The venus_helper_alloc_dpb_bufs() implementation allows an early return
on an error path when checking the id from ida_alloc_min() which would
not release the earlier buffer allocation.

Move the direct kfree() from the error checking of dma_alloc_attrs() to
the common fail path to ensure that allocations are released on all
error paths in this function.

> Addresses-Coverity: 1494120 ("Resource leak")
>=20
> Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com>

Of course having suggested it, I believe this is the right fix so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> Changes in v2:
> move kfree() immediately after kfree() as suggested by Kieran Bingham
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/=
platform/qcom/venus/helpers.c
> index 84c3a511ec31..0bca95d01650 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -189,7 +189,6 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *in=
st)
>                 buf->va =3D dma_alloc_attrs(dev, buf->size, &buf->da, GFP=
_KERNEL,
>                                           buf->attrs);
>                 if (!buf->va) {
> -                       kfree(buf);
>                         ret =3D -ENOMEM;
>                         goto fail;
>                 }
> @@ -209,6 +208,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *in=
st)
>         return 0;
> =20
>  fail:
> +       kfree(buf);
>         venus_helper_free_dpb_bufs(inst);
>         return ret;
>  }
> --=20
> 2.25.1
>