mm: compaction: buffer overflow in isolate_migratepages_range

DISCUSSION Sasha Levin 4 participants 11 messages Aug 10, 2014 – Aug 15, 2014 Memory management

Share & tools

Bad access happens when we read page->mapping->flags in mapping_ballon(). Address of page->mapping->flags here is ffff88051b70eb49, so the lowest bit is set,
which means that the lowest bit is also set in page->mapping pointer. So page->mapping is a pointer to anon_vma, not to address_space.

I guess this could be fixed with something like following (completely untested) patch:

include/linux/balloon_compaction.h +1 -1

@@ -127,7 +127,7 @@ static inline bool page_flags_cleared(struct page *page)
  */
 static inline bool __is_movable_balloon_page(struct page *page)
 {
-       struct address_space *mapping = page->mapping;
+       struct address_space *mapping = page_mapping(page);
        return mapping_balloon(mapping);
 }

Signature · 8 lines

Best regards,
Andrey Ryabinin

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

Rafael Aquini

2014-08-13 15:35 UTC #2 lore raw

Quoted 65 lines · Andrey Ryabinin

2014-08-10 5:45 GMT+04:00 Sasha Levin <[email protected]>:

Quoted 64 lines

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel with the KASAN patchset, I've stumbled on the following spew:

[ 3837.070452] ==================================================================
[ 3837.073101] AddressSanitizer: buffer overflow in isolate_migratepages_range+0x85f/0xd90 at addr ffff88051b70eb49
[ 3837.076310] page:ffffea00146dc380 count:0 mapcount:0 mapping:          (null) index:0x0
[ 3837.079876] page flags: 0xafffff80008000(tail)
[ 3837.114592] page dumped because: kasan error
[ 3837.115897] CPU: 4 PID: 29613 Comm: trinity-c467 Not tainted 3.16.0-next-20140808-sasha-00051-gf368221 #1051
[ 3837.118024]  00000000000000fc 0000000000000000 ffffea00146dc380 ffff8801f326f718
[ 3837.119837]  ffffffff97e0d344 ffff8801f326f7e8 ffff8801f326f7d8 ffffffff9342d5bc
[ 3837.121708]  ffffea00085163c0 0000000000000000 ffff8801f326f8e0 ffffffff93fe02fb
[ 3837.123704] Call Trace:
[ 3837.124272] dump_stack (lib/dump_stack.c:52)
[ 3837.125166] kasan_report_error (mm/kasan/report.c:98 mm/kasan/report.c:166)
[ 3837.126128] ? trace_hardirqs_on_thunk (arch/x86/lib/thunk_64.S:33)
[ 3837.127462] ? retint_restore_args (arch/x86/kernel/entry_64.S:828)
[ 3837.128753] __asan_load8 (mm/kasan/kasan.c:364)
[ 3837.129914] ? isolate_migratepages_range (./arch/x86/include/asm/bitops.h:311 include/linux/pagemap.h:70 include/linux/balloon_compaction.h:131 include/linux/balloon_compaction.h:156 mm/compaction.c:596)
[ 3837.131613] isolate_migratepages_range (./arch/x86/include/asm/bitops.h:311 include/linux/pagemap.h:70 include/linux/balloon_compaction.h:131 include/linux/balloon_compaction.h:156 mm/compaction.c:596)
[ 3837.132838] compact_zone (mm/compaction.c:877 mm/compaction.c:1044)
[ 3837.133818] compact_zone_order (mm/compaction.c:1106)
[ 3837.134982] try_to_compact_pages (mm/compaction.c:1161)
[ 3837.135970] __alloc_pages_direct_compact (mm/page_alloc.c:2313)
[ 3837.137217] ? next_zones_zonelist (mm/mmzone.c:72)
[ 3837.138861] __alloc_pages_nodemask (mm/page_alloc.c:2640 mm/page_alloc.c:2806)
[ 3837.139897] ? check_chain_key (kernel/locking/lockdep.c:2190)
[ 3837.141220] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[ 3837.142434] alloc_pages_vma (mm/mempolicy.c:2046)
[ 3837.143479] ? do_huge_pmd_wp_page (mm/huge_memory.c:774 mm/huge_memory.c:1123)
[ 3837.144663] do_huge_pmd_wp_page (mm/huge_memory.c:774 mm/huge_memory.c:1123)
[ 3837.145653] handle_mm_fault (mm/memory.c:3312 mm/memory.c:3370)
[ 3837.146717] ? vmacache_find (mm/vmacache.c:100 (discriminator 1))
[ 3837.147404] ? find_vma (mm/mmap.c:2024)
[ 3837.147982] __do_page_fault (arch/x86/mm/fault.c:1231)
[ 3837.148613] ? context_tracking_user_exit (kernel/context_tracking.c:184)
[ 3837.149388] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[ 3837.150212] ? trace_hardirqs_off_caller (kernel/locking/lockdep.c:2641 (discriminator 8))
[ 3837.150977] ? trace_hardirqs_off (kernel/locking/lockdep.c:2647)
[ 3837.151686] trace_do_page_fault (arch/x86/mm/fault.c:1314 include/linux/jump_label.h:115 include/linux/context_tracking_state.h:27 include/linux/context_tracking.h:45 arch/x86/mm/fault.c:1315)
[ 3837.152870] do_async_page_fault (arch/x86/kernel/kvm.c:279)
[ 3837.153886] async_page_fault (arch/x86/kernel/entry_64.S:1313)
[ 3837.155293] Read of size 8 by thread T29613:
[ 3837.156058] Memory state around the buggy address:
[ 3837.156885]  ffff88051b70e880: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
[ 3837.158141]  ffff88051b70e900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.159492]  ffff88051b70e980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.160863]  ffff88051b70ea00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 3837.162165]  ffff88051b70ea80: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
[ 3837.163552] >ffff88051b70eb00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.164866]                                               ^
[ 3837.165914]  ffff88051b70eb80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.167317]  ffff88051b70ec00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.168616]  ffff88051b70ec80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.169898]  ffff88051b70ed00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.171298]  ffff88051b70ed80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.172611] ==================================================================

Thanks,
Sasha

Nice pick from the sanitizer bits!

Quoted 7 lines

