Re: [PATCH] ARM: mmu: fix flush_cacheable_pages off-by-one touching guard page

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Stephano,

Thanks for your patch!

On 8/22/26 4:06 PM, Stephano Cetola wrote:
> flush_cacheable_pages() accumulates contiguous cacheable page ranges and
> tracks flush_end as the exclusive end of each range: the address of the
> first page BEYOND the last cacheable block, which equals the start
> address of the next block. The flush_end == addr extension test relies
> on this invariant holding everywhere flush_end is assigned.
> 
> Two places break the invariant.
> 
> First: when a non-cacheable page (e.g. the stack guard page) creates a
> gap in the middle of a flush region followed by more cacheable pages,
> dma_flush_range_end(flush_start, flush_end) is called just before
> starting a new range.
> 
> Second: flush_end is clamped against region_end via
> min(flush_end + block_size, region_end), in both the range-extension
> branch and right after starting a new range. region_end is computed as
> PAGE_ALIGN(region_start + size) - 1, an inclusive last-address value.
> 
> Fix both by keeping flush_end consistently exclusive: clamp against
> region_end + 1 (not region_end) at both extension sites, and subtract
> 1 to convert to the inclusive end dma_flush_range_end expects at both
> call sites.
> 
> Observed on RK3588S (Radxa CM5) during boot-from NVMe bring-up.
> 
> Fixes: 04bfef82e33e ("ARM: mmu64: fix benign off-by-one in flush_cacheable_pages")
> Signed-off-by: Stephano Cetola <stephano@cetola.net>
> ---
>  arch/arm/cpu/flush_cacheable_pages.h | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/cpu/flush_cacheable_pages.h b/arch/arm/cpu/flush_cacheable_pages.h
> index a5c54864d4..25990282ca 100644
> --- a/arch/arm/cpu/flush_cacheable_pages.h
> +++ b/arch/arm/cpu/flush_cacheable_pages.h
> @@ -55,26 +55,23 @@ static void flush_cacheable_pages(void *start, size_t size)
>  
>  		if (flush_end == addr) {
>  			/*
> -			 * While it's safe to flush the whole block_size,
> -			 * it's unnecessary time waste to go beyond region_end.
> +			 * region_end is inclusive, flush_end exclusive:
> +			 * clamp to region_end + 1.
>  			 */

I agree there is a bug here, but making flush_end exclusive doesn't
sound like the best remedy to me, exactly because these + 1's and - 1's
are so error-prone.

Also it seems the code has problems if a cached region reaches the end
of address space, which can happen on some 32-bit ARM SoCs, like the i.MX6Q.

I am working on a revised patch that I will share within the week.

Thanks,
Ahmad

> -			flush_end = min(flush_end + block_size, region_end);
> +			flush_end = min(flush_end + block_size, region_end + 1);
>  			continue;
>  		}
>  
> -		/*
> -		 * We don't have a previous contiguous flush area to append to.
> -		 * If we recorded any area before, let's flush it now
> -		 */
> +		/* flush_end is exclusive; dma_flush_range_end() wants an inclusive end. */
>  		if (flush_start != ~0UL)
> -			dma_flush_range_end(flush_start, flush_end);
> +			dma_flush_range_end(flush_start, flush_end - 1);
>  
>  		/* and start the new contiguous flush area with this page */
>  		flush_start = addr;
> -		flush_end = min(flush_start + block_size, region_end);
> +		flush_end = min(flush_start + block_size, region_end + 1);
>  	}
>  
>  	/* The previous loop won't flush the last cached range, so do it here */
>  	if (flush_start != ~0UL)
> -		dma_flush_range_end(flush_start, flush_end);
> +		dma_flush_range_end(flush_start, flush_end - 1);
>  }
> 
> ---
> base-commit: 9bc1a26592a59919d2ee8c60c273d87d3d9f2e81
> change-id: 20260821-send-mmu-flush-guard-fde994a1b433
> 
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |