From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Zimmermann Subject: Re: [PATCH, RESEND] drm/vmwgfx: Don't double-free the mode stored in par->set_mode Date: Tue, 19 Mar 2019 18:56:26 +0100 Message-ID: <70fd5a3d-f762-29f8-134d-2859d939d0f5@suse.de> References: <20190318144758.21267-1-tzimmermann@suse.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1038181950==" Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 67065899A5 for ; Tue, 19 Mar 2019 17:56:33 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Deepak Singh Rawat , thellstrom@vmware.com, linux-graphics-maintainer@vmware.com Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============1038181950== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="63guAAvy56352DzAi8BodfLT8lWcsCquC" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --63guAAvy56352DzAi8BodfLT8lWcsCquC Content-Type: multipart/mixed; boundary="UMlJxElBzqPHKJ30MtlpDljXZb3F3CDkg"; protected-headers="v1" From: Thomas Zimmermann To: Deepak Singh Rawat , thellstrom@vmware.com, linux-graphics-maintainer@vmware.com Cc: dri-devel@lists.freedesktop.org Message-ID: <70fd5a3d-f762-29f8-134d-2859d939d0f5@suse.de> Subject: Re: [PATCH, RESEND] drm/vmwgfx: Don't double-free the mode stored in par->set_mode References: <20190318144758.21267-1-tzimmermann@suse.de> In-Reply-To: --UMlJxElBzqPHKJ30MtlpDljXZb3F3CDkg Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Deepak Am 18.03.19 um 17:59 schrieb Deepak Singh Rawat: > Hi Thomas, >=20 > Thanks for doing this and somehow I missed the last patch, sorry about > that. Have some questions below otherwise the patch looks good to me. >=20 > Reviewed-by: Deepak Rawat >=20 > I will include your changes in vmwgfx-next and run tests. Thank you. >=20 > On Mon, 2019-03-18 at 15:47 +0100, Thomas Zimmermann wrote: >> When calling vmw_fb_set_par(), the mode stored in par->set_mode gets >> free'd >> twice. The first free is in vmw_fb_kms_detach(), the second is near >> the >> end of vmw_fb_set_par() under the name of 'old_mode'. The mode- >> setting code >> only works correctly if the mode doesn't actually change. >=20 > You mean to say that without your patch vmwgfx fb driver fail to change= > the mode? Yes. It's hard to trigger as the usual resolution always appears to be 800x600. We (SUSE) have a customer with a custom X11 modeline setting, which sets the resolution to something different. In this case, the double-free bug happens. >=20 >> Removing 'old_mode' >> in favor of using par->set_mode directly fixes the problem. >> >> Signed-off-by: Thomas Zimmermann >> --- >> drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 12 +++--------- >> 1 file changed, 3 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c >> b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c >> index b913a56f3426..2a9112515f46 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c >> @@ -564,11 +564,9 @@ static int vmw_fb_set_par(struct fb_info *info) >> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >> DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) >> }; >> - struct drm_display_mode *old_mode; >> struct drm_display_mode *mode; >> int ret; >> >> - old_mode =3D par->set_mode; >> mode =3D drm_mode_duplicate(vmw_priv->dev, &new_mode); >> if (!mode) { >> DRM_ERROR("Could not create new fb mode.\n"); >> @@ -579,11 +577,7 @@ static int vmw_fb_set_par(struct fb_info *info) >> mode->vdisplay =3D var->yres; >> vmw_guess_mode_timing(mode); >> >> - if (old_mode && drm_mode_equal(old_mode, mode)) { >> - drm_mode_destroy(vmw_priv->dev, mode); >> - mode =3D old_mode; >> - old_mode =3D NULL; >=20 > I am having hard time understanding original intention for this piece > of code. Was there a restriction to send pointer to old mode if mode > were same and that restriction don't hold anymore. Sorry I am not > familiar with this code area. Hmm, I can only guess about motivation: it looks like the author wanted to keep the original value of 'par->set_mode' unless the mode really changes. But it doesn't make a difference whether the pointer's value changes or not. Best regards Thomas >=20 >> - } else if (!vmw_kms_validate_mode_vram(vmw_priv, >> + if (!vmw_kms_validate_mode_vram(vmw_priv, >> mode->hdisplay * >> DIV_ROUND_UP(var- >>> bits_per_pixel, 8), >> mode->vdisplay)) { >> @@ -620,8 +614,8 @@ static int vmw_fb_set_par(struct fb_info *info) >> schedule_delayed_work(&par->local_work, 0); >> >> out_unlock: >> - if (old_mode) >> - drm_mode_destroy(vmw_priv->dev, old_mode); >> + if (par->set_mode) >> + drm_mode_destroy(vmw_priv->dev, par->set_mode); >> par->set_mode =3D mode; >> >> mutex_unlock(&par->bo_mutex); >> -- >> 2.20.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> > https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flis= ts.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=3D02%7C01%7C= drawat%40vmware.com%7Cb1508247a3954fb0b08b08d6abb0bcd0%7Cb39138ca3cee4b4a= a4d6cd83d9dd62f0%7C0%7C0%7C636885172908359973&sdata=3DAN6UTrMzxcVK7MC= 6bX3OxNbcyq0j4HdKt0dk1yyHOHc%3D&reserved=3D0 >=20 --=20 Thomas Zimmermann Graphics Driver Developer SUSE Linux GmbH, Maxfeldstr. 5, D-90409 N=C3=BCrnberg Tel: +49-911-74053-0; Fax: +49-911-7417755; https://www.suse.com/ SUSE Linux GmbH, GF: Felix Imend=C3=B6rffer, Jane Smithard, Graham Norton, HRB 21284 (AG N=C3=BCrnberg) --UMlJxElBzqPHKJ30MtlpDljXZb3F3CDkg-- --63guAAvy56352DzAi8BodfLT8lWcsCquC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEchf7rIzpz2NEoWjlaA3BHVMLeiMFAlyRLU8ACgkQaA3BHVML eiNR5wf+LP9mdYJp+NYsZiWggIwrS880liZc77eiM8/6nuXsdR9UpXnD3sudkovF jSO8SHencz2bpG3oXNLyzmxT3IFu8XOjndat5oNWR+ltPQVQTHNrBT7ASmz0/Zfd Q1SmrhQXDxzBDGioTvusZOW8+3yHnaV24pF/w8jeYPaRMlpIg5KWzXbkJ2heNaHJ bebqkgpSZ9WU5Jonl6hxbGMDb9IIQ3Xtb7BKpJTencQsQGHsYQg1o3mr+oEwjPfM jurWCIDQ2eLYfhL6HagYpm/Icl499Fra51LlZ08akSn08eoq605dz1XedGTtXNZ3 HPtMUmMxK0sQfl9B+Ic/pGEyBCb+0g== =99/J -----END PGP SIGNATURE----- --63guAAvy56352DzAi8BodfLT8lWcsCquC-- --===============1038181950== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1038181950==--