mm: compaction: buffer overflow in isolate_migratepages_range

mm: compaction: buffer overflow in isolate_migratepages_range

[Sasha Levin]

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

mm: compaction: buffer overflow in isolate_migratepages_range

[Sasha Levin]

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

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Andrey Ryabinin]

2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> 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

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:

--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }

-- 
Best regards,
Andrey Ryabinin

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Andrey Ryabinin]

2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> 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

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:

--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }

-- 
Best regards,
Andrey Ryabinin

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Sun, Aug 10, 2014 at 12:49:47PM +0400, Andrey Ryabinin wrote:
> 2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> > 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!

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

 
> 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(). 
 
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Sun, Aug 10, 2014 at 12:49:47PM +0400, Andrey Ryabinin wrote:
> 2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> > 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!

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

 
> 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(). 
 
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -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 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>

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Wed, Aug 13, 2014 at 12:35:02PM -0300, Rafael Aquini wrote:
> On Sun, Aug 10, 2014 at 12:49:47PM +0400, Andrey Ryabinin wrote:
> > 2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> > > 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!
> 
> > 
> > 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.
>

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.

 
> 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 

>  
> > 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(). 
>  
> > --- a/include/linux/balloon_compaction.h
> > +++ b/include/linux/balloon_compaction.h
> > @@ -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 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>

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Wed, Aug 13, 2014 at 12:35:02PM -0300, Rafael Aquini wrote:
> On Sun, Aug 10, 2014 at 12:49:47PM +0400, Andrey Ryabinin wrote:
> > 2014-08-10 5:45 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> > > 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!
> 
> > 
> > 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.
>

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.

 
> 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 

>  
> > 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(). 
>  
> > --- a/include/linux/balloon_compaction.h
> > +++ b/include/linux/balloon_compaction.h
> > @@ -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 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>

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Andrey Ryabinin]

2014-08-14 19:13 GMT+04:00 Rafael Aquini <aquini@redhat.com>:
>> 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.
>
>
>> 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.
>

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.


-- 
Best regards,
Andrey Ryabinin

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Andrey Ryabinin]

2014-08-14 19:13 GMT+04:00 Rafael Aquini <aquini@redhat.com>:
>> 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.
>
>
>> 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.
>

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.


-- 
Best regards,
Andrey Ryabinin

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> 2014-08-14 19:13 GMT+04:00 Rafael Aquini <aquini@redhat.com>:
> > 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? 


> 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 ;)

 
> 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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> 2014-08-14 19:13 GMT+04:00 Rafael Aquini <aquini@redhat.com>:
> > 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? 


> 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 ;)

 
> 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

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> 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 <aquini@redhat.com>
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(-)

diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
index 089743a..1409ccc 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }
 
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 6e45a50..1cfb254 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -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
+	 * 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)) {
 		/*
-		 * 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;
 }
-- 
1.9.3

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> 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 <aquini@redhat.com>
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(-)

diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
index 089743a..1409ccc 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }
 
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 6e45a50..1cfb254 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -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
+	 * 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)) {
 		/*
-		 * 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;
 }
-- 
1.9.3

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 06:43:50PM -0300, Rafael Aquini wrote:
> On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> > 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 <aquini@redhat.com>
> 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(-)
> 
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 089743a..1409ccc 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -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);
>  }
>  
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 6e45a50..1cfb254 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -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
> +	 * 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.


>  		/*
> -		 * 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;
>  }
> -- 
> 1.9.3
> 

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Thu, Aug 14, 2014 at 06:43:50PM -0300, Rafael Aquini wrote:
> On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> > 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 <aquini@redhat.com>
> 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(-)
> 
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 089743a..1409ccc 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -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);
>  }
>  
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 6e45a50..1cfb254 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -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
> +	 * 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.


>  		/*
> -		 * 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;
>  }
> -- 
> 1.9.3
> 

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Konstantin Khlebnikov]

On Fri, Aug 15, 2014 at 2:07 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Thu, Aug 14, 2014 at 06:43:50PM -0300, Rafael Aquini wrote:
>> On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
>> > 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 <aquini@redhat.com>
>> 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(-)
>>
>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
>> index 089743a..1409ccc 100644
>> --- a/include/linux/balloon_compaction.h
>> +++ b/include/linux/balloon_compaction.h
>> @@ -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);
>>  }
>>
>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
>> index 6e45a50..1cfb254 100644
>> --- a/mm/balloon_compaction.c
>> +++ b/mm/balloon_compaction.c
>> @@ -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
>> +      * 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.

>
>
>>               /*
>> -              * 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;
>>  }
>> --
>> 1.9.3
>>

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Konstantin Khlebnikov]

On Fri, Aug 15, 2014 at 2:07 AM, Rafael Aquini <aquini@redhat.com> wrote:
> On Thu, Aug 14, 2014 at 06:43:50PM -0300, Rafael Aquini wrote:
>> On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
>> > 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 <aquini@redhat.com>
>> 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(-)
>>
>> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
>> index 089743a..1409ccc 100644
>> --- a/include/linux/balloon_compaction.h
>> +++ b/include/linux/balloon_compaction.h
>> @@ -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);
>>  }
>>
>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
>> index 6e45a50..1cfb254 100644
>> --- a/mm/balloon_compaction.c
>> +++ b/mm/balloon_compaction.c
>> @@ -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
>> +      * 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.

>
>
>>               /*
>> -              * 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;
>>  }
>> --
>> 1.9.3
>>

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Fri, Aug 15, 2014 at 07:36:16AM +0400, Konstantin Khlebnikov wrote:
> 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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[Rafael Aquini]

On Fri, Aug 15, 2014 at 07:36:16AM +0400, Konstantin Khlebnikov wrote:
> 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

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

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[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 <aquini@redhat.com>
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.

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.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 59 ++++++++++++++++++++++--------------
 2 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
index 089743a..e00d5b0 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }
 
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 6e45a50..84fe746 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -209,6 +209,27 @@ static inline int __migrate_balloon_page(struct address_space *mapping,
 	return page->mapping->a_ops->migratepage(mapping, newpage, page, mode);
 }
 
+static inline bool __is_balloon_page_isolated(struct page *page)
+{
+	/*
+	 * A ballooned page, by default, holds just one refcount that
+	 * get increased by balloon_page_isolate() upon compaction demand.
+	 * We can prevent concurrent compaction threads from (re)isolating
+	 * an already isolated balloon page by refcount check.
+	 */
+	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);
+			return true;
+		} else {
+			dump_page(page, "not movable balloon page");
+			WARN_ON(1);
+		}
+	}
+	return false;
+}
+
 /* __isolate_lru_page() counterpart for a ballooned page */
 bool balloon_page_isolate(struct page *page)
 {
@@ -221,27 +242,17 @@ bool balloon_page_isolate(struct page *page)
 	 * the put_page() at the end of this block will take care of
 	 * release this page, thus avoiding a nasty leakage.
 	 */
-	if (likely(get_page_unless_zero(page))) {
+	if (get_page_unless_zero(page)) {
 		/*
 		 * 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.
+		 * The aforementioned operations are done under the safety of
+		 * page lock, so lets be sure we hold it before proceeding the
+		 * isolation steps here.
 		 */
-		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) {
-				__isolate_balloon_page(page);
+		if (trylock_page(page)) {
+			if (__is_balloon_page_isolated(page)) {
 				unlock_page(page);
 				return true;
 			}
@@ -255,19 +266,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 +299,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;
 }
-- 
1.9.3

Re: mm: compaction: buffer overflow in isolate_migratepages_range

[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 <aquini@redhat.com>
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.

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.

Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
 include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
 mm/balloon_compaction.c            | 59 ++++++++++++++++++++++--------------
 2 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
index 089743a..e00d5b0 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -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);
 }
 
diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 6e45a50..84fe746 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -209,6 +209,27 @@ static inline int __migrate_balloon_page(struct address_space *mapping,
 	return page->mapping->a_ops->migratepage(mapping, newpage, page, mode);
 }
 
+static inline bool __is_balloon_page_isolated(struct page *page)
+{
+	/*
+	 * A ballooned page, by default, holds just one refcount that
+	 * get increased by balloon_page_isolate() upon compaction demand.
+	 * We can prevent concurrent compaction threads from (re)isolating
+	 * an already isolated balloon page by refcount check.
+	 */
+	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);
+			return true;
+		} else {
+			dump_page(page, "not movable balloon page");
+			WARN_ON(1);
+		}
+	}
+	return false;
+}
+
 /* __isolate_lru_page() counterpart for a ballooned page */
 bool balloon_page_isolate(struct page *page)
 {
@@ -221,27 +242,17 @@ bool balloon_page_isolate(struct page *page)
 	 * the put_page() at the end of this block will take care of
 	 * release this page, thus avoiding a nasty leakage.
 	 */
-	if (likely(get_page_unless_zero(page))) {
+	if (get_page_unless_zero(page)) {
 		/*
 		 * 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.
+		 * The aforementioned operations are done under the safety of
+		 * page lock, so lets be sure we hold it before proceeding the
+		 * isolation steps here.
 		 */
-		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) {
-				__isolate_balloon_page(page);
+		if (trylock_page(page)) {
+			if (__is_balloon_page_isolated(page)) {
 				unlock_page(page);
 				return true;
 			}
@@ -255,19 +266,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 +299,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;
 }
-- 
1.9.3

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