linux-kernel - Re: [PATCH 3/4] media: mediatek: vcodec: Fix mtk_vcodec_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAC=S1nhp3HoKGQr1UgKtZJ9SLMqvm-G_YZi9dEWWF3tj2d=OFQ@mail.gmail.com>
Date:   Thu, 7 Dec 2023 19:17:58 +0800
From:   Fei Shao <fshao@...omium.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>, Tiffany Lin <tiffany.lin@...iatek.com>,
        Yunfei Dong <yunfei.dong@...iatek.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 3/4] media: mediatek: vcodec: Fix mtk_vcodec_mem_free()
 error log criteria

Hi Angelo,

On Wed, Dec 6, 2023 at 6:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Il 13/11/23 13:26, Fei Shao ha scritto:
> > mtk_vcodec_mem_free() shouldn't print error if the target DMA buffer has
> > never been allocated or was freed properly in the previous call. That
> > makes log confusing.
> >
> > Update the error path to print log only when the caller attempts to free
> > nonzero-size buffer with VA being NULL, which indicates something indeed
> > went wrong.
> >
> > This brings another benefit that the callers no more need to check
> > mem->va explicitly to avoid the error, which can make the code more
> > compact and neat.
> >
> > Signed-off-by: Fei Shao <fshao@...omium.org>
>
> I think that this error is supposed to catch two issues in one:
>   - We're called to free no memory (something that does make no sense),
>     this may happen for example when calling xxx_free() twice, and it
>     is a mistake that *must* be fixed;
When I made the change, I was thinking of kfree() that doesn't warn
against a NULL pointer.
I imagine mtk_vcodec_mem_free() calls with NULL VA and mem size 0
probably have the similar nuance (if the buffer exists, free it; never
mind otherwise), but I could have missed some important differences
specific to the MTK vcodec driver.

Looking at the mtk_vcodec_mem_free() usages, almost every one of those
checks VA beforehand, but nothing else - they don't warn or do
anything special when they encounter a NULL VA, and they should if
that's a concern.
Some even don't check at all (and I think that's why I ended up seeing
the errors mentioned in the cover letter). As for that, I think
there's nothing else we can fix except prepending "if (mem->va)".
So from all this, I feel perhaps we don't need to worry much about
those NULL VA, and we can further remove the checks (or at least move
it into mtk_vcodec_mem_free()) to trim the lines in the driver. That's
the reason for patch [4/4].

Not sure if that makes sense to you.

>   - We're failing to free memory for real (which you covered)
>
> ....that said, I think that if you want to clarify the error messages
> in this function, it should look something like this:
>
> if (!mem->va) {
>         mtk_v4l2_err(plat_dev, "%s: Tried to free a NULL VA", __func__);
>         if (mem->size)
>                 mtk_v4l2_err(plat_dev, "Failed to free %lu bytes", mem->size);
>         return;
> }
Sure, I can revise the patch with this, but I also want to make sure
if the NULL VA print needs to be an error.
If you still think it should, I guess I'll drop the current patch
[4/4] and instead add the check before every mtk_vcodec_mem_free()
calls. This should also work for the issue I want to address in the
first place.

And thanks for the review.  :)

Regards,
Fei

> Cheers,
> Angelo
>