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

[Helge Deller <deller@gmx.de>]

On 9/17/25 00:21, Mina Almasry wrote:
> 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.

Btw, on 32-bit parisc we have PAGE_OFFSET = 0x10000000.
Other architectures seem to have other values than 0xc0000000 too.

Helge