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

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

Pre-existing issues:
- [High] The `pages` variable is declared as `unsigned long`, and the `fls()` and `BIT()` macros are used, leading to silent truncation and potential infinite loops or crashes for large allocations.
- [High] Calling the `is_power_of_2()` macro with 64-bit variables (`min_block_size` and `size`) silently truncates them to 32 bits on 32-bit architectures, breaking allocator validation.
--

--- Patch [2]: [PATCH 2/2] drm_buddy: fix power-of-2 rounding errs ---
commit 6e9e2fbe5490ccf9c40ea5dcdf1e897c00f63b30
Author: Jim Cromie <[email protected]>

drm_buddy: fix power-of-2 rounding errs

This patch replaces standard roundup_pow_of_two() and rounddown_pow_of_two()
macros with their 64-bit equivalents in the GPU buddy allocator. This fixes
silent truncation of 64-bit allocation sizes on 32-bit architectures, which
was causing tests to fail.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index a5553fcec28c2..64f41f1fe9089 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_two64(size);
>  		min_block_size = size;
>  		/*
>  		 * Normalize the requested size to min_block_size for regular allocations.

[Severity: High]
This is a pre-existing issue, but since you are fixing 32-bit truncation
errors in this function, are there similar truncation issues with the pages
variable later in gpu_buddy_alloc_blocks()?

The pages variable is declared as unsigned long:

	unsigned long pages;

And later initialized and used with fls() and BIT():

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

Since unsigned long is 32-bit on 32-bit architectures, pages truncates
for sizes > 16TB (assuming 4KB chunks).

Furthermore, fls() takes a 32-bit integer, so it will truncate pages on 64-bit
systems.

BIT(order) also uses 1UL, which overflows on 32-bit architectures if order >= 32.

Can this lead to infinite loops or crashes for large allocations?

Similar truncation might also occur in gpu_buddy_offset_aligned_allocation(),
which also uses unsigned long pages.

[Severity: High]
This is another pre-existing issue, but does is_power_of_2() also silently
truncate 64-bit variables on 32-bit architectures?

Earlier in gpu_buddy_alloc_blocks():

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

The is_power_of_2() macro casts its argument to unsigned long.

If a 64-bit value like 4GB + 4KB is passed, it truncates to 4KB on 32-bit
systems, which is a valid power of two.

This bypasses the validation and allows non-power-of-two sizes to reach
round_up(), which could corrupt sizes and allocator memory.

A similar check exists in gpu_buddy_can_offset_align():

	return size < min_block_size && is_power_of_2(size);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2