Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] amdgpu: data race and OOB read in amdgpu_drm_memory_info lazy init - [High] amdgpu: embedded flexible array member causes UB and FORTIFY panics - [Medium] amdgpu: array index mismatch in fdinfo mixes up memory stats - [Medium] amdgpu: unprotected concurrent updates to region->used_mb cause data race --
commit 0821285be07eaef79323cee5db70d046b94e519f Author: Tvrtko Ursulin <[email protected]> drm/amdgpu: Wire up DRM memory stats reporting This commit wires up the driver agnostic DRM memory reporting for the VRAM and GTT memory regions. It keeps the persistent storage at the device level and exports a helper for standardized region names. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 79b69d74eb2e5..8c00f76cfc7b6 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -965,6 +965,9 @@ struct amdgpu_device { > > struct amdgpu_uma_carveout_info uma_info; > > + struct drm_memory_info memory_info; > + struct drm_memory_region_info __memory_region_info[2]; /* Storage for > memory_info */ > + [Severity: High] Does embedding a structure with a flexible array member in the middle of another struct cause undefined behavior and compiler warnings? struct drm_memory_info ends with a flexible array member (region[]). Embedding it here before __memory_region_info[2] means the compiler evaluates the size of memory_info as only its fixed members (4 bytes). When writing to info->region[i] later, FORTIFY/UBSAN checks may panic as the access exceeds the recognized object bounds. [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > index b349bb3676d5f..3bbd053cd9f7f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c [ ... ] > @@ -85,15 +104,12 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct > drm_file *file) > > drm_printf(p, "pasid:\t%u\n", fpriv->vm.pasid); > > - for (i = 0; i < ARRAY_SIZE(pl_name); i++) { > - if (!pl_name[i]) > - continue; > - > + for (i = 0; i < ARRAY_SIZE(regions); i++) { > drm_print_memory_stats(p, > - &stats[i].drm, > + &stats[i].drm, > DRM_GEM_OBJECT_RESIDENT | > DRM_GEM_OBJECT_PURGEABLE, > - pl_name[i]); > + amdgpu_ttm_pl_to_name(regions[i])); > } [Severity: Medium] Is there an array index mismatch here? The stats array is populated by amdgpu_vm_get_memory() which maps placement IDs directly to indices (e.g., TTM_PL_SYSTEM is 0, TTM_PL_VRAM is 2). However, the loop iterates using a sequential index i (0 to 7) and fetches &stats[i].drm, but labels it with amdgpu_ttm_pl_to_name(regions[i]). For i=0, regions[0] is TTM_PL_VRAM (2), meaning it fetches stats[0] (CPU memory) but labels it as VRAM. Should the fetch use &stats[regions[i]].drm to correctly match the data with the placement ID? [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 0ca9b1669e0fe..d6da850eadc97 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -3026,3 +3026,40 @@ void amdgpu_ttm_debugfs_init(struct amdgpu_device > *adev) > > #endif > } > + > +const struct drm_memory_info *amdgpu_drm_memory_info(struct drm_device *drm) > +{ > + struct amdgpu_device *adev = drm_to_adev(drm); > + struct drm_memory_info *info = &adev->memory_info; [ ... ] > + if (!info->num_regions) { > + for (i = 0; i < ARRAY_SIZE(regions); i++) { > + region = &info->region[i]; > + strscpy(region->name, > + amdgpu_ttm_pl_to_name(regions[i]), > + sizeof(region->name)); > + man = ttm_manager_type(&adev->mman.bdev, regions[i]); > + region->total_mb = man->size >> 20; > + info->num_regions++; > + } > + } [Severity: High] Is this lockless lazy initialization safe against concurrent access? If multiple threads query DRM memory info concurrently via fdinfo, they can simultaneously enter the if (!info->num_regions) block. Both threads could execute info->num_regions++ in a loop. Since the backing array (__memory_region_info) only has 2 elements, concurrent increments can push num_regions to 4. When the DRM core later iterates up to num_regions, will this lead to out-of-bounds memory reads? > + > + for (i = 0; i < ARRAY_SIZE(regions); i++) { > + region = &info->region[i]; > + man = ttm_manager_type(&adev->mman.bdev, regions[i]); > + region->used_mb = ttm_resource_manager_usage(man) >> 20; > + } > + > + return &adev->memory_info; > +} [Severity: Medium] Can unprotected concurrent updates to region->used_mb cause data races? Because amdgpu_drm_memory_info() updates region->used_mb directly within the globally shared adev->memory_info structure without synchronization, concurrent readers and writers might race, leading to torn reads or KCSAN splats. Would it be safer to allocate a drm_memory_info struct per-query instead of caching it globally? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