Bad access happens when we read page->mapping->flags in mapping_ballon(). Address of page->mapping->flags here is ffff88051b70eb49, so the lowest bit is set,
which means that the lowest bit is also set in page->mapping pointer. So page->mapping is a pointer to anon_vma, not to address_space.

Yeah, it happens because I failed to anticipate a race window opening where balloon_page_movable() can stumble across an anon page being released -- somewhere in the midway of __page_cache_release() & free_pages_prepare() down on the put_page() codepath -- while isolate_migratepages_range() performs its loop in the (lru) unlocked case.

Although harmless, IMO, as we only go for the isolation step if we hold the lru lock (and the check is re-done under lock safety) this is an annoying thing we have to get rid of to not defeat the purpose of having the kasan in place.

Quoted 3 lines

I guess this could be fixed with something like following (completely untested) patch:

Despite not having it tested, as well: yes, I belive this should be the way to sort out that warning, for the aforementioned unlocked iteration case of isolate_migratepages_range().

include/linux/balloon_compaction.h static inline bool page_flags_cleared(struct page *page) · line 133

Quoted 11 lines

include/linux/balloon_compaction.h +1 -1

@@ -127,7 +127,7 @@ static inline bool page_flags_cleared(struct page *page)
  */
 static inline bool __is_movable_balloon_page(struct page *page)
 {
-       struct address_space *mapping = page->mapping;
+       struct address_space *mapping = page_mapping(page);
        return mapping_balloon(mapping);
 }

Please, keep me in the loop if things don't settle properly.

Cheers,
-- Rafael

Rafael Aquini

2014-08-14 15:13 UTC #3 lore raw

Quoted 77 lines · Rafael Aquini

Quoted 65 lines · Andrey Ryabinin

2014-08-10 5:45 GMT+04:00 Sasha Levin <[email protected]>:

Quoted 64 lines

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel with the KASAN patchset, I've stumbled on the following spew:

[ 3837.070452] ==================================================================
[ 3837.073101] AddressSanitizer: buffer overflow in isolate_migratepages_range+0x85f/0xd90 at addr ffff88051b70eb49
[ 3837.076310] page:ffffea00146dc380 count:0 mapcount:0 mapping:          (null) index:0x0
[ 3837.079876] page flags: 0xafffff80008000(tail)
[ 3837.114592] page dumped because: kasan error
[ 3837.115897] CPU: 4 PID: 29613 Comm: trinity-c467 Not tainted 3.16.0-next-20140808-sasha-00051-gf368221 #1051
[ 3837.118024]  00000000000000fc 0000000000000000 ffffea00146dc380 ffff8801f326f718
[ 3837.119837]  ffffffff97e0d344 ffff8801f326f7e8 ffff8801f326f7d8 ffffffff9342d5bc
[ 3837.121708]  ffffea00085163c0 0000000000000000 ffff8801f326f8e0 ffffffff93fe02fb
[ 3837.123704] Call Trace:
[ 3837.124272] dump_stack (lib/dump_stack.c:52)
[ 3837.125166] kasan_report_error (mm/kasan/report.c:98 mm/kasan/report.c:166)
[ 3837.126128] ? trace_hardirqs_on_thunk (arch/x86/lib/thunk_64.S:33)
[ 3837.127462] ? retint_restore_args (arch/x86/kernel/entry_64.S:828)
[ 3837.128753] __asan_load8 (mm/kasan/kasan.c:364)
[ 3837.129914] ? isolate_migratepages_range (./arch/x86/include/asm/bitops.h:311 include/linux/pagemap.h:70 include/linux/balloon_compaction.h:131 include/linux/balloon_compaction.h:156 mm/compaction.c:596)
[ 3837.131613] isolate_migratepages_range (./arch/x86/include/asm/bitops.h:311 include/linux/pagemap.h:70 include/linux/balloon_compaction.h:131 include/linux/balloon_compaction.h:156 mm/compaction.c:596)
[ 3837.132838] compact_zone (mm/compaction.c:877 mm/compaction.c:1044)
[ 3837.133818] compact_zone_order (mm/compaction.c:1106)
[ 3837.134982] try_to_compact_pages (mm/compaction.c:1161)
[ 3837.135970] __alloc_pages_direct_compact (mm/page_alloc.c:2313)
[ 3837.137217] ? next_zones_zonelist (mm/mmzone.c:72)
[ 3837.138861] __alloc_pages_nodemask (mm/page_alloc.c:2640 mm/page_alloc.c:2806)
[ 3837.139897] ? check_chain_key (kernel/locking/lockdep.c:2190)
[ 3837.141220] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[ 3837.142434] alloc_pages_vma (mm/mempolicy.c:2046)
[ 3837.143479] ? do_huge_pmd_wp_page (mm/huge_memory.c:774 mm/huge_memory.c:1123)
[ 3837.144663] do_huge_pmd_wp_page (mm/huge_memory.c:774 mm/huge_memory.c:1123)
[ 3837.145653] handle_mm_fault (mm/memory.c:3312 mm/memory.c:3370)
[ 3837.146717] ? vmacache_find (mm/vmacache.c:100 (discriminator 1))
[ 3837.147404] ? find_vma (mm/mmap.c:2024)
[ 3837.147982] __do_page_fault (arch/x86/mm/fault.c:1231)
[ 3837.148613] ? context_tracking_user_exit (kernel/context_tracking.c:184)
[ 3837.149388] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[ 3837.150212] ? trace_hardirqs_off_caller (kernel/locking/lockdep.c:2641 (discriminator 8))
[ 3837.150977] ? trace_hardirqs_off (kernel/locking/lockdep.c:2647)
[ 3837.151686] trace_do_page_fault (arch/x86/mm/fault.c:1314 include/linux/jump_label.h:115 include/linux/context_tracking_state.h:27 include/linux/context_tracking.h:45 arch/x86/mm/fault.c:1315)
[ 3837.152870] do_async_page_fault (arch/x86/kernel/kvm.c:279)
[ 3837.153886] async_page_fault (arch/x86/kernel/entry_64.S:1313)
[ 3837.155293] Read of size 8 by thread T29613:
[ 3837.156058] Memory state around the buggy address:
[ 3837.156885]  ffff88051b70e880: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
[ 3837.158141]  ffff88051b70e900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.159492]  ffff88051b70e980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.160863]  ffff88051b70ea00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 3837.162165]  ffff88051b70ea80: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
[ 3837.163552] >ffff88051b70eb00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.164866]                                               ^
[ 3837.165914]  ffff88051b70eb80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 3837.167317]  ffff88051b70ec00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.168616]  ffff88051b70ec80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.169898]  ffff88051b70ed00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.171298]  ffff88051b70ed80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 3837.172611] ==================================================================

