Mesa (staging/21.1): winsys/amdgpu: don't read bo->u.slab.entry after pb_slab_free

[GitLab Mirror]

Module: Mesa
Branch: staging/21.1
Commit: f5f9e42e5ef904e14c841e025e376913496cbeb8
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=f5f9e42e5ef904e14c841e025e376913496cbeb8

Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer at amd.com>
Date:   Mon May 17 18:41:26 2021 +0200

winsys/amdgpu: don't read bo->u.slab.entry after pb_slab_free

Otherwise the pb_slabs might be freed by another thread in between.

Valgrind example:

==676841== Invalid read of size 1
==676841==    at 0x6B0A8B3: get_slab_wasted_size (amdgpu_bo.c:659)
==676841==    by 0x6B0AD7D: amdgpu_bo_slab_destroy (amdgpu_bo.c:684)
==676841==    by 0x6ACF94F: pb_destroy (pb_buffer.h:259)
==676841==    by 0x6ACF94F: pb_reference_with_winsys (pb_buffer.h:282)
==676841==    by 0x6ACF94F: radeon_bo_reference (radeon_winsys.h:754)
==676841==    by 0x6ACF94F: si_replace_buffer_storage (si_buffer.c:274)
==676841==    by 0x6957036: tc_call_replace_buffer_storage (u_threaded_context.c:1554)
                            [...]
==676841==    by 0x4ECCDEE: clone (clone.S:95)
==676841==  Address 0x27879945 is 5 bytes inside a block of size 208 free'd
==676841==    at 0x48399AB: free (vg_replace_malloc.c:538)
==676841==    by 0x6B0E8BD: amdgpu_bo_slab_free (amdgpu_bo.c:863)
==676841==    by 0x6B89D4A: pb_slabs_reclaim_locked (pb_slab.c:84)
==676841==    by 0x6B89D4A: pb_slab_alloc (pb_slab.c:130)
==676841==    by 0x6B0EE7F: amdgpu_bo_create (amdgpu_bo.c:1429)

Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/4736
Fixes: 965c6445ad4 ("winsys/amdgpu,radeonsi: add HUD counters for how much memory is wasted by slabs")
Reviewed-by: Marek Olšák <marek.olsak at amd.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/11010>
(cherry picked from commit 1bd64d8cfbc1034593c912c77e1a0403ac9007db)

---

 .pick_status.json                         |  2 +-
 src/gallium/winsys/amdgpu/drm/amdgpu_bo.c | 12 ++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/.pick_status.json b/.pick_status.json
index 37d8be420c9..e06472f5d1f 100644
--- a/.pick_status.json
+++ b/.pick_status.json
@@ -1579,7 +1579,7 @@
         "description": "winsys/amdgpu: don't read bo->u.slab.entry after pb_slab_free",
         "nominated": true,
         "nomination_type": 1,
-        "resolution": 0,
+        "resolution": 1,
         "main_sha": null,
         "because_sha": "965c6445ad419aa49302f75db1d99345708c5aae"
     },
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
index 6cadfaa4313..ba98aa13075 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
@@ -666,22 +666,18 @@ static void amdgpu_bo_slab_destroy(struct radeon_winsys *rws, struct pb_buffer *
 {
    struct amdgpu_winsys *ws = amdgpu_winsys(rws);
    struct amdgpu_winsys_bo *bo = amdgpu_winsys_bo(_buf);
+   struct pb_slabs *slabs;
 
    assert(!bo->bo);
 
-   if (bo->base.usage & RADEON_FLAG_ENCRYPTED)
-      pb_slab_free(get_slabs(ws,
-                             bo->base.size,
-                             RADEON_FLAG_ENCRYPTED), &bo->u.slab.entry);
-   else
-      pb_slab_free(get_slabs(ws,
-                             bo->base.size,
-                             0), &bo->u.slab.entry);
+   slabs = get_slabs(ws, bo->base.size, bo->base.usage & RADEON_FLAG_ENCRYPTED);
 
    if (bo->base.placement & RADEON_DOMAIN_VRAM)
       ws->slab_wasted_vram -= get_slab_wasted_size(ws, bo);
    else
       ws->slab_wasted_gtt -= get_slab_wasted_size(ws, bo);
+
+   pb_slab_free(slabs, &bo->u.slab.entry);
 }
 
 static const struct pb_vtbl amdgpu_winsys_bo_slab_vtbl = {