linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Kirill A. Shutemov" <kirill@shutemov.name>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>,
	David Rientjes <rientjes@google.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Dave Hansen <dave.hansen@intel.com>, Mel Gorman <mgorman@suse.de>,
	Rik van Riel <riel@redhat.com>, Christoph Lameter <cl@gentwo.org>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Steve Capper <steve.capper@linaro.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Michal Hocko <mhocko@suse.cz>,
	Jerome Marchand <jmarchan@redhat.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: page-flags behavior on compound pages: a worry
Date: Thu, 13 Aug 2015 01:21:36 +0300	[thread overview]
Message-ID: <20150812222136.GA15010@node.dhcp.inet.fi> (raw)
In-Reply-To: <20150812141644.ceb541e5b52d76049339a243@linux-foundation.org>

On Wed, Aug 12, 2015 at 02:16:44PM -0700, Andrew Morton wrote:
> On Wed, 12 Aug 2015 17:35:09 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Thu, Aug 06, 2015 at 12:24:22PM -0700, Hugh Dickins wrote:
> > > > IIUC, the only potentially problematic callsites left are physical memory
> > > > scanners. This code requires audit. I'll do that.
> > > 
> > > Please.
> > 
> > I haven't finished the exercise yet. But here's an issue I believe present
> > in current *Linus* tree:
> > 
> > >From e78eec7d7a8c4cba8b5952a997973f7741e704f4 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Date: Wed, 12 Aug 2015 17:09:16 +0300
> > Subject: [PATCH] mm: fix potential race in isolate_migratepages_block()
> > 
> > Hugh has pointed that compound_head() call can be unsafe in some context.
> > There's one example:
> > 
> > 	CPU0					CPU1
> > 
> > isolate_migratepages_block()
> >   page_count()
> >     compound_head()
> >       !!PageTail() == true
> > 					put_page()
> > 					  tail->first_page = NULL
> >       head = tail->first_page
> > 					alloc_pages(__GFP_COMP)
> > 					   prep_compound_page()
> > 					     tail->first_page = head
> > 					     __SetPageTail(p);
> >       !!PageTail() == true
> >     <head == NULL dereferencing>
> > 
> > The race is pure theoretical. I don't it's possible to trigger it in
> > practice. But who knows.
> > 
> > This can be fixed by avoiding compound_head() in unsafe context.
> 
> This is nuts :( page_count() should Just Work without us having to
> worry about bizarre races against splitting.  Sigh.

Split is not involved. And this race is present even for THP=n. :(

> 
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -787,7 +787,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >  		 * admittedly racy check.
> >  		 */
> >  		if (!page_mapping(page) &&
> > -		    page_count(page) > page_mapcount(page))
> > +		    atomic_read(&page->_count) > page_mapcount(page))
> >  			continue;
> 
> If we're going to do this sort of thing, can we please do it in a more
> transparent manner?  Let's not sprinkle unexplained and
> incomprehensible direct accesses to ->_count all over the place.
> 
> Create a formal function to do this, with an appropriate name and with
> documentation which fully explains what's going on.  Then use that
> here, and in has_unmovable_pages() (at least).

All this situation is ugly. I'm thinking on more general solution for
PageTail() vs. ->first_page race.

We would be able to avoid the race in first place if we encode PageTail()
and position of head page within the same word in struct page. This way we
update both thing in one shot without possibility of race.

Details get tricky.

I'm going to try tomorrow something like this: encode the position of head
as offset from the tail page and store it as negative number in the union
with ->mapping and ->s_mem. PageTail() can be implemented as check value
of the field to be in range -1..-MAX_ORDER_NR_PAGES. 

I'm not sure at all if it's going to work, especially looking on
ridiculously high CONFIG_FORCE_MAX_ZONEORDER some architectures allow.

We could also try to encode page order instead (again as negative number)
and calculate head page position based on alignment...

Any other ideas are welcome.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2015-08-12 22:21 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-19 17:08 [PATCH 00/16] Sanitize usage of ->flags and ->mapping for tail pages Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 01/16] mm: consolidate all page-flags helpers in <linux/page-flags.h> Kirill A. Shutemov
2015-03-23  0:10   ` Hugh Dickins
2015-03-19 17:08 ` [PATCH 02/16] page-flags: trivial cleanup for PageTrans* helpers Kirill A. Shutemov
2015-03-23  0:12   ` Hugh Dickins
2015-03-19 17:08 ` [PATCH 03/16] page-flags: introduce page flags policies wrt compound pages Kirill A. Shutemov
2015-03-20 20:35   ` Andrew Morton
2015-03-20 21:34     ` Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 04/16] page-flags: define PG_locked behavior on " Kirill A. Shutemov
2015-03-27 15:11   ` Mateusz Krawczuk
2015-03-27 15:13   ` Mateusz Krawczuk
2015-03-27 16:37     ` Kirill A. Shutemov
2015-07-15 20:20   ` Christoph Lameter
2015-08-06  4:15   ` page-flags behavior on compound pages: a worry Hugh Dickins
2015-08-06 15:33     ` Kirill A. Shutemov
2015-08-06 19:24       ` Hugh Dickins
2015-08-06 20:45         ` Christoph Lameter
2015-08-07 14:50           ` Kirill A. Shutemov
2015-08-07 15:28             ` Christoph Lameter
2015-08-10 11:09               ` Kirill A. Shutemov
2015-08-10 13:50                 ` Christoph Lameter
2015-08-07 14:49         ` Kirill A. Shutemov
2015-08-13  5:10           ` Hugh Dickins
2015-08-12 14:35         ` Kirill A. Shutemov
2015-08-12 14:47           ` Vlastimil Babka
2015-08-12 21:16           ` Andrew Morton
2015-08-12 22:21             ` Kirill A. Shutemov [this message]
2015-08-13  4:12               ` Hugh Dickins
2015-03-19 17:08 ` [PATCH 05/16] page-flags: define behavior of FS/IO-related flags on compound pages Kirill A. Shutemov
2015-03-19 18:29   ` Dave Hansen
2015-03-19 20:02     ` Kirill A. Shutemov
2015-03-23  0:02       ` Hugh Dickins
2015-03-23 12:17         ` Kirill A. Shutemov
2015-03-24 22:54           ` Hugh Dickins
2015-03-25 10:23             ` Kirill A. Shutemov
2015-03-25 18:56               ` Hugh Dickins
2015-03-19 17:08 ` [PATCH 06/16] page-flags: define behavior of LRU-related " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 07/16] page-flags: define behavior SL*B-related " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 08/16] page-flags: define behavior of Xen-related " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 09/16] page-flags: define PG_reserved behavior " Kirill A. Shutemov
2020-01-31 15:24   ` Chris Wilson
2020-02-03 15:18     ` Kirill A. Shutemov
2020-02-03 15:24       ` Chris Wilson
2020-02-03 17:10         ` David Hildenbrand
2020-02-03 17:29       ` Christoph Hellwig
2015-03-19 17:08 ` [PATCH 10/16] page-flags: define PG_swapbacked " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 11/16] page-flags: define PG_swapcache " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 12/16] page-flags: define PG_mlocked " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 13/16] page-flags: define PG_uncached " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 14/16] page-flags: define PG_uptodate " Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 15/16] page-flags: look on head page if the flag is encoded in page->mapping Kirill A. Shutemov
2015-03-19 17:08 ` [PATCH 16/16] mm: sanitize page->mapping for tail pages Kirill A. Shutemov
2015-03-23  0:28 ` [PATCH 00/16] Sanitize usage of ->flags and ->mapping " Hugh Dickins
2015-03-23 10:04   ` Kirill A. Shutemov
2015-03-24 23:42     ` Hugh Dickins
2015-03-25 10:55       ` Kirill A. Shutemov
2015-03-24 17:39 ` Konstantin Khlebnikov
2015-03-24 20:04   ` Kirill A. Shutemov
2015-07-15 20:20 ` Christoph Lameter
2015-07-15 21:18   ` Kirill A. Shutemov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150812222136.GA15010@node.dhcp.inet.fi \
    --to=kirill@shutemov.name \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=cl@gentwo.org \
    --cc=dave.hansen@intel.com \
    --cc=hannes@cmpxchg.org \
    --cc=hughd@google.com \
    --cc=jmarchan@redhat.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=mhocko@suse.cz \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=riel@redhat.com \
    --cc=rientjes@google.com \
    --cc=steve.capper@linaro.org \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).