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

[Kirill A. Shutemov]

On Tue, Aug 25, 2015 at 01:44:13PM +0200, Vlastimil Babka wrote:
> 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->lru.next;
> >>>  - page->next;
> >>>  - page->rcu_head.next;
> >>>  - page->pmd_huge_pte;
> >>>
> 
> We should probably ask Paul about the chances that rcu_head.next would like
> to use the bit too one day?

+Paul.

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

I will move it to other word, just in case.

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

I will add BUILD_BUG_ON((unsigned long)LIST_POISON1 & 1); 

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

So, do you think it worth it or not after all?
> 
> >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.

Right.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/