Fix 32-bit boot failure due inaccurate page_pool_page_is_pp()

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.

So page_pool_page_is_pp() now sometimes wrongly reports pages as page pool pages and as such triggers a kernel BUG as it believes it found a page pool leak.

There are patches upcoming where page_pool_page_is_pp() will not depend on PP_MAGIC_MASK and instead use page flags to identify page pool pages. Until those patches are merged, the easiest temporary fix is to disable the check on 32-bit platforms.

Cc Jesper Dangaard Brouer <[email protected]>

Cc Ilias Apalodimas <[email protected]>

Cc David Hildenbrand <[email protected]>

Acked-by David Hildenbrand <[email protected]>

Cc Toke Høiland-Jørgensen <[email protected]>

Cc Linux Memory Management List <[email protected]>

Cc [email protected]

Cc Linux parisc List <[email protected]>

Cc <[email protected]> # v6.15+

Signed-off-by Helge Deller <[email protected]>

Link https://www.spinics.net/lists/kernel/msg5849623.html

Fixes ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")

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.

So page_pool_page_is_pp() now sometimes wrongly reports pages as page pool pages and as such triggers a kernel BUG as it believes it found a page pool leak.

There are patches upcoming where page_pool_page_is_pp() will not depend on PP_MAGIC_MASK and instead use page flags to identify page pool pages. Until those patches are merged, the easiest temporary fix is to disable the check on 32-bit platforms.

Cc Jesper Dangaard Brouer <[email protected]>

Cc Ilias Apalodimas <[email protected]>

Cc David Hildenbrand <[email protected]>

Acked-by David Hildenbrand <[email protected]>

Cc Toke Høiland-Jørgensen <[email protected]>

Cc Linux Memory Management List <[email protected]>

Cc [email protected]

Cc Linux parisc List <[email protected]>

Cc <[email protected]> # v6.15+

Signed-off-by Helge Deller <[email protected]>

Link https://www.spinics.net/lists/kernel/msg5849623.html

Fixes ee62ce7a1d90 ("page_pool: Track DMA-mapped pages and unmap them when destroying the pool")

include/linux/mm.h +0 -0

index 1ae97a0b8ec7..f3822ae70a81 100644

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.

So page_pool_page_is_pp() now sometimes wrongly reports pages as page pool pages and as such triggers a kernel BUG as it believes it found a page pool leak.

There are patches upcoming where page_pool_page_is_pp() will not depend on PP_MAGIC_MASK and instead use page flags to identify page pool pages. Until those patches are merged, the easiest temporary fix is to disable the check on 32-bit platforms.

Quoted 4 lines · Helge Deller

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.

Quoted 3 lines

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 indicated by the comment above the definition of the PP_DMA_INDEX_* definitions, I did put some care into ensuring that the values would not get mistaken, see specifically this:

(...) On arches where POISON_POINTER_DELTA is
* 0, we make sure that we leave the six topmost bits empty, as that guarantees
* we won't mistake a valid kernel pointer for a value we set, regardless of the
* VMSPLIT setting.

So if that does not hold, I'd like to understand why not?

Quoted 4 lines

So page_pool_page_is_pp() now sometimes wrongly reports pages as page pool pages and as such triggers a kernel BUG as it believes it found a page pool leak.

Quoted 4 lines

There are patches upcoming where page_pool_page_is_pp() will not depend on PP_MAGIC_MASK and instead use page flags to identify page pool pages. Until those patches are merged, the easiest temporary fix is to disable the check on 32-bit platforms.

As Jesper pointed out, we also use this check internally in the network stack, and the patch as proposed will at least trigger the DEBUG_NET_WARN_ON_ONCE() in include/net/netmem.h.

I think a better
solution would be, as Jesper also alludes to, simply adding more bits to the mask. For instance, the patch below reserves (somewhat arbitrarily) six bits instead of two, changing the mask to 0xfc00007c; would that work?

(quoted diff) +0 -1

-Toke

