[PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Thomas Hellström]

If caching mode change fails due to, for example, OOM we
free the allocated pages in a two-step process. First the pages
for which the caching change has already succeeded. Secondly
the pages for which a caching change did not succeed.

However the second step was incorrectly freeing the pages already
freed in the first step.

Fix.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error path")
Cc: Christian König <christian.koenig@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Huang Rui <ray.huang@amd.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.4+
---
 drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index b62f420a9f96..112438d965ff 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -387,7 +387,7 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
 				enum ttm_caching caching,
 				pgoff_t start_page, pgoff_t end_page)
 {
-	struct page **pages = tt->pages;
+	struct page **pages = &tt->pages[start_page];
 	unsigned int order;
 	pgoff_t i, nr;
 
-- 
2.43.0

✗ Fi.CI.BAT: failure for drm/ttm: Fix an invalid freeing on already freed page in error path

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4349 bytes --]

== Series Details ==

Series: drm/ttm: Fix an invalid freeing on already freed page in error path
URL   : https://patchwork.freedesktop.org/series/130170/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14305 -> Patchwork_130170v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_130170v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_130170v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/index.html

Participating hosts (40 -> 37)
------------------------------

  Missing    (3): bat-mtlp-8 fi-snb-2520m fi-bsw-n3050 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_130170v1:

### IGT changes ###

