Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 17 May 2024 13:23:22 +0200 | Subject | Re: [PATCH] drm/mediatek/dp: fix spurious kfree() | From | AngeloGioacchino Del Regno <> |
| |
Il 17/05/24 13:21, Michael Walle ha scritto: > On Fri May 17, 2024 at 1:09 PM CEST, AngeloGioacchino Del Regno wrote: >> Il 17/05/24 13:07, Michael Walle ha scritto: >>> On Fri May 17, 2024 at 12:35 PM CEST, AngeloGioacchino Del Regno wrote: >>>> Il 17/05/24 11:30, Michael Walle ha scritto: >>>>> drm_edid_to_sad() might return an error or just zero. If that is the >>>>> case, we must not free the SADs because there was no allocation in >>>>> the first place. >>>>> >>>>> Fixes: dab12fa8d2bd ("drm/mediatek/dp: fix memory leak on ->get_edid callback audio detection") >>>>> Signed-off-by: Michael Walle <mwalle@kernel.org> >>>>> --- >>>>> drivers/gpu/drm/mediatek/mtk_dp.c | 10 ++++++++-- >>>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c >>>>> index 536366956447..ada12927bbac 100644 >>>>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c >>>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c >>>>> @@ -2073,9 +2073,15 @@ static const struct drm_edid *mtk_dp_edid_read(struct drm_bridge *bridge, >>>>> */ >>>>> const struct edid *edid = drm_edid_raw(drm_edid); >>>>> struct cea_sad *sads; >>>>> + int ret; >>>>> >>>>> - audio_caps->sad_count = drm_edid_to_sad(edid, &sads); >>>>> - kfree(sads); >>>>> + ret = drm_edid_to_sad(edid, &sads); >>>>> + /* Ignore any errors */ >>>>> + if (ret < 0) >>>>> + ret = 0; >>>>> + if (ret) >>>> >>>> Eh, this will never work, because you're clearing the error before checking >>>> if there's any error here?!?! :-P >>> >>> Don't get what you mean? Yes, I'm ignoring the error. Thus, in case >>> of an error ret will be zero and there will be no free. If ret was >>> zero, there won't be a free either. So you're left with the "normal" >>> case, where you have to free the sads. Just like before. >>> >>>> Anyway in reality, it returns -ENOMEM if the allocation was not successful... >>>> in the event that any future update adds any other error we'd be back with the same >>>> issue, but I'm not sure how much should we worry about that. >>>> >>>> To be extremely safe, we could do... >>>> >>>> if (ret != -ENOMEM) >>>> kfree(sads) >>>> >>>> audio_caps->sad_count = ret < 0 ? 0 : ret; >>> >>> Which is the same as above, but you only check for ENOMEM? >>> >> >> Yes, the point is to avoid kfree(sads) for -ENOMEM only, as other errors that may >> be introduced later might still allocate it and leave it allocated. > > Honestly, I doubt that any sane function will allocate memory, then > return an error and expect the caller to free it. >
My point was "you never know". But that wasn't a strong opinion anyway.
It's ok for me regardless of what you choose, either follow what I said or don't, the end result is the same, you're still fixing the issue and both ways are acceptable, so...
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
| ![\](/images/icornerr.gif) |