include/linux/mm.h +4 -4

index 1ae97a0b8ec7..17cb8157ba08 100644
@@ -4159,12 +4159,12 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
   * since this value becomes part of PP_SIGNATURE; meaning we can just use the
   * space between the PP_SIGNATURE value (without POISON_POINTER_DELTA), and the
   * lowest bits of POISON_POINTER_DELTA. On arches where POISON_POINTER_DELTA is
- * 0, we make sure that we leave the two topmost bits empty, as that guarantees
+ * 0, we make sure that we leave the six topmost bits empty, as that guarantees
   * we won't mistake a valid kernel pointer for a value we set, regardless of the
   * VMSPLIT setting.
   *
   * Altogether, this means that the number of bits available is constrained by
- * the size of an unsigned long (at the upper end, subtracting two bits per the
+ * the size of an unsigned long (at the upper end, subtracting six bits per the
   * above), and the definition of PP_SIGNATURE (with or without
   * POISON_POINTER_DELTA).
   */

@@ -4175,8 +4175,8 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
   */
  #define PP_DMA_INDEX_BITS MIN(32, __ffs(POISON_POINTER_DELTA) - PP_DMA_INDEX_SHIFT)
  #else
-/* Always leave out the topmost two; see above. */
-#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 2)
+/* Always leave out the topmost six; see above. */
+#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 6)
  #endif
 
  #define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS + PP_DMA_INDEX_SHIFT - 1, \

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 10 lines

As indicated by the comment above the definition of the PP_DMA_INDEX_* definitions, I did put some care into ensuring that the values would not get mistaken, see specifically this:

(...) On arches where POISON_POINTER_DELTA is
* 0, we make sure that we leave the six topmost bits empty, as that guarantees
* we won't mistake a valid kernel pointer for a value we set, regardless of the
* VMSPLIT setting.

So if that does not hold, I'd like to understand why not?

Because on 32-bit arches POISON_POINTER_DELTA is zero, and as such you basically can't take away any of the remaining low 32 (30) bits.

Quoted 9 lines

Quoted 8 lines

So page_pool_page_is_pp() now sometimes wrongly reports pages as page pool pages and as such triggers a kernel BUG as it believes it found a page pool leak.

There are patches upcoming where page_pool_page_is_pp() will not depend on PP_MAGIC_MASK and instead use page flags to identify page pool pages. Until those patches are merged, the easiest temporary fix is to disable the check on 32-bit platforms.

Quoted 3 lines

As Jesper pointed out, we also use this check internally in the network stack, and the patch as proposed will at least trigger the DEBUG_NET_WARN_ON_ONCE() in include/net/netmem.h.

Interestingly it did not triggered this warning for me. Need to look into this.

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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.

AFAICT, only proper resolution I see is a revert of the breaking patch
+ reland after we can make pp a page-flag and deprecate using pp_magic. Sorry about that. Thoughts Toke? Anything better you can think of here?

Quoted 43 lines · Mina Almasry

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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?

Quoted 6 lines

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).

Quoted 53 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Mina Almasry

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 53 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Mina Almasry

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 62 lines · Mina Almasry

Quoted 53 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Mina Almasry

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 7 lines

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).

Quoted 2 lines

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?

Well, from me grepping through the code and trying to make sense of all the different cases of the preprocessor and config directives across architectures. Seems I did not quite succeed :/

Quoted 15 lines

/*
* 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 */

Quoted 3 lines

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.

Right, that's certainly one thing I missed. As was the parisc arch thing, as Helge followed up with. Ugh :/

Quoted 3 lines

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.

Not according to my pahole:

[...]
union {
long unsigned int __folio_index; /* 32 8 */
[...]
struct {
long unsigned int pp_magic; /* 8 8 */

So I think we're good with this, no?

So given the above, we could do something equivalent to this, I think?

include/linux/mm.h +0 -0

index 1ae97a0b8ec7..615aaa19c60c 100644

include/linux/mm.h +3 -0

+#else
+#define PP_DMA_INDEX_BITS 0
+#endif /* PAGE_OFFSET > PP_SIGNATURE */
 #endif
 
