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

[Paul E. McKenney]

On Tue, Aug 25, 2015 at 09:33:54PM +0300, Kirill A. Shutemov wrote:
> 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.

The call_rcu() function does stomp that bit, but if you stop using that
bit before you invoke call_rcu(), no problem.

								Thanx, 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/