Re: [RFC 2/2] drm/amdgpu: Wire up DRM memory stats reporting

[sashiko-bot@kernel.org]

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 <tvrtko.ursulin@igalia.com>
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/20260917120444.86471-1-tvrtko.ursulin@igalia.com?part=2