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 <[email protected]>
> ---
> 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 |