#### Possible regressions ####

  * igt@dmabuf@all-tests@dma_fence:
    - fi-kbl-guc:         [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14305/fi-kbl-guc/igt@dmabuf@all-tests@dma_fence.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/fi-kbl-guc/igt@dmabuf@all-tests@dma_fence.html

  * igt@dmabuf@all-tests@sanitycheck:
    - fi-kbl-guc:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14305/fi-kbl-guc/igt@dmabuf@all-tests@sanitycheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/fi-kbl-guc/igt@dmabuf@all-tests@sanitycheck.html

  
Known issues
------------

  Here are the changes found in Patchwork_130170v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@gem_exec_create@basic@smem:
    - {bat-arls-1}:       [DMESG-WARN][5] ([i915#10194]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14305/bat-arls-1/igt@gem_exec_create@basic@smem.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/bat-arls-1/igt@gem_exec_create@basic@smem.html

  * igt@vgem_basic@create:
    - {bat-arls-2}:       [FAIL][7] ([i915#10294]) -> [PASS][8] +4 other tests pass
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14305/bat-arls-2/igt@vgem_basic@create.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/bat-arls-2/igt@vgem_basic@create.html

  * igt@vgem_basic@dmabuf-mmap:
    - {bat-arls-2}:       [INCOMPLETE][9] ([i915#10294]) -> [PASS][10] +6 other tests pass
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14305/bat-arls-2/igt@vgem_basic@dmabuf-mmap.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/bat-arls-2/igt@vgem_basic@dmabuf-mmap.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
  [i915#10215]: https://gitlab.freedesktop.org/drm/intel/issues/10215
  [i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
  [i915#10294]: https://gitlab.freedesktop.org/drm/intel/issues/10294
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077


Build changes
-------------

  * Linux: CI_DRM_14305 -> Patchwork_130170v1

  CI-20190529: 20190529
  CI_DRM_14305: 4b8a238dee9c18201f3652695414587cd2ef6d8f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7718: 40e8b9122853f455c84afcfa56469a6bc9a0d564 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_130170v1: 4b8a238dee9c18201f3652695414587cd2ef6d8f @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

fd3077a5c869 drm/ttm: Fix an invalid freeing on already freed page in error path

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130170v1/index.html

[-- Attachment #2: Type: text/html, Size: 4612 bytes --]

Re: [PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Matthew Auld]

On 21/02/2024 07:33, Thomas Hellström wrote:
> If caching mode change fails due to, for example, OOM we
> free the allocated pages in a two-step process. First the pages
> for which the caching change has already succeeded. Secondly
> the pages for which a caching change did not succeed.
> 
> However the second step was incorrectly freeing the pages already
> freed in the first step.
> 
> Fix.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error path")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.4+
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Re: [PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Christian König]

Am 21.02.24 um 08:33 schrieb Thomas Hellström:
> If caching mode change fails due to, for example, OOM we
> free the allocated pages in a two-step process. First the pages
> for which the caching change has already succeeded. Secondly
> the pages for which a caching change did not succeed.
>
> However the second step was incorrectly freeing the pages already
> freed in the first step.
>
> Fix.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error path")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.4+

You don't know how much time I've spend staring at this line to find the 
bug in it and haven't seen it. Got bug reports about that for month as well.

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index b62f420a9f96..112438d965ff 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -387,7 +387,7 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
>   				enum ttm_caching caching,
>   				pgoff_t start_page, pgoff_t end_page)
>   {
> -	struct page **pages = tt->pages;
> +	struct page **pages = &tt->pages[start_page];
>   	unsigned int order;
>   	pgoff_t i, nr;
>   

Re: [PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Thomas Hellström]

On Wed, 2024-02-21 at 11:26 +0100, Christian König wrote:
> Am 21.02.24 um 08:33 schrieb Thomas Hellström:
> > If caching mode change fails due to, for example, OOM we
> > free the allocated pages in a two-step process. First the pages
> > for which the caching change has already succeeded. Secondly
> > the pages for which a caching change did not succeed.
> > 
> > However the second step was incorrectly freeing the pages already
> > freed in the first step.
> > 
> > Fix.
> > 
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error path")
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Christian Koenig <christian.koenig@amd.com>
> > Cc: Huang Rui <ray.huang@amd.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v6.4+
> 
> You don't know how much time I've spend staring at this line to find
> the 
> bug in it and haven't seen it. Got bug reports about that for month
> as well.


Yeah, sorry about that. We should probably have Kunit tests exercising
OOM in the pool code involving WC pages.

I'll push this to drm-misc-next.

/Thomas

> 
> Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> > ---
> >   drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> > b/drivers/gpu/drm/ttm/ttm_pool.c
> > index b62f420a9f96..112438d965ff 100644
> > --- a/drivers/gpu/drm/ttm/ttm_pool.c
> > +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> > @@ -387,7 +387,7 @@ static void ttm_pool_free_range(struct ttm_pool
> > *pool, struct ttm_tt *tt,
> >   				enum ttm_caching caching,
> >   				pgoff_t start_page, pgoff_t
> > end_page)
> >   {
> > -	struct page **pages = tt->pages;
> > +	struct page **pages = &tt->pages[start_page];
> >   	unsigned int order;
> >   	pgoff_t i, nr;
> >   
> 

Re: [PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Thomas Hellström]

On Thu, 2024-02-22 at 08:34 +0100, Thomas Hellström wrote:
> On Wed, 2024-02-21 at 11:26 +0100, Christian König wrote:
> > Am 21.02.24 um 08:33 schrieb Thomas Hellström:
> > > If caching mode change fails due to, for example, OOM we
> > > free the allocated pages in a two-step process. First the pages
> > > for which the caching change has already succeeded. Secondly
> > > the pages for which a caching change did not succeed.
> > > 
> > > However the second step was incorrectly freeing the pages already
> > > freed in the first step.
> > > 
> > > Fix.
> > > 
> > > Signed-off-by: Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>
> > > Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error
> > > path")
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Christian Koenig <christian.koenig@amd.com>
> > > Cc: Huang Rui <ray.huang@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v6.4+
> > 
> > You don't know how much time I've spend staring at this line to
> > find
> > the 
> > bug in it and haven't seen it. Got bug reports about that for month
> > as well.
> 
> 
> Yeah, sorry about that. We should probably have Kunit tests
> exercising
> OOM in the pool code involving WC pages.
> 
> I'll push this to drm-misc-next.

drm-misc-fixes..
/Thomas


> 
> /Thomas
> 
> > 
> > Reviewed-by: Christian König <christian.koenig@amd.com>
> > 
> > > ---
> > >   drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> > > b/drivers/gpu/drm/ttm/ttm_pool.c
> > > index b62f420a9f96..112438d965ff 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_pool.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> > > @@ -387,7 +387,7 @@ static void ttm_pool_free_range(struct
> > > ttm_pool
> > > *pool, struct ttm_tt *tt,
> > >   				enum ttm_caching caching,
> > >   				pgoff_t start_page, pgoff_t
> > > end_page)
> > >   {
> > > -	struct page **pages = tt->pages;
> > > +	struct page **pages = &tt->pages[start_page];
> > >   	unsigned int order;
> > >   	pgoff_t i, nr;
> > >   
> > 
> 

Re: [PATCH] drm/ttm: Fix an invalid freeing on already freed page in error path

[Christian König]

Am 22.02.24 um 08:34 schrieb Thomas Hellström:
> On Wed, 2024-02-21 at 11:26 +0100, Christian König wrote:
>> Am 21.02.24 um 08:33 schrieb Thomas Hellström:
>>> If caching mode change fails due to, for example, OOM we
>>> free the allocated pages in a two-step process. First the pages
>>> for which the caching change has already succeeded. Secondly
>>> the pages for which a caching change did not succeed.
>>>
>>> However the second step was incorrectly freeing the pages already
>>> freed in the first step.
>>>
>>> Fix.
>>>
>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Fixes: 379989e7cbdc ("drm/ttm/pool: Fix ttm_pool_alloc error path")
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Dave Airlie <airlied@redhat.com>
>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>> Cc: Huang Rui <ray.huang@amd.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: <stable@vger.kernel.org> # v6.4+
>> You don't know how much time I've spend staring at this line to find
>> the
>> bug in it and haven't seen it. Got bug reports about that for month
>> as well.
>
> Yeah, sorry about that. We should probably have Kunit tests exercising
> OOM in the pool code involving WC pages.
>
> I'll push this to drm-misc-next.

drm-misc-fixes please! That needs to be backported ASAP.

Need to dig up the bug report for this again.

Thanks,
Christian.

>
> /Thomas
>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>>> ---
>>>    drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>> index b62f420a9f96..112438d965ff 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>> @@ -387,7 +387,7 @@ static void ttm_pool_free_range(struct ttm_pool
>>> *pool, struct ttm_tt *tt,
>>>    				enum ttm_caching caching,
>>>    				pgoff_t start_page, pgoff_t
>>> end_page)
>>>    {
>>> -	struct page **pages = tt->pages;
>>> +	struct page **pages = &tt->pages[start_page];
>>>    	unsigned int order;
>>>    	pgoff_t i, nr;
>>>