Re: [PATCH v2 2/3] drm_buddy: fix power-of-2 rounding errs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Silent 32-bit truncation of u64 variables in is_power_of_2() checks.
- [Medium] The patch incompletely fixes 32-bit truncation of `u64 size` by leaving `unsigned long pages`, `fls()`, and `BIT()` in the exact same calculation paths.
--

commit c788807ff6acd696c6d8c9760009db4310c73ca2
Author: Jim Cromie <jim.cromie@gmail.com>
Subject: drm_buddy: fix power-of-2 rounding errs

This commit fixes 32-bit truncation of 64-bit sizes when using the standard
roundup_pow_of_two() and rounddown_pow_of_two() macros. It replaces them with
safe 64-bit equivalents to resolve unexpected test successes in DRM-CI on
32-bit systems.

Link: https://lore.kernel.org/all/20260326185413.1205870-40-jim.cromie@gmail.com/

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index a5553fcec28c2..0c432719f8f17 100644
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c

[ ... ]

> @@ -1440,7 +1440,7 @@ int gpu_buddy_alloc_blocks(struct gpu_buddy *mm,
>  
>  	/* Roundup the size to power of 2 */
>  	if (flags & GPU_BUDDY_CONTIGUOUS_ALLOCATION) {
> -		size = roundup_pow_of_two(size);
> +		size = roundup_pow_of_two_u64(size);

[Severity: High]
This is a pre-existing issue, but does is_power_of_2() also suffer from the
exact same silent 32-bit truncation?

Earlier in gpu_buddy_alloc_blocks(), the 64-bit min_block_size is validated:

	if (!is_power_of_2(min_block_size))
		return -EINVAL;

Since is_power_of_2() expects an unsigned long, if a 64-bit value is provided
where the lower 32 bits form a power of two but the upper bits are non-zero
(e.g., 0x100001000), it seems it will be truncated to a valid power of two on
32-bit architectures. This would bypass validation and corrupt the allocator
state.

Other places doing this check on 64-bit variables:
- gpu_buddy_init() validates the 64-bit chunk_size
- gpu_buddy_can_offset_align() validates the 64-bit size

[Severity: Medium]
This is also a pre-existing issue, but does this function incompletely fix
the truncation of size by leaving 32-bit types in the subsequent
calculations?

Just below this hunk in gpu_buddy_alloc_blocks(), pages is calculated:

	unsigned long pages;
	...
	pages = size >> ilog2(mm->chunk_size);
	order = fls(pages) - 1;

Since pages is an unsigned long, it will be silently truncated if the shifted
value exceeds 32 bits on 32-bit architectures.

Additionally, on 64-bit architectures where pages is 64-bit, the fls() macro
takes a 32-bit unsigned int. If pages > 0xFFFFFFFF, fls() only sees the
lower 32 bits, which may be 0, incorrectly setting order to -1. 

Further down in the allocation loop, the BIT() macro is used:

	pages -= BIT(order);

If order >= 32, the BIT() macro will overflow and evaluate incorrectly. Could
these paths lead to incorrect block allocations or memory corruption for very
large sizes?

>  		min_block_size = size;
>  		/*
>  		 * Normalize the requested size to min_block_size for regular allocations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830103321.2042968-1-david@davidgow.net?part=2