Received-SPF: pass (google.com: domain of linux-kernel+bounces-182043-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: multipart/signed;
 boundary=f27161c941001cd0bbbbc62b61244dbaa44f7ccf8dabf629896d85fb65df;
 micalg=pgp-sha384; protocol="application/pgp-signature"
Date: Fri, 17 May 2024 13:21:43 +0200
Message-Id: <D1BVPABXTSPH.22ML1GLOMHEJR@kernel.org>
To: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>,
 "Chun-Kuang Hu" <chunkuang.hu@kernel.org>, "Philipp Zabel"
 <p.zabel@pengutronix.de>, "David Airlie" <airlied@gmail.com>, "Daniel
 Vetter" <daniel@ffwll.ch>, "Matthias Brugger" <matthias.bgg@gmail.com>
Subject: Re: [PATCH] drm/mediatek/dp: fix spurious kfree()
Cc: "Jani Nikula" <jani.nikula@intel.com>, "Chen-Yu Tsai"
 <wenst@chromium.org>, <linux-mediatek@lists.infradead.org>,
 <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>
From: "Michael Walle" <mwalle@kernel.org>
References: <20240517093024.1702750-1-mwalle@kernel.org>
 <de0191f3-271b-4f0e-aa73-910543587c9d@collabora.com>
 <D1BVE3G40OVL.3KX13LU75M122@kernel.org>
 <0d8f89d4-e16e-4c8d-b983-38df8fcc387e@collabora.com>
In-Reply-To: <0d8f89d4-e16e-4c8d-b983-38df8fcc387e@collabora.com>
Precedence: bulk

--f27161c941001cd0bbbbc62b61244dbaa44f7ccf8dabf629896d85fb65df
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

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/medi=
atek/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 =3D drm_edid_raw(drm_edid);
> >>>    		struct cea_sad *sads;
> >>> +		int ret;
> >>>   =20
> >>> -		audio_caps->sad_count =3D drm_edid_to_sad(edid, &sads);
> >>> -		kfree(sads);
> >>> +		ret =3D drm_edid_to_sad(edid, &sads);
> >>> +		/* Ignore any errors */
> >>> +		if (ret < 0)
> >>> +			ret =3D 0;
> >>> +		if (ret)
> >>
> >> Eh, this will never work, because you're clearing the error before che=
cking
> >> if there's any error here?!?! :-P
> >=20
> > 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.
> >=20
> >> Anyway in reality, it returns -ENOMEM if the allocation was not succes=
sful...
> >> 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 !=3D -ENOMEM)
> >> 	kfree(sads)
> >>
> >> audio_caps->sad_count =3D ret < 0 ? 0 : ret;
> >=20
> > Which is the same as above, but you only check for ENOMEM?
> >=20
>
> 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.

-michael

--f27161c941001cd0bbbbc62b61244dbaa44f7ccf8dabf629896d85fb65df
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iKgEABMJADAWIQTIVZIcOo5wfU/AngkSJzzuPgIf+AUCZkc9yBIcbXdhbGxlQGtl
cm5lbC5vcmcACgkQEic87j4CH/hZRgGA3L/dMBtHlbW7ZOV5nIDapRev3O4bmq6g
8ONEpwEM2TT1gUSVO++inHv0WwdC/5P5AX45dE/OEijkk2jcFlW7qjVKOYztnjwz
6Augvm2E5aMYSW9zE3gdpGOeijQL7myf92w=
=7cyw
-----END PGP SIGNATURE-----

--f27161c941001cd0bbbbc62b61244dbaa44f7ccf8dabf629896d85fb65df--