 #define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS +  PP_DMA_INDEX_SHIFT - 1, \

Except that it won't work in this form as-is because PAGE_OFFSET is not always a constant (see the #define PAGE_OFFSET ((unsigned long)phys_ram_base) that your quoted above), so we'll have to turn it into an inline function or something.

I'm not sure adding this extra complexity is really worth it, or if we should just go with the '#define PP_DMA_INDEX_BITS 0' when POISON_POINTER_DELTA is unset and leave it at that for the temporary workaround. WDYT?

Quoted 116 lines · Toke Høiland-Jørgensen

Quoted 62 lines · Mina Almasry

Quoted 53 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Mina Almasry

Quoted 28 lines · Helge Deller

Quoted 6 lines · Toke Høiland-Jørgensen

Quoted 4 lines · Helge Deller

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.

Quoted 6 lines

Quoted 3 lines

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 ....

Quoted 3 lines

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?

Quoted 6 lines

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.

Quoted 7 lines

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).

Quoted 2 lines

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?

Well, from me grepping through the code and trying to make sense of all the different cases of the preprocessor and config directives across architectures. Seems I did not quite succeed :/

Quoted 15 lines

/*
* 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 */

Quoted 3 lines

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.

Right, that's certainly one thing I missed. As was the parisc arch thing, as Helge followed up with. Ugh :/

Quoted 3 lines

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.

Not according to my pahole:

[...]
union {
long unsigned int __folio_index; /* 32 8 */
[...]
struct {
long unsigned int pp_magic; /* 8 8 */

So I think we're good with this, no?

So given the above, we could do something equivalent to this, I think?

include/linux/mm.h +0 -0

index 1ae97a0b8ec7..615aaa19c60c 100644

include/linux/mm.h int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status); · line 4180

Quoted 7 lines

include/linux/mm.h +2 -1

@@ -4175,8 +4175,12 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
  */
 #define PP_DMA_INDEX_BITS MIN(32, __ffs(POISON_POINTER_DELTA) - PP_DMA_INDEX_SHIFT)
 #else
+#if PAGE_OFFSET > PP_SIGNATURE
 /* Always leave out the topmost two; see above. */
-#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 2)
+#define PP_DMA_INDEX_BITS MIN(32, __fls(PAGE_OFFSET) - PP_DMA_INDEX_SHIFT - 1)

Shouldn't have this been:

#define PP_DMA_INDEX_BITS MIN(32, __ffs(PAGE_OFFSET) - PP_DMA_INDEX_SHIFT)

I.e. you're trying to use the space between the least significant bit set in PAGE_OFFSET and the most significant bit set in PP_SIGNATURE. Hmm. I'm not sure I understand this, I may be reading wrong.

Quoted 13 lines

include/linux/mm.h +3 -0

+#else
+#define PP_DMA_INDEX_BITS 0
+#endif /* PAGE_OFFSET > PP_SIGNATURE */
 #endif
 
 #define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS +  PP_DMA_INDEX_SHIFT - 1, \

Except that it won't work in this form as-is because PAGE_OFFSET is not always a constant (see the #define PAGE_OFFSET ((unsigned long)phys_ram_base) that your quoted above), so we'll have to turn it into an inline function or something.

Quoted 5 lines

I'm not sure adding this extra complexity is really worth it, or if we should just go with the '#define PP_DMA_INDEX_BITS 0' when POISON_POINTER_DELTA is unset and leave it at that for the temporary workaround. WDYT?

I think this would work. It still wouldn't handle cases where the data in pp_magic ends up used as a non-pointer at all or a pointer to some static variable in the code like .mp_ops = &dmabuf_devmem_ops, right? Because these were never allocated from memory so are unrelated to PAGE_OFFSET.

But I guess things like that would have been a problem with the old code anwyway, so should be of no concern?