Thanks,
Sasha

Nice pick from the sanitizer bits!

Quoted 7 lines

Bad access happens when we read page->mapping->flags in mapping_ballon(). Address of page->mapping->flags here is ffff88051b70eb49, so the lowest bit is set,
which means that the lowest bit is also set in page->mapping pointer. So page->mapping is a pointer to anon_vma, not to address_space.

Quoted 6 lines

Yeah, it happens because I failed to anticipate a race window opening where balloon_page_movable() can stumble across an anon page being released -- somewhere in the midway of __page_cache_release() & free_pages_prepare() down on the put_page() codepath -- while isolate_migratepages_range() performs its loop in the (lru) unlocked case.

Giving it a second thought, I see my first analisys (above) isn't accurate, as if we had raced against a page being released at the point I mentioned, balloon_page_movable() would have bailed out while performing its page_flags_cleared() checkpoint.

But I now can see from where this occurrence is coming from, actually.

The real race window for this issue opens when balloon_page_movable() checkpoint @ isolate_migratepages_range() stumbles across a (new) page under migration at:

static int move_to_new_page(struct page *newpage, struct page *page, ... {
...
newpage->mapping = page->mapping;

At this point, *newpage points to a fresh page coming out from the allocator (just as any other possible ballooned page), but it gets its ->mapping pointer set, which can create the conditions to the access (for mapping flag checking purposes only) KASAN is complaining about, if *page happens to be pointing to an anon page.

Quoted 5 lines

Although harmless, IMO, as we only go for the isolation step if we hold the lru lock (and the check is re-done under lock safety) this is an annoying thing we have to get rid of to not defeat the purpose of having the kasan in place.

It still a harmless condition as before, but considering what goes above I'm now convinced & confident the patch proposed by Andrey is the real fix for such occurrences.

Cheers,
-- Rafael

Quoted 30 lines

Quoted 4 lines
I guess this could be fixed with something like following (completely untested) patch:
Despite not having it tested, as well: yes, I belive this should be the way to sort out that warning, for the aforementioned unlocked iteration case of isolate_migratepages_range().
include/linux/balloon_compaction.h static inline bool page_flags_cleared(struct page *page) · line 133
Quoted 11 lines
include/linux/balloon_compaction.h +1 -1
@@ -127,7 +127,7 @@ static inline bool page_flags_cleared(struct page *page)
  */
 static inline bool __is_movable_balloon_page(struct page *page)
 {
-       struct address_space *mapping = page->mapping;
+       struct address_space *mapping = page_mapping(page);
        return mapping_balloon(mapping);
 }
 
Please, keep me in the loop if things don't settle properly.
Cheers,
-- Rafael
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrey Ryabinin

2014-08-14 18:07 UTC #4 lore raw

2014-08-14 19:13 GMT+04:00 Rafael Aquini <[email protected]>:

Quoted 37 lines

Quoted 6 lines
Yeah, it happens because I failed to anticipate a race window opening where balloon_page_movable() can stumble across an anon page being released -- somewhere in the midway of __page_cache_release() & free_pages_prepare() down on the put_page() codepath -- while isolate_migratepages_range() performs its loop in the (lru) unlocked case.
Giving it a second thought, I see my first analisys (above) isn't accurate, as if we had raced against a page being released at the point I mentioned, balloon_page_movable() would have bailed out while performing its page_flags_cleared() checkpoint.
But I now can see from where this occurrence is coming from, actually.
The real race window for this issue opens when balloon_page_movable() checkpoint @ isolate_migratepages_range() stumbles across a (new) page under migration at:
static int move_to_new_page(struct page *newpage, struct page *page, ...
{
   ...
   newpage->mapping = page->mapping;
At this point, *newpage points to a fresh page coming out from the allocator (just as any other possible ballooned page), but it gets its ->mapping pointer set, which can create the conditions to the access (for mapping flag checking purposes only) KASAN is complaining about, if *page happens to be pointing to an anon page.
Quoted 5 lines
Although harmless, IMO, as we only go for the isolation step if we hold the lru lock (and the check is re-done under lock safety) this is an annoying thing we have to get rid of to not defeat the purpose of having the kasan in place.

Quoted 4 lines

It still a harmless condition as before, but considering what goes above I'm now convinced & confident the patch proposed by Andrey is the real fix for such occurrences.

I don't think that it's harmless, because we could cross page boundary here and try to read from a memory hole.
And this code has more potential problems like use after free. Since we don't hold locks properly here,
page->mapping could point to freed struct address_space.

We discussed this with Konstantin and he suggested a better solution for this. If I understood him correctly the main idea was to store bit identifying ballon page
in struct page (special value in _mapcount), so we won't need to check mapping->flags.

Signature · 8 lines

Best regards,
Andrey Ryabinin

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

Rafael Aquini

2014-08-14 18:54 UTC #5 lore raw

Quoted 9 lines · Andrey Ryabinin

2014-08-14 19:13 GMT+04:00 Rafael Aquini <[email protected]>:
Quoted 4 lines
It still a harmless condition as before, but considering what goes above I'm now convinced & confident the patch proposed by Andrey is the real fix for such occurrences.
I don't think that it's harmless, because we could cross page boundary here and try to read from a memory hole.

I think isolate_migratepages_range() skips over holes, doesn't it?

Quoted 4 lines

And this code has more potential problems like use after free. Since we don't hold locks properly here,
page->mapping could point to freed struct address_space.

Thinking on how things go for isolate_migratepages_range() and balloon pages, I struggle to find a way where that could happen. OTOH, I failed to see things more blatant before, so I won't argue here. Defensive programming is always better than negating possibilities ;)

Quoted 6 lines

We discussed this with Konstantin and he suggested a better solution for this. If I understood him correctly the main idea was to store bit identifying ballon page
in struct page (special value in _mapcount), so we won't need to check mapping->flags.

I liked it. Something in the line of PageBuddy()/PAGE_BUDDY_MAPCOUNT_VALUE scheme. This is clearly cleaner than what we have in place today, and I'm ashamed I didn't think of it before. Thanks for pointing that out.

Cheers,
-- Rafael

Rafael Aquini

2014-08-14 21:44 UTC #6 lore raw

In reply to Andrey Ryabinin

Quoted 6 lines · Andrey Ryabinin

We discussed this with Konstantin and he suggested a better solution for this. If I understood him correctly the main idea was to store bit identifying ballon page
in struct page (special value in _mapcount), so we won't need to check mapping->flags.

Here goes what I thought doing, following that suggestion of Konstantin and yours. (I didn't tested it yet)

Comments are welcomed.

Cheers,
-- Rafael

---- 8< ----
From: Rafael Aquini <[email protected]>
Subject: mm: balloon_compaction: enhance balloon_page_movable() checkpoint against races

While testing linux-next for the Kernel Address Sanitizer patchset (KASAN) Sasha Levin reported a buffer overflow warning triggered for isolate_migratepages_range(), which lated was discovered happening due to a condition where balloon_page_movable() raced against move_to_new_page(), while the later was copying the page->mapping of an anon page.

Because we can perform balloon_page_movable() in a lockless fashion at isolate_migratepages_range(), the dicovered race has unveiled the scheme actually used to spot ballooned pages among page blocks that checks for page_flags_cleared() and dereference page->mapping to check its mapping flags is weak and potentially prone to stumble across another similar conditions in the future.

Following Konstantin Khlebnikov's and Andrey Ryabinin's suggestions, this patch replaces the old page->flags && mapping->flags checking scheme with a more simple and strong page->_mapcount read and compare value test. Similarly to what is done for PageBuddy() checks, BALLOON_PAGE_MAPCOUNT_VALUE is introduced here to mark balloon pages. This allows balloon_page_movable() to skip the proven troublesome dereference of page->mapping for flag checking while it goes on isolate_migratepages_range() lockless rounds. page->mapping dereference and flag-checking will be performed later, when all locks are held properly.

---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 53 +++++++++++++++++----------------
 2 files changed, 45 insertions(+), 69 deletions(-)

include/linux/balloon_compaction.h +18 ~~-43~~

index 089743a..1409ccc 100644
@@ -108,54 +108,29 @@ static inline void balloon_mapping_free(struct address_space *balloon_mapping)
 }
 
 /*
- * page_flags_cleared - helper to perform balloon @page ->flags tests.
+ * balloon_page_movable - identify balloon pages that can be moved by
+ *			  compaction / migration.
  *
- * As balloon pages are obtained from buddy and we do not play with page->flags
- * at driver level (exception made when we get the page lock for compaction),
- * we can safely identify a ballooned page by checking if the
- * PAGE_FLAGS_CHECK_AT_PREP page->flags are all cleared.  This approach also
- * helps us skip ballooned pages that are locked for compaction or release, thus
- * mitigating their racy check at balloon_page_movable()
+ * BALLOON_PAGE_MAPCOUNT_VALUE must be <= -2 but better not too close to
+ * -2 so that an underflow of the page_mapcount() won't be mistaken
+ * for a genuine BALLOON_PAGE_MAPCOUNT_VALUE.
  */
-static inline bool page_flags_cleared(struct page *page)
+#define BALLOON_PAGE_MAPCOUNT_VALUE (-256)
+static inline bool balloon_page_movable(struct page *page)
 {
-	return !(page->flags & PAGE_FLAGS_CHECK_AT_PREP);
+	return atomic_read(&page->_mapcount) == BALLOON_PAGE_MAPCOUNT_VALUE;
 }
 
-/*
- * __is_movable_balloon_page - helper to perform @page mapping->flags tests
- */
-static inline bool __is_movable_balloon_page(struct page *page)
+static inline void __balloon_page_set(struct page *page)
 {
-	struct address_space *mapping = page->mapping;
-	return mapping_balloon(mapping);
+	VM_BUG_ON_PAGE(!atomic_read(&page->_mapcount) != -1, page);
+	atomic_set(&page->_mapcount, BALLOON_PAGE_MAPCOUNT_VALUE);
 }
 
-/*
- * balloon_page_movable - test page->mapping->flags to identify balloon pages
- *			  that can be moved by compaction/migration.
- *
- * This function is used at core compaction's page isolation scheme, therefore
- * most pages exposed to it are not enlisted as balloon pages and so, to avoid
- * undesired side effects like racing against __free_pages(), we cannot afford
- * holding the page locked while testing page->mapping->flags here.
- *
- * As we might return false positives in the case of a balloon page being just
- * released under us, the page->mapping->flags need to be re-tested later,
- * under the proper page lock, at the functions that will be coping with the
- * balloon page case.
- */
-static inline bool balloon_page_movable(struct page *page)
+static inline void __balloon_page_clear(struct page *page)
 {
-	/*
-	 * Before dereferencing and testing mapping->flags, let's make sure
-	 * this is not a page that uses ->mapping in a different way
-	 */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) == 1)
-		return __is_movable_balloon_page(page);
-
-	return false;
+	VM_BUG_ON_PAGE(!balloon_page_movable(page), page)
+	atomic_set(&page->_mapcount, -1);
 }
 
 /*

@@ -170,10 +145,8 @@ static inline bool balloon_page_movable(struct page *page)
  */
 static inline bool isolated_balloon_page(struct page *page)
 {
-	/* Already isolated balloon pages, by default, have a raised refcount */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) >= 2)
-		return __is_movable_balloon_page(page);
+	if (balloon_page_movable && page_count(page) > 1)
+		return true;
 
 	return false;
 }

