Re: [PATCHv3 4/5] mm: make compound_head() robust

From: Vlastimil Babka
Date: Tue Aug 25 2015 - 07:44:22 EST

On 08/21/2015 02:10 PM, Kirill A. Shutemov wrote:
On Thu, Aug 20, 2015 at 04:36:43PM -0700, Andrew Morton wrote:
On Wed, 19 Aug 2015 12:21:45 +0300 "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:

The patch introduces page->compound_head into third double word block in
front of compound_dtor and compound_order. That means it shares storage
space with:

- page->;
- page->next;
- page->;
- page->pmd_huge_pte;

We should probably ask Paul about the chances that would like to use the bit too one day?
For pgtable_t I can't think of anything better than a warning in the generic definition in include/asm-generic/page.h and hope that anyone reimplementing it for a new arch will look there first.
The lru part is probably the hardest to prevent danger. It can be used for any private purposes. Hopefully everyone currently uses only standard list operations here, and the list poison values don't set bit 0. But I see there can be some arbitrary CONFIG_ILLEGAL_POINTER_VALUE added to the poisons, so maybe that's worth some build error check? Anyway we would be imposing restrictions on types that are not ours, so there might be some resistance...

Anyway, this is quite subtle and there's a risk that people will
accidentally break it later on. I don't think the patch puts
sufficient documentation in place to prevent this.

I would appreciate for suggestion on place and form of documentation.

And even documentation might not be enough to prevent accidents.

The only think I can propose is VM_BUG_ON() in PageTail() and
compound_head() which would ensure that page->compound_page points to
place within MAX_ORDER_NR_PAGES before the current page if bit 0 is set.

That should probably catch some bad stuff, but probably only moments before it would crash anyway if the pointer was bogus. But I also don't see better way, because we can't proactively put checks in those who would "misbehave", as we don't know who they are. Putting more debug checks in e.g. page freeing might help, but probably not much.

Do you consider this helpful?


--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -120,7 +120,12 @@ struct page {

- /* Third double word block */
+ /*
+ * Third double word block
+ *
+ * WARNING: bit 0 of the first word encode PageTail and *must* be 0
+ * for non-tail pages.
+ */
union {
struct list_head lru; /* Pageout list, eg. active_list
* protected by zone->lru_lock !
@@ -143,6 +148,7 @@ struct page {
/* First tail page of compound page */

Note that compound_head is not just in the *first* tail page. Only the rest is.

struct {
+ unsigned long compound_head; /* If bit zero is set */

I think the comments around here should have more details and should
be louder!

I'm always bad when it comes to documentation. Is it enough?

* Third double word block
* WARNING: bit 0 of the first word encode PageTail(). That means
* the rest users of the storage space MUST NOT use the bit to
* avoid collision and false-positive PageTail().

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at