Re: [PATCH v3 03/19] powerpc/mm: Fix wrong addr_pfn tracking in compound vmemmap population

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Muchun Song <songmuchun@bytedance.com> writes:

> vmemmap_populate_compound_pages() uses addr_pfn to determine the PFN
> offset within a compound page and to decide whether the current
> vmemmap slot should be populated as a head page mapping or should reuse
> a tail page mapping.
>
> However, addr_pfn is advanced manually in parallel with addr.  The loop
> itself progresses in vmemmap address space, so each PAGE_SIZE step in
> addr covers PAGE_SIZE / sizeof(struct page) struct page slots.  Since
> addr_pfn is compared against nr_pages in data-PFN units, it should
> advance by the same number of PFNs.  The existing manual increments do
> not match that and therefore do not reliably track the PFN
> corresponding to the current addr.
>
> As a result, pfn_offset can be computed from the wrong PFN and the code
> can make the head/tail decision for the wrong compound-page position.
>
> Fix this by deriving addr_pfn directly from the current vmemmap address
> instead of carrying it as loop state.
>
> Fixes: f2b79c0d7968 ("powerpc/book3s64/radix: add support for vmemmap optimization for radix")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Oscar Salvador <osalvador@suse.de>

Thanks for fixing it. I guess this was not caught because section size
on powerpc is 16MB and with 64K pagesize we have 256 pfns to map. The
vmemmap size required for this is 256*sizeof(struct page) = 16KB which
is < 64K (pagesize). So basically we never loop in
vmemmap_populate_compound_page(), because
next = addr+PAGE_SIZE will be > end after the 1st iteration itself.

But I agree this is a bug which needs fixing and it can be easily caught
with 4K pagesize, where we have 4096 pfns to map within a 16MB section.


The change looks good to me. Can we please add stable tag too?
Cc: stable@kernel.org

Also, feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>