@@ -193,6 +166,7 @@ static inline void balloon_page_insert(struct page *page,
 				       struct list_head *head)
 {
 	page->mapping = mapping;
+	__balloon_page_set(page);
 	list_add(&page->lru, head);
 }

@@ -207,6 +181,7 @@ static inline void balloon_page_insert(struct page *page,
 static inline void balloon_page_delete(struct page *page)
 {
 	page->mapping = NULL;
+	__balloon_page_clear(page);
 	list_del(&page->lru);
 }

mm/balloon_compaction.c +27 ~~-26~~

95 lines not on this page · raw message

Signature · 1 line

1.9.3

Rafael Aquini

2014-08-14 22:07 UTC #7 lore raw

Quoted 157 lines · Rafael Aquini

Quoted 6 lines · Andrey Ryabinin

We discussed this with Konstantin and he suggested a better solution for this. If I understood him correctly the main idea was to store bit identifying ballon page
in struct page (special value in _mapcount), so we won't need to check mapping->flags.

Here goes what I thought doing, following that suggestion of Konstantin and yours. (I didn't tested it yet)

Comments are welcomed.

Cheers,
-- Rafael

---- 8< ----
From: Rafael Aquini <[email protected]>
Subject: mm: balloon_compaction: enhance balloon_page_movable() checkpoint against races

---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 53 +++++++++++++++++----------------
 2 files changed, 45 insertions(+), 69 deletions(-)

include/linux/balloon_compaction.h +18 ~~-43~~

index 089743a..1409ccc 100644
@@ -108,54 +108,29 @@ static inline void balloon_mapping_free(struct address_space *balloon_mapping)
 }
 
 /*
- * page_flags_cleared - helper to perform balloon @page ->flags tests.
+ * balloon_page_movable - identify balloon pages that can be moved by
+ *			  compaction / migration.
  *
- * As balloon pages are obtained from buddy and we do not play with page->flags
- * at driver level (exception made when we get the page lock for compaction),
- * we can safely identify a ballooned page by checking if the
- * PAGE_FLAGS_CHECK_AT_PREP page->flags are all cleared.  This approach also
- * helps us skip ballooned pages that are locked for compaction or release, thus
- * mitigating their racy check at balloon_page_movable()
+ * BALLOON_PAGE_MAPCOUNT_VALUE must be <= -2 but better not too close to
+ * -2 so that an underflow of the page_mapcount() won't be mistaken
+ * for a genuine BALLOON_PAGE_MAPCOUNT_VALUE.
  */
-static inline bool page_flags_cleared(struct page *page)
+#define BALLOON_PAGE_MAPCOUNT_VALUE (-256)
+static inline bool balloon_page_movable(struct page *page)
 {
-	return !(page->flags & PAGE_FLAGS_CHECK_AT_PREP);
+	return atomic_read(&page->_mapcount) == BALLOON_PAGE_MAPCOUNT_VALUE;
 }
 
-/*
- * __is_movable_balloon_page - helper to perform @page mapping->flags tests
- */
-static inline bool __is_movable_balloon_page(struct page *page)
+static inline void __balloon_page_set(struct page *page)
 {
-	struct address_space *mapping = page->mapping;
-	return mapping_balloon(mapping);
+	VM_BUG_ON_PAGE(!atomic_read(&page->_mapcount) != -1, page);
+	atomic_set(&page->_mapcount, BALLOON_PAGE_MAPCOUNT_VALUE);
 }
 
-/*
- * balloon_page_movable - test page->mapping->flags to identify balloon pages
- *			  that can be moved by compaction/migration.
- *
- * This function is used at core compaction's page isolation scheme, therefore
- * most pages exposed to it are not enlisted as balloon pages and so, to avoid
- * undesired side effects like racing against __free_pages(), we cannot afford
- * holding the page locked while testing page->mapping->flags here.
- *
- * As we might return false positives in the case of a balloon page being just
- * released under us, the page->mapping->flags need to be re-tested later,
- * under the proper page lock, at the functions that will be coping with the
- * balloon page case.
- */
-static inline bool balloon_page_movable(struct page *page)
+static inline void __balloon_page_clear(struct page *page)
 {
-	/*
-	 * Before dereferencing and testing mapping->flags, let's make sure
-	 * this is not a page that uses ->mapping in a different way
-	 */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) == 1)
-		return __is_movable_balloon_page(page);
-
-	return false;
+	VM_BUG_ON_PAGE(!balloon_page_movable(page), page)
+	atomic_set(&page->_mapcount, -1);
 }
 
 /*

@@ -170,10 +145,8 @@ static inline bool balloon_page_movable(struct page *page)
  */
 static inline bool isolated_balloon_page(struct page *page)
 {
-	/* Already isolated balloon pages, by default, have a raised refcount */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) >= 2)
-		return __is_movable_balloon_page(page);
+	if (balloon_page_movable && page_count(page) > 1)
+		return true;
 
 	return false;
 }

@@ -193,6 +166,7 @@ static inline void balloon_page_insert(struct page *page,
 				       struct list_head *head)
 {
 	page->mapping = mapping;
+	__balloon_page_set(page);
 	list_add(&page->lru, head);
 }

@@ -207,6 +181,7 @@ static inline void balloon_page_insert(struct page *page,
 static inline void balloon_page_delete(struct page *page)
 {
 	page->mapping = NULL;
+	__balloon_page_clear(page);
 	list_del(&page->lru);
 }

mm/balloon_compaction.c +0 -0

index 6e45a50..1cfb254 100644
@@ -220,33 +220,32 @@ bool balloon_page_isolate(struct page *page)
 	 * raise its refcount preventing __free_pages() from doing its job
 	 * the put_page() at the end of this block will take care of
 	 * release this page, thus avoiding a nasty leakage.

mm/balloon_compaction.c bool balloon_page_isolate(struct page *page) · line 231

Quoted 10 lines

mm/balloon_compaction.c +8 -1

+	 *
+	 * As balloon pages are not isolated from LRU lists, concurrent
+	 * compaction threads can race against page migration functions
+	 * as well as race against the balloon driver releasing a page.
+	 * The aforementioned operations are done under the safety of
+	 * page lock, so lets be sure we hold it before proceeding the
+	 * isolations steps here.
 	 */
-	if (likely(get_page_unless_zero(page))) {
+	if (get_page_unless_zero(page) && trylock_page(page)) {

Oops, I need to change this one, as we can left behind pages with raised refcounts if get_page_unless_zero(page) but trylock_page(page) fails here.

Quoted 87 lines

mm/balloon_compaction.c +19 ~~-25~~

 		/*
-		 * As balloon pages are not isolated from LRU lists, concurrent
-		 * compaction threads can race against page migration functions
-		 * as well as race against the balloon driver releasing a page.
-		 *
-		 * In order to avoid having an already isolated balloon page
-		 * being (wrongly) re-isolated while it is under migration,
-		 * or to avoid attempting to isolate pages being released by
-		 * the balloon driver, lets be sure we have the page lock
-		 * before proceeding with the balloon page isolation steps.
+		 * A ballooned page, by default, has just one refcount.
+		 * Prevent concurrent compaction threads from isolating
+		 * an already isolated balloon page by refcount check.
 		 */
-		if (likely(trylock_page(page))) {
-			/*
-			 * A ballooned page, by default, has just one refcount.
-			 * Prevent concurrent compaction threads from isolating
-			 * an already isolated balloon page by refcount check.
-			 */
-			if (__is_movable_balloon_page(page) &&
-			    page_count(page) == 2) {
+		if (balloon_page_movable(page) && page_count(page) == 2) {
+			struct address_space *mapping = page_mapping(page);
+			if (likely(mapping_balloon(mapping))) {
 				__isolate_balloon_page(page);
 				unlock_page(page);
 				return true;
+			} else {
+				dump_page(page, "not movable balloon page");
+				WARN_ON(1);
 			}
-			unlock_page(page);
 		}
+		unlock_page(page);
 		put_page(page);
 	}
 	return false;

@@ -255,19 +254,21 @@ bool balloon_page_isolate(struct page *page)
 /* putback_lru_page() counterpart for a ballooned page */
 void balloon_page_putback(struct page *page)
 {
+	struct address_space *mapping;
 	/*
 	 * 'lock_page()' stabilizes the page and prevents races against
 	 * concurrent isolation threads attempting to re-isolate it.
 	 */
 	lock_page(page);
 
-	if (__is_movable_balloon_page(page)) {
+	mapping = page_mapping(page);
+	if (balloon_page_movable(page) && mapping_balloon(mapping)) {
 		__putback_balloon_page(page);
 		/* drop the extra ref count taken for page isolation */
 		put_page(page);
 	} else {
-		WARN_ON(1);
 		dump_page(page, "not movable balloon page");
+		WARN_ON(1);
 	}
 	unlock_page(page);
 }

@@ -286,16 +287,16 @@ int balloon_page_migrate(struct page *newpage,
 	 */
 	BUG_ON(!trylock_page(newpage));
 
-	if (WARN_ON(!__is_movable_balloon_page(page))) {
+	mapping = page_mapping(page);
+	if (!(balloon_page_movable(page) && mapping_balloon(mapping))) {
 		dump_page(page, "not movable balloon page");
-		unlock_page(newpage);
-		return rc;
+		WARN_ON(1);
+		goto out;
 	}
 
-	mapping = page->mapping;
 	if (mapping)
 		rc = __migrate_balloon_page(mapping, newpage, page, mode);
-
+out:
 	unlock_page(newpage);
 	return rc;
 }

Signature · 1 line

1.9.3

Konstantin Khlebnikov

2014-08-15 03:36 UTC #8 lore raw

Quoted 163 lines · Rafael Aquini

Quoted 162 lines · Rafael Aquini

Quoted 6 lines · Andrey Ryabinin

We discussed this with Konstantin and he suggested a better solution for this. If I understood him correctly the main idea was to store bit identifying ballon page
in struct page (special value in _mapcount), so we won't need to check mapping->flags.

Here goes what I thought doing, following that suggestion of Konstantin and yours. (I didn't tested it yet)

Comments are welcomed.

Cheers,
-- Rafael

---- 8< ----
From: Rafael Aquini <[email protected]>
Subject: mm: balloon_compaction: enhance balloon_page_movable() checkpoint against races

---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 53 +++++++++++++++++----------------
 2 files changed, 45 insertions(+), 69 deletions(-)

include/linux/balloon_compaction.h +18 ~~-43~~

index 089743a..1409ccc 100644
@@ -108,54 +108,29 @@ static inline void balloon_mapping_free(struct address_space *balloon_mapping)
 }
 
 /*
- * page_flags_cleared - helper to perform balloon @page ->flags tests.
+ * balloon_page_movable - identify balloon pages that can be moved by
+ *                     compaction / migration.
  *
- * As balloon pages are obtained from buddy and we do not play with page->flags
- * at driver level (exception made when we get the page lock for compaction),
- * we can safely identify a ballooned page by checking if the
- * PAGE_FLAGS_CHECK_AT_PREP page->flags are all cleared.  This approach also
- * helps us skip ballooned pages that are locked for compaction or release, thus
- * mitigating their racy check at balloon_page_movable()
+ * BALLOON_PAGE_MAPCOUNT_VALUE must be <= -2 but better not too close to
+ * -2 so that an underflow of the page_mapcount() won't be mistaken
+ * for a genuine BALLOON_PAGE_MAPCOUNT_VALUE.
  */
-static inline bool page_flags_cleared(struct page *page)
+#define BALLOON_PAGE_MAPCOUNT_VALUE (-256)
+static inline bool balloon_page_movable(struct page *page)
 {
-     return !(page->flags & PAGE_FLAGS_CHECK_AT_PREP);
+     return atomic_read(&page->_mapcount) == BALLOON_PAGE_MAPCOUNT_VALUE;
 }
 
-/*
- * __is_movable_balloon_page - helper to perform @page mapping->flags tests
- */
-static inline bool __is_movable_balloon_page(struct page *page)
+static inline void __balloon_page_set(struct page *page)
 {
-     struct address_space *mapping = page->mapping;
-     return mapping_balloon(mapping);
+     VM_BUG_ON_PAGE(!atomic_read(&page->_mapcount) != -1, page);
+     atomic_set(&page->_mapcount, BALLOON_PAGE_MAPCOUNT_VALUE);
 }
 
-/*
- * balloon_page_movable - test page->mapping->flags to identify balloon pages
- *                     that can be moved by compaction/migration.
- *
- * This function is used at core compaction's page isolation scheme, therefore
- * most pages exposed to it are not enlisted as balloon pages and so, to avoid
- * undesired side effects like racing against __free_pages(), we cannot afford
- * holding the page locked while testing page->mapping->flags here.
- *
- * As we might return false positives in the case of a balloon page being just
- * released under us, the page->mapping->flags need to be re-tested later,
- * under the proper page lock, at the functions that will be coping with the
- * balloon page case.
- */
-static inline bool balloon_page_movable(struct page *page)
+static inline void __balloon_page_clear(struct page *page)
 {
-     /*
-      * Before dereferencing and testing mapping->flags, let's make sure
-      * this is not a page that uses ->mapping in a different way
-      */
-     if (page_flags_cleared(page) && !page_mapped(page) &&
-         page_count(page) == 1)
-             return __is_movable_balloon_page(page);
-
-     return false;
+     VM_BUG_ON_PAGE(!balloon_page_movable(page), page)
+     atomic_set(&page->_mapcount, -1);
 }
 
 /*

@@ -170,10 +145,8 @@ static inline bool balloon_page_movable(struct page *page)
  */
 static inline bool isolated_balloon_page(struct page *page)
 {
-     /* Already isolated balloon pages, by default, have a raised refcount */
-     if (page_flags_cleared(page) && !page_mapped(page) &&
-         page_count(page) >= 2)
-             return __is_movable_balloon_page(page);
+     if (balloon_page_movable && page_count(page) > 1)
+             return true;
 
      return false;
 }

@@ -193,6 +166,7 @@ static inline void balloon_page_insert(struct page *page,
                                     struct list_head *head)
 {
      page->mapping = mapping;
+     __balloon_page_set(page);
      list_add(&page->lru, head);
 }

@@ -207,6 +181,7 @@ static inline void balloon_page_insert(struct page *page,
 static inline void balloon_page_delete(struct page *page)
 {
      page->mapping = NULL;
+     __balloon_page_clear(page);
      list_del(&page->lru);
 }

mm/balloon_compaction.c +5 -0

index 6e45a50..1cfb254 100644
@@ -220,33 +220,32 @@ bool balloon_page_isolate(struct page *page)
       * raise its refcount preventing __free_pages() from doing its job
       * the put_page() at the end of this block will take care of
       * release this page, thus avoiding a nasty leakage.
+      *
+      * As balloon pages are not isolated from LRU lists, concurrent
+      * compaction threads can race against page migration functions
+      * as well as race against the balloon driver releasing a page.
+      * The aforementioned operations are done under the safety of

Quoted 8 lines

mm/balloon_compaction.c bool balloon_page_isolate(struct page *page) · line 231
Quoted 5 lines
mm/balloon_compaction.c +3 -1
+      * page lock, so lets be sure we hold it before proceeding the
+      * isolations steps here.
       */
-     if (likely(get_page_unless_zero(page))) {
+     if (get_page_unless_zero(page) && trylock_page(page)) {
Oops, I need to change this one, as we can left behind pages with raised refcounts if get_page_unless_zero(page) but trylock_page(page) fails here.

Don't hurry. The code in this state for years.
I'm working on patches for this, if everything goes well I'll show it today. As usual I couldn't stop myself from cleaning the mess, so it will be bigger than yours.

Quoted 78 lines

mm/balloon_compaction.c +18 ~~-23~~

              /*
-              * As balloon pages are not isolated from LRU lists, concurrent
-              * compaction threads can race against page migration functions
-              * as well as race against the balloon driver releasing a page.
-              *
-              * In order to avoid having an already isolated balloon page
-              * being (wrongly) re-isolated while it is under migration,
-              * or to avoid attempting to isolate pages being released by
-              * the balloon driver, lets be sure we have the page lock
-              * before proceeding with the balloon page isolation steps.
+              * A ballooned page, by default, has just one refcount.
+              * Prevent concurrent compaction threads from isolating
+              * an already isolated balloon page by refcount check.
               */
-             if (likely(trylock_page(page))) {
-                     /*
-                      * A ballooned page, by default, has just one refcount.
-                      * Prevent concurrent compaction threads from isolating
-                      * an already isolated balloon page by refcount check.
-                      */
-                     if (__is_movable_balloon_page(page) &&
-                         page_count(page) == 2) {
+             if (balloon_page_movable(page) && page_count(page) == 2) {
+                     struct address_space *mapping = page_mapping(page);
+                     if (likely(mapping_balloon(mapping))) {
                              __isolate_balloon_page(page);
                              unlock_page(page);
                              return true;
+                     } else {
+                             dump_page(page, "not movable balloon page");
+                             WARN_ON(1);
                      }
-                     unlock_page(page);
              }
+             unlock_page(page);
              put_page(page);
      }
      return false;

@@ -255,19 +254,21 @@ bool balloon_page_isolate(struct page *page)
 /* putback_lru_page() counterpart for a ballooned page */
 void balloon_page_putback(struct page *page)
 {
+     struct address_space *mapping;
      /*
       * 'lock_page()' stabilizes the page and prevents races against
       * concurrent isolation threads attempting to re-isolate it.
       */
      lock_page(page);
 
-     if (__is_movable_balloon_page(page)) {
+     mapping = page_mapping(page);
+     if (balloon_page_movable(page) && mapping_balloon(mapping)) {
              __putback_balloon_page(page);
              /* drop the extra ref count taken for page isolation */
              put_page(page);
      } else {
-             WARN_ON(1);
              dump_page(page, "not movable balloon page");
+             WARN_ON(1);
      }
      unlock_page(page);
 }

@@ -286,16 +287,16 @@ int balloon_page_migrate(struct page *newpage,
       */
      BUG_ON(!trylock_page(newpage));
 
-     if (WARN_ON(!__is_movable_balloon_page(page))) {
+     mapping = page_mapping(page);
+     if (!(balloon_page_movable(page) && mapping_balloon(mapping))) {
              dump_page(page, "not movable balloon page");
-             unlock_page(newpage);
-             return rc;
+             WARN_ON(1);
+             goto out;
      }

Quoted 11 lines

mm/balloon_compaction.c +1 -3

-     mapping = page->mapping;
      if (mapping)
              rc = __migrate_balloon_page(mapping, newpage, page, mode);
-
+out:
      unlock_page(newpage);
      return rc;
 }
--

1.9.3

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

Rafael Aquini

2014-08-15 05:22 UTC #9 lore raw

Quoted 5 lines · Konstantin Khlebnikov

Don't hurry. The code in this state for years.
I'm working on patches for this, if everything goes well I'll show it today. As usual I couldn't stop myself from cleaning the mess, so it will be bigger than yours.

Sorry,

I didn't see this reply of yours before sending out an adjusted-and-tested version of that patch, and asked Sasha to check it against his test-case.

Please, do not hesitate in providing your change ideas, though. I'd really appreciate your assessment feedback on that code.

Cheers,
-- Rafael

Rafael Aquini

2014-08-15 05:12 UTC #10 lore raw

In reply to Rafael Aquini

Here's a potential final version for the patch mentioned in a earlier message. The nitpick I raised to myself and a couple of other minor typing issues are fixed.

I did a preliminary testround, in a KVM guest ballooning in and out memory by chunks of 1GB while a script within the guest was forcing compaction concurrently verything looked alright.

Sasha, could you give this a try to see if that reported KASAN warning fades away, please?

Cheers,
-- Rafael

---8<---
From: Rafael Aquini <[email protected]>
Subject: [PATCH v2] mm: balloon_compaction: enhance balloon_page_movable()
checkpoint against races

While testing linux-next for the Kernel Address Sanitizer patchset (KASAN) Sasha Levin reported a buffer overflow warning triggered for isolate_migratepages_range(), which later was discovered happening due to a condition where balloon_page_movable() raced against move_to_new_page(), while the later was copying the page->mapping of an anon page.

Because we can perform balloon_page_movable() in a lockless fashion at isolate_migratepages_range(), the discovered race has unveiled the scheme actually used to spot ballooned pages among page blocks that checks for page_flags_cleared() and dereference page->mapping to check its mapping flags is weak and potentially prone to stumble across another similar conditions in the future.

Signed-off-by Rafael Aquini <[email protected]>

---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 59 ++++++++++++++++++++++--------------
 2 files changed, 54 insertions(+), 66 deletions(-)

include/linux/balloon_compaction.h +18 ~~-43~~

index 089743a..e00d5b0 100644
@@ -108,54 +108,29 @@ static inline void balloon_mapping_free(struct address_space *balloon_mapping)
 }
 
 /*
- * page_flags_cleared - helper to perform balloon @page ->flags tests.
+ * balloon_page_movable - identify balloon pages that can be moved by
+ *			  compaction / migration.
  *
- * As balloon pages are obtained from buddy and we do not play with page->flags
- * at driver level (exception made when we get the page lock for compaction),
- * we can safely identify a ballooned page by checking if the
- * PAGE_FLAGS_CHECK_AT_PREP page->flags are all cleared.  This approach also
- * helps us skip ballooned pages that are locked for compaction or release, thus
- * mitigating their racy check at balloon_page_movable()
+ * BALLOON_PAGE_MAPCOUNT_VALUE must be <= -2 but better not too close to
+ * -2 so that an underflow of the page_mapcount() won't be mistaken
+ * for a genuine BALLOON_PAGE_MAPCOUNT_VALUE.
  */
-static inline bool page_flags_cleared(struct page *page)
+#define BALLOON_PAGE_MAPCOUNT_VALUE (-256)
+static inline bool balloon_page_movable(struct page *page)
 {
-	return !(page->flags & PAGE_FLAGS_CHECK_AT_PREP);
+	return atomic_read(&page->_mapcount) == BALLOON_PAGE_MAPCOUNT_VALUE;
 }
 
-/*
- * __is_movable_balloon_page - helper to perform @page mapping->flags tests
- */
-static inline bool __is_movable_balloon_page(struct page *page)
+static inline void __balloon_page_set(struct page *page)
 {
-	struct address_space *mapping = page->mapping;
-	return mapping_balloon(mapping);
+	VM_BUG_ON_PAGE(atomic_read(&page->_mapcount) != -1, page);
+	atomic_set(&page->_mapcount, BALLOON_PAGE_MAPCOUNT_VALUE);
 }
 
-/*
- * balloon_page_movable - test page->mapping->flags to identify balloon pages
- *			  that can be moved by compaction/migration.
- *
- * This function is used at core compaction's page isolation scheme, therefore
- * most pages exposed to it are not enlisted as balloon pages and so, to avoid
- * undesired side effects like racing against __free_pages(), we cannot afford
- * holding the page locked while testing page->mapping->flags here.
- *
- * As we might return false positives in the case of a balloon page being just
- * released under us, the page->mapping->flags need to be re-tested later,
- * under the proper page lock, at the functions that will be coping with the
- * balloon page case.
- */
-static inline bool balloon_page_movable(struct page *page)
+static inline void __balloon_page_clear(struct page *page)
 {
-	/*
-	 * Before dereferencing and testing mapping->flags, let's make sure
-	 * this is not a page that uses ->mapping in a different way
-	 */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) == 1)
-		return __is_movable_balloon_page(page);
-
-	return false;
+	VM_BUG_ON_PAGE(!balloon_page_movable(page), page);
+	atomic_set(&page->_mapcount, -1);
 }
 
 /*

@@ -170,10 +145,8 @@ static inline bool balloon_page_movable(struct page *page)
  */
 static inline bool isolated_balloon_page(struct page *page)
 {
-	/* Already isolated balloon pages, by default, have a raised refcount */
-	if (page_flags_cleared(page) && !page_mapped(page) &&
-	    page_count(page) >= 2)
-		return __is_movable_balloon_page(page);
+	if (balloon_page_movable(page) && page_count(page) > 1)
+		return true;
 
 	return false;
 }

@@ -193,6 +166,7 @@ static inline void balloon_page_insert(struct page *page,
 				       struct list_head *head)
 {
 	page->mapping = mapping;
+	__balloon_page_set(page);
 	list_add(&page->lru, head);
 }

@@ -207,6 +181,7 @@ static inline void balloon_page_insert(struct page *page,
 static inline void balloon_page_delete(struct page *page)
 {
 	page->mapping = NULL;
+	__balloon_page_clear(page);
 	list_del(&page->lru);
 }

mm/balloon_compaction.c +36 ~~-23~~

104 lines not on this page · raw message

Signature · 1 line

1.9.3