Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 8 Jul 2021 14:04:02 +0300
From:   Pavel Skripkin <paskripkin@gmail.com>
To:     Christian =?UTF-8?B?S8O2bmln?= <christian.koenig@amd.com>
Cc:     ray.huang@amd.com, airlied@linux.ie,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gpu: ttm: fix GPF in ttm_bo_release
Message-ID: <20210708140402.4ce25702@gmail.com>
In-Reply-To: <55aa8ece-1f1d-76d0-4f2f-951d39e79484@amd.com>
References: <20210707185108.3798-1-paskripkin@gmail.com>
        <f4bb7b29-3abc-a056-fc24-4e91b5de8d74@amd.com>
        <20210708113701.4cea7989@gmail.com>
        <20210708130910.63a15c73@gmail.com>
        <55aa8ece-1f1d-76d0-4f2f-951d39e79484@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, 8 Jul 2021 12:56:19 +0200
Christian K=C3=B6nig <christian.koenig@amd.com> wrote:

> Am 08.07.21 um 12:09 schrieb Pavel Skripkin:
> > On Thu, 8 Jul 2021 11:37:01 +0300
> > Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> >> On Thu, 8 Jul 2021 08:49:48 +0200
> >> Christian K=C3=B6nig <christian.koenig@amd.com> wrote:
> >>
> >>> Am 07.07.21 um 20:51 schrieb Pavel Skripkin:
> >>>> My local syzbot instance hit GPF in ttm_bo_release().
> >>>> Unfortunately, syzbot didn't produce a reproducer for this, but I
> >>>> found out possible scenario:
> >>>>
> >>>> drm_gem_vram_create()            <-- drm_gem_vram_object
> >>>> kzalloced (bo embedded in this object)
> >>>>     ttm_bo_init()
> >>>>       ttm_bo_init_reserved()
> >>>>         ttm_resource_alloc()
> >>>>           man->func->alloc()       <-- allocation failure
> >>>>         ttm_bo_put()
> >>>> 	ttm_bo_release()
> >>>> 	  ttm_mem_io_free()      <-- bo->resource =3D=3D NULL passed
> >>>> 				     as second argument
> >>>> 	     *GPF*
> >>>>
> >>>> So, I've added check in ttm_bo_release() to avoid passing
> >>>> NULL as second argument to ttm_mem_io_free().
> >> Hi, Christian!
> >>
> >> Thank you for quick feedback :)
> >>
> >>> There is another ocassion of this a bit down before we call
> >>> ttm_bo_move_to_lru_tail() apart from that good catch.
> >>>
> >> Did you mean, that ttm_bo_move_to_lru_tail() should have NULL check
> >> too?
>=20
> Yes, exactly that.
>=20
> >>   I checked it's realization, and, I think, NULL check is necessary
> >> there, since mem pointer is dereferenced w/o any checking
> >>
> >>> But I'm wondering if we should make the functions NULL save
> >>> instead of the external check.
> >>>
> >> I tried to find more possible scenarios of GPF in ttm_bo_release(),
> >> but I didn't find one. But, yes, moving NULL check inside
> >> ttm_mem_io_free() is more general approach and it will defend this
> >> function from GPFs in the future.
> >>
> >>
> >>
> >> With regards,
> >> Pavel Skripkin
> >>
> > I misclicked and sent this email to Christian privately :(
> >
> > Added all thread participants back, sorry.
>=20
> No problem.
>=20
> Do you want to update your patch or should I take care of this?
>=20

Yes, I will send v2 soon. Thank you!


With regards,
Pavel Skripkin