Re: [PATCH][RESEND][RFC] Fix 32-bit boot failure due inaccurate page_pool_page_is_pp()

[Mina Almasry <almasrymina@google.com>]

On Tue, Sep 16, 2025 at 2:27 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Mina Almasry <almasrymina@google.com> writes:
>
> > On Mon, Sep 15, 2025 at 6:08 AM Helge Deller <deller@gmx.de> wrote:
> >>
> >> On 9/15/25 13:44, Toke Høiland-Jørgensen wrote:
> >> > Helge Deller <deller@kernel.org> writes:
> >> >
> >> >> Commit ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when
> >> >> destroying the pool") changed PP_MAGIC_MASK from 0xFFFFFFFC to 0xc000007c on
> >> >> 32-bit platforms.
> >> >>
> >> >> The function page_pool_page_is_pp() uses PP_MAGIC_MASK to identify page pool
> >> >> pages, but the remaining bits are not sufficient to unambiguously identify
> >> >> such pages any longer.
> >> >
> >> > Why not? What values end up in pp_magic that are mistaken for the
> >> > pp_signature?
> >>
> >> As I wrote, PP_MAGIC_MASK changed from 0xFFFFFFFC to 0xc000007c.
> >> And we have PP_SIGNATURE == 0x40  (since POISON_POINTER_DELTA is zero on 32-bit platforms).
> >> That means, that before page_pool_page_is_pp() could clearly identify such pages,
> >> as the (value & 0xFFFFFFFC) == 0x40.
> >> So, basically only the 0x40 value indicated a PP page.
> >>
> >> Now with the mask a whole bunch of pointers suddenly qualify as being a pp page,
> >> just showing a few examples:
> >> 0x01111040
> >> 0x082330C0
> >> 0x03264040
> >> 0x0ad686c0 ....
> >>
> >> For me it crashes immediately at bootup when memblocked pages are handed
> >> over to become normal pages.
> >>
> >
> > I tried to take a look to double check here and AFAICT Helge is correct.
> >
> > Before the breaking patch with PP_MAGIC_MASK==0xFFFFFFFC, basically
> > 0x40 is the only pointer that may be mistaken as a valid pp_magic.
> > AFAICT each bit we 0 in the PP_MAGIC_MASK (aside from the 3 least
> > significant bits), doubles the number of pointers that can be mistaken
> > for pp_magic. So with 0xFFFFFFFC, only one value (0x40) can be
> > mistaken as a valid pp_magic, with  0xc000007c AFAICT 2^22 values can
> > be mistaken as pp_magic?
> >
> > I don't know that there is any bits we can take away from
> > PP_MAGIC_MASK I think? As each bit doubles the probablity :(
> >
> > I would usually say we can check the 3 least significant bits to tell
> > if pp_magic is a pointer or not, but pp_magic is unioned with
> > page->lru I believe which will use those bits.
>
> So if the pointers stored in the same field can be any arbitrary value,
> you are quite right, there is no safe value. The critical assumption in
> the bit stuffing scheme is that the pointers stored in the field will
> always be above PAGE_OFFSET, and that PAGE_OFFSET has one (or both) of
> the two top-most bits set (that is what the VMSPLIT reference in the
> comment above the PP_DMA_INDEX_SHIFT definition is alluding to).
>

I see... but where does the 'PAGE_OFFSET has one (or both) of the two
top-most bits set)' assumption come from? Is it from this code?

/*
 * PAGE_OFFSET -- the first address of the first page of memory.
 * When not using MMU this corresponds to the first free page in
 * physical memory (aligned on a page boundary).
 */
#ifdef CONFIG_MMU
#ifdef CONFIG_64BIT
....
#else
#define PAGE_OFFSET _AC(0xc0000000, UL)
#endif /* CONFIG_64BIT */
#else
#define PAGE_OFFSET ((unsigned long)phys_ram_base)
#endif /* CONFIG_MMU */

It looks like with !CONFIG_MMU we use phys_ram_base and I'm unable to
confirm that all the values of this have the first 2 bits set. I
wonder if his setup is !CONFIG_MMU indeed.

It also looks like pp_magic is also union'd with __folio_index in
struct page, and it looks like the data there is sometimes used as a
pointer and sometimes not.

-- 
Thanks,
Mina