Re: [PATCH v4 2/4] drm/xe/xe_migrate: Align MEM_COPY pitch with destination address

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: integer overflow and assertion failure with 64K pages
--

commit c393bea53ab90ffca1212c4a87ea68cccbab7104
Author: Christoph Manszewski <c.manszewski@gmail.com>

drm/xe/xe_migrate: Align MEM_COPY pitch with destination address

MEM_COPY_CMD can corrupt memory when MATRIX_COPY mode uses a pitch that
is not aligned to the destination address. This commit fixes it by
incorporating the destination address into the pitch selection so the
chosen pitch is aligned to both the copy length and the destination address.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 3711d4542ad3d..4b995c93fcea9 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -2187,17 +2187,18 @@ static bool xe_migrate_vram_use_pde(struct drm_pagemap_addr *sram_addr,
>  #define XE_CACHELINE_BYTES	64ull
>  #define XE_CACHELINE_MASK	(XE_CACHELINE_BYTES - 1)
>  
> -static u32 xe_migrate_copy_pitch(struct xe_device *xe, u32 len)
> +static u32 xe_migrate_copy_pitch(struct xe_device *xe, u32 len, u64 dst_addr)
>  {
> +	u64 align_check = len | dst_addr;
>  	u32 pitch;
>  
> -	if (IS_ALIGNED(len, PAGE_SIZE))
> +	if (IS_ALIGNED(align_check, PAGE_SIZE))
>  		pitch = PAGE_SIZE;

[Severity: Medium]
Could returning PAGE_SIZE here exceed the hardware's U16_MAX limit when 
the kernel is built with 64K pages, such as on ARM64 under COMPILE_TEST?

If PAGE_SIZE evaluates to 65536, this pitch will be greater than 65535, 
which would trigger the xe_gt_assert(gt, pitch <= U16_MAX) inside 
emit_mem_copy().

[ ... ]

> @@ -2535,7 +2540,9 @@ int xe_migrate_access_memory(struct xe_migrate *m, struct xe_bo *bo,
>  		else
>  			current_bytes = min_t(int, bytes_left, cursor.size);
>  
> -		pitch = xe_migrate_copy_pitch(xe, current_bytes);
> +		pitch = xe_migrate_copy_pitch(xe, current_bytes,
> +					      write ? vram_addr :
> +					      (unsigned long)buf & ~PAGE_MASK);
>  		if (xe->info.has_mem_copy_instr)
>  			current_bytes = min_t(int, current_bytes, U16_MAX * pitch);

[Severity: Medium]
When pitch is 65536 due to 64K pages, does U16_MAX * pitch overflow here?

U16_MAX * 65536 evaluates to 4294901760. The min_t(int, ...) macro casts
this value to a signed 32-bit int, resulting in -65536. 

Since current_bytes becomes negative, and is later passed to xe_migrate_vram() 
as an unsigned long len, could this result in a massive length that triggers 
memory corruption or OOM panics during KUnit tests or CI systems?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014134634.582314-1-jan.maslak@intel.com?part=2