From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Barnett Subject: Re: [PATCH v3] drm/amdgpu: user pages array memory leak fix Date: Sat, 12 Oct 2019 09:12:26 -0700 Message-ID: References: <20191011143620.8785-1-Philip.Yang@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1161036630==" Return-path: In-Reply-To: <20191011143620.8785-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org> List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Yang, Philip" Cc: "Koenig, Christian" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============1161036630== Content-Type: multipart/alternative; boundary="00000000000039a1ee0594b8e7cf" --00000000000039a1ee0594b8e7cf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Confirming that v3 patch still fixes the bug. Thanks, -Joe On Fri, Oct 11, 2019 at 7:36 AM Yang, Philip wrote: > user_pages array should always be freed after validation regardless if > user pages are changed after bo is created because with HMM change parse > bo always allocate user pages array to get user pages for userptr bo. > > v2: remove unused local variable and amend commit > > v3: add back get user pages in gem_userptr_ioctl, to detect application > bug where an userptr VMA is not ananymous memory and reject it. > > Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962 > > Signed-off-by: Philip Yang > Tested-by: Joe Barnett > Reviewed-by: Christian K=C3=B6nig > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index c18a153b3d2a..e7b39daa22f6 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -476,7 +476,6 @@ static int amdgpu_cs_list_validate(struct > amdgpu_cs_parser *p, > > list_for_each_entry(lobj, validated, tv.head) { > struct amdgpu_bo *bo =3D ttm_to_amdgpu_bo(lobj->tv.bo); > - bool binding_userptr =3D false; > struct mm_struct *usermm; > > usermm =3D amdgpu_ttm_tt_get_usermm(bo->tbo.ttm); > @@ -493,14 +492,13 @@ static int amdgpu_cs_list_validate(struct > amdgpu_cs_parser *p, > > amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm, > lobj->user_pages); > - binding_userptr =3D true; > } > > r =3D amdgpu_cs_validate(p, bo); > if (r) > return r; > > - if (binding_userptr) { > + if (lobj->user_pages) { > kvfree(lobj->user_pages); > lobj->user_pages =3D NULL; > } > -- > 2.17.1 > > --00000000000039a1ee0594b8e7cf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Confirming that v3 patch still fixes the bug.

Thanks,
-Joe

On Fri, Oct 11, 2019 at 7= :36 AM Yang, Philip <Philip.Yang@= amd.com> wrote:
user_pages array should always be freed after validation regardless = if
user pages are changed after bo is created because with HMM change parse bo always allocate user pages array to get user pages for userptr bo.

v2: remove unused local variable and amend commit

v3: add back get user pages in gem_userptr_ioctl, to detect application
bug where an userptr VMA is not ananymous memory and reject it.

Bugzilla: https://bugs.launchpad.net/ubu= ntu/+source/linux/+bug/1844962

Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org>
Tested-by: Joe Barnett <thejoe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Christian K=C3=B6nig <christian.koenig-5C7GfCeVMHo@public.gmane.org>
---
=C2=A0drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
=C2=A01 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/a= mdgpu/amdgpu_cs.c
index c18a153b3d2a..e7b39daa22f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -476,7 +476,6 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_par= ser *p,

=C2=A0 =C2=A0 =C2=A0 =C2=A0 list_for_each_entry(lobj, validated, tv.head) {=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct amdgpu_bo *b= o =3D ttm_to_amdgpu_bo(lobj->tv.bo);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bool binding_userpt= r =3D false;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct mm_struct *u= sermm;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 usermm =3D amdgpu_t= tm_tt_get_usermm(bo->tbo.ttm);
@@ -493,14 +492,13 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_p= arser *p,

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lobj->user_pages);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0binding_userptr =3D true;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r =3D amdgpu_cs_val= idate(p, bo);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 return r;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (binding_userptr= ) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (lobj->user_p= ages) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 kvfree(lobj->user_pages);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 lobj->user_pages =3D NULL;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
--
2.17.1

--00000000000039a1ee0594b8e7cf-- --===============1161036630== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============1161036630==--