| Message ID | 1563556862-54056-3-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: mempolicy: fix mbind()'s inconsistent behavior for unmovable pages | expand |
On 7/19/19 7:21 PM, Yang Shi wrote: > When running syzkaller internally, we ran into the below bug on 4.9.x > kernel: > > kernel BUG at mm/huge_memory.c:2124! > invalid opcode: 0000 [#1] SMP KASAN > Dumping ftrace buffer: > (ftrace buffer empty) > Modules linked in: > CPU: 0 PID: 1518 Comm: syz-executor107 Not tainted 4.9.168+ #2 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.5.1 01/01/2011 > task: ffff880067b34900 task.stack: ffff880068998000 > RIP: 0010:[<ffffffff81895d6b>] [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124 > RSP: 0018:ffff88006899f980 EFLAGS: 00010286 > RAX: 0000000000000000 RBX: ffffea00018f1700 RCX: 0000000000000000 > RDX: 1ffffd400031e2e7 RSI: 0000000000000001 RDI: ffffea00018f1738 > RBP: ffff88006899f9e8 R08: 0000000000000001 R09: 0000000000000000 > R10: 0000000000000000 R11: fffffbfff0d8b13e R12: ffffea00018f1400 > R13: ffffea00018f1400 R14: ffffea00018f1720 R15: ffffea00018f1401 > FS: 00007fa333996740(0000) GS:ffff88006c600000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000020000040 CR3: 0000000066b9c000 CR4: 00000000000606f0 > Stack: > 0000000000000246 ffff880067b34900 0000000000000000 ffff88007ffdc000 > 0000000000000000 ffff88006899f9e8 ffffffff812b4015 ffff880064c64e18 > ffffea00018f1401 dffffc0000000000 ffffea00018f1700 0000000020ffd000 > Call Trace: > [<ffffffff818490f1>] split_huge_page include/linux/huge_mm.h:100 [inline] > [<ffffffff818490f1>] queue_pages_pte_range+0x7e1/0x1480 mm/mempolicy.c:538 > [<ffffffff817ed0da>] walk_pmd_range mm/pagewalk.c:50 [inline] > [<ffffffff817ed0da>] walk_pud_range mm/pagewalk.c:90 [inline] > [<ffffffff817ed0da>] walk_pgd_range mm/pagewalk.c:116 [inline] > [<ffffffff817ed0da>] __walk_page_range+0x44a/0xdb0 mm/pagewalk.c:208 > [<ffffffff817edb94>] walk_page_range+0x154/0x370 mm/pagewalk.c:285 > [<ffffffff81844515>] queue_pages_range+0x115/0x150 mm/mempolicy.c:694 > [<ffffffff8184f493>] do_mbind mm/mempolicy.c:1241 [inline] > [<ffffffff8184f493>] SYSC_mbind+0x3c3/0x1030 mm/mempolicy.c:1370 > [<ffffffff81850146>] SyS_mbind+0x46/0x60 mm/mempolicy.c:1352 > [<ffffffff810097e2>] do_syscall_64+0x1d2/0x600 arch/x86/entry/common.c:282 > [<ffffffff82ff6f93>] entry_SYSCALL_64_after_swapgs+0x5d/0xdb > Code: c7 80 1c 02 00 e8 26 0a 76 01 <0f> 0b 48 c7 c7 40 46 45 84 e8 4c > RIP [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124 > RSP <ffff88006899f980> > > with the below test: > > ---8<--- > > uint64_t r[1] = {0xffffffffffffffff}; > > int main(void) > { > syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0); > intptr_t res = 0; > res = syscall(__NR_socket, 0x11, 3, 0x300); > if (res != -1) > r[0] = res; > *(uint32_t*)0x20000040 = 0x10000; > *(uint32_t*)0x20000044 = 1; > *(uint32_t*)0x20000048 = 0xc520; > *(uint32_t*)0x2000004c = 1; > syscall(__NR_setsockopt, r[0], 0x107, 0xd, 0x20000040, 0x10); > syscall(__NR_mmap, 0x20fed000, 0x10000, 0, 0x8811, r[0], 0); > *(uint64_t*)0x20000340 = 2; > syscall(__NR_mbind, 0x20ff9000, 0x4000, 0x4002, 0x20000340, > 0x45d4, 3); > return 0; > } > > ---8<--- > > Actually the test does: > > mmap(0x20000000, 16777216, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000 > socket(AF_PACKET, SOCK_RAW, 768) = 3 > setsockopt(3, SOL_PACKET, PACKET_TX_RING, {block_size=65536, block_nr=1, frame_size=50464, frame_nr=1}, 16) = 0 > mmap(0x20fed000, 65536, PROT_NONE, MAP_SHARED|MAP_FIXED|MAP_POPULATE|MAP_DENYWRITE, 3, 0) = 0x20fed000 > mbind(..., MPOL_MF_STRICT|MPOL_MF_MOVE) = 0 > > The setsockopt() would allocate compound pages (16 pages in this test) > for packet tx ring, then the mmap() would call packet_mmap() to map the > pages into the user address space specified by the mmap() call. > > When calling mbind(), it would scan the vma to queue the pages for > migration to the new node. It would split any huge page since 4.9 > doesn't support THP migration, however, the packet tx ring compound > pages are not THP and even not movable. So, the above bug is triggered. > > However, the later kernel is not hit by this issue due to the > commit d44d363f65780f2ac2 ("mm: don't assume anonymous pages have > SwapBacked flag"), which just removes the PageSwapBacked check for a > different reason. > > But, there is a deeper issue. According to the semantic of mbind(), it > should return -EIO if MPOL_MF_MOVE or MPOL_MF_MOVE_ALL was specified and > MPOL_MF_STRICT was also specified, but the kernel was unable to move > all existing pages in the range. The tx ring of the packet socket is > definitely not movable, however, mbind() returns success for this case. > > Although the most socket file associates with non-movable pages, but XDP > may have movable pages from gup. So, it sounds not fine to just check > the underlying file type of vma in vma_migratable(). > > Change migrate_page_add() to check if the page is movable or not, if it > is unmovable, just return -EIO. But do not abort pte walk immediately, > since there may be pages off LRU temporarily. We should migrate other > pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some > paged could not be not moved, then return -EIO for mbind() eventually. > > With this change the above test would return -EIO as expected. > > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mel Gorman <mgorman@techsingularity.net> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks!
On Mon, 22 Jul 2019 09:25:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > > since there may be pages off LRU temporarily. We should migrate other > > pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some > > paged could not be not moved, then return -EIO for mbind() eventually. > > > > With this change the above test would return -EIO as expected. > > > > Cc: Vlastimil Babka <vbabka@suse.cz> > > Cc: Michal Hocko <mhocko@suse.com> > > Cc: Mel Gorman <mgorman@techsingularity.net> > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks. I'm a bit surprised that this doesn't have a cc:stable. Did we consider that? Also, is this patch dependent upon "mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified"? Doesn't look that way.. Also, I have a note that you had concerns with "mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified". What is the status now? From: Yang Shi <yang.shi@linux.alibaba.com> Subject: mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should try best to migrate misplaced pages, if some of the pages could not be migrated, then return -EIO. There are three different sub-cases: 1. vma is not migratable 2. vma is migratable, but there are unmovable pages 3. vma is migratable, pages are movable, but migrate_pages() fails If #1 happens, kernel would just abort immediately, then return -EIO, after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified"). If #3 happens, kernel would set policy and migrate pages with best-effort, but won't rollback the migrated pages and reset the policy back. Before that commit, they behaves in the same way. It'd better to keep their behavior consistent. But, rolling back the migrated pages and resetting the policy back sounds not feasible, so just make #1 behave as same as #3. Userspace will know that not everything was successfully migrated (via -EIO), and can take whatever steps it deems necessary - attempt rollback, determine which exact page(s) are violating the policy, etc. Make queue_pages_range() return 1 to indicate there are unmovable pages or vma is not migratable. The #2 is not handled correctly in the current kernel, the following patch will fix it. Link: http://lkml.kernel.org/r/1561162809-59140-2-git-send-email-yang.shi@linux.alibaba.com Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Michal Hocko <mhocko@suse.com> Cc: Mel Gorman <mgorman@techsingularity.net> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/mempolicy.c | 84 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 24 deletions(-) --- a/mm/mempolicy.c~mm-mempolicy-make-the-behavior-consistent-when-mpol_mf_move-and-mpol_mf_strict-were-specified +++ a/mm/mempolicy.c @@ -429,11 +429,14 @@ static inline bool queue_pages_required( } /* - * queue_pages_pmd() has three possible return values: + * queue_pages_pmd() has four possible return values: + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were + * specified. * 1 - pages are placed on the right node or queued successfully. * 0 - THP was split. - * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing - * page was already on a node that does not follow the policy. + * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an + * existing page was already on a node that does not follow the + * policy. */ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, unsigned long end, struct mm_walk *walk) @@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, s /* go to thp migration */ if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { if (!vma_migratable(walk->vma)) { - ret = -EIO; + ret = 2; goto unlock; } @@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t * struct queue_pages *qp = walk->private; unsigned long flags = qp->flags; int ret; + bool has_unmovable = false; pte_t *pte; spinlock_t *ptl; ptl = pmd_trans_huge_lock(pmd, vma); if (ptl) { ret = queue_pages_pmd(pmd, ptl, addr, end, walk); - if (ret > 0) + switch (ret) { + /* THP was split, fall through to pte walk */ + case 0: + break; + /* Pages are placed on the right node or queued successfully */ + case 1: return 0; - else if (ret < 0) + /* + * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT + * were specified. + */ + case 2: + return 1; + case -EIO: return ret; + } } if (pmd_trans_unstable(pmd)) @@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t * if (!queue_pages_required(page, qp)) continue; if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { - if (!vma_migratable(vma)) + /* MPOL_MF_STRICT must be specified if we get here */ + if (!vma_migratable(vma)) { + has_unmovable |= true; break; + } migrate_page_add(page, qp->pagelist, flags); } else break; } pte_unmap_unlock(pte - 1, ptl); cond_resched(); + + if (has_unmovable) + return 1; + return addr != end ? -EIO : 0; } @@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigne * * If pages found in a given range are on a set of nodes (determined by * @nodes and @flags,) it's isolated and queued to the pagelist which is - * passed via @private.) + * passed via @private. + * + * queue_pages_range() has three possible return values: + * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were + * specified. + * 0 - queue pages successfully or no misplaced page. + * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified. */ static int queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, @@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start struct mempolicy *new; unsigned long end; int err; + int ret; LIST_HEAD(pagelist); if (flags & ~(unsigned long)MPOL_MF_VALID) @@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start if (err) goto mpol_out; - err = queue_pages_range(mm, start, end, nmask, + ret = queue_pages_range(mm, start, end, nmask, flags | MPOL_MF_INVERT, &pagelist); - if (!err) - err = mbind_range(mm, start, end, new); - if (!err) { - int nr_failed = 0; + if (ret < 0) + err = -EIO; + else { + err = mbind_range(mm, start, end, new); - if (!list_empty(&pagelist)) { - WARN_ON_ONCE(flags & MPOL_MF_LAZY); - nr_failed = migrate_pages(&pagelist, new_page, NULL, - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); - if (nr_failed) - putback_movable_pages(&pagelist); - } + if (!err) { + int nr_failed = 0; - if (nr_failed && (flags & MPOL_MF_STRICT)) - err = -EIO; - } else - putback_movable_pages(&pagelist); + if (!list_empty(&pagelist)) { + WARN_ON_ONCE(flags & MPOL_MF_LAZY); + nr_failed = migrate_pages(&pagelist, new_page, + NULL, start, MIGRATE_SYNC, + MR_MEMPOLICY_MBIND); + if (nr_failed) + putback_movable_pages(&pagelist); + } + + if ((ret > 0) || + (nr_failed && (flags & MPOL_MF_STRICT))) + err = -EIO; + } else + putback_movable_pages(&pagelist); + } up_write(&mm->mmap_sem); mpol_out:
On 7/22/19 6:02 PM, Andrew Morton wrote: > On Mon, 22 Jul 2019 09:25:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >>> since there may be pages off LRU temporarily. We should migrate other >>> pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some >>> paged could not be not moved, then return -EIO for mbind() eventually. >>> >>> With this change the above test would return -EIO as expected. >>> >>> Cc: Vlastimil Babka <vbabka@suse.cz> >>> Cc: Michal Hocko <mhocko@suse.com> >>> Cc: Mel Gorman <mgorman@techsingularity.net> >>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > Thanks. > > I'm a bit surprised that this doesn't have a cc:stable. Did we > consider that? The VM_BUG just happens on 4.9, and it is enabled only by CONFIG_VM. For post-4.9 kernel, this fixes the semantics of mbind which should be not a regression IMHO. > > Also, is this patch dependent upon "mm: mempolicy: make the behavior > consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified"? > Doesn't look that way.. No, it depends on patch #1. > > Also, I have a note that you had concerns with "mm: mempolicy: make the > behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were > specified". What is the status now? Vlastimil had given his Reviewed-by. > > > From: Yang Shi <yang.shi@linux.alibaba.com> > Subject: mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified > > When both MPOL_MF_MOVE* and MPOL_MF_STRICT was specified, mbind() should > try best to migrate misplaced pages, if some of the pages could not be > migrated, then return -EIO. > > There are three different sub-cases: > 1. vma is not migratable > 2. vma is migratable, but there are unmovable pages > 3. vma is migratable, pages are movable, but migrate_pages() fails > > If #1 happens, kernel would just abort immediately, then return -EIO, > after the commit a7f40cfe3b7ada57af9b62fd28430eeb4a7cfcb7 ("mm: mempolicy: > make mbind() return -EIO when MPOL_MF_STRICT is specified"). > > If #3 happens, kernel would set policy and migrate pages with best-effort, > but won't rollback the migrated pages and reset the policy back. > > Before that commit, they behaves in the same way. It'd better to keep > their behavior consistent. But, rolling back the migrated pages and > resetting the policy back sounds not feasible, so just make #1 behave as > same as #3. > > Userspace will know that not everything was successfully migrated (via > -EIO), and can take whatever steps it deems necessary - attempt rollback, > determine which exact page(s) are violating the policy, etc. > > Make queue_pages_range() return 1 to indicate there are unmovable pages or > vma is not migratable. > > The #2 is not handled correctly in the current kernel, the following patch > will fix it. > > Link: http://lkml.kernel.org/r/1561162809-59140-2-git-send-email-yang.shi@linux.alibaba.com > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mel Gorman <mgorman@techsingularity.net> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > mm/mempolicy.c | 84 +++++++++++++++++++++++++++++++++-------------- > 1 file changed, 60 insertions(+), 24 deletions(-) > > --- a/mm/mempolicy.c~mm-mempolicy-make-the-behavior-consistent-when-mpol_mf_move-and-mpol_mf_strict-were-specified > +++ a/mm/mempolicy.c > @@ -429,11 +429,14 @@ static inline bool queue_pages_required( > } > > /* > - * queue_pages_pmd() has three possible return values: > + * queue_pages_pmd() has four possible return values: > + * 2 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were > + * specified. > * 1 - pages are placed on the right node or queued successfully. > * 0 - THP was split. > - * -EIO - is migration entry or MPOL_MF_STRICT was specified and an existing > - * page was already on a node that does not follow the policy. > + * -EIO - is migration entry or only MPOL_MF_STRICT was specified and an > + * existing page was already on a node that does not follow the > + * policy. > */ > static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, > unsigned long end, struct mm_walk *walk) > @@ -463,7 +466,7 @@ static int queue_pages_pmd(pmd_t *pmd, s > /* go to thp migration */ > if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { > if (!vma_migratable(walk->vma)) { > - ret = -EIO; > + ret = 2; > goto unlock; > } > > @@ -488,16 +491,29 @@ static int queue_pages_pte_range(pmd_t * > struct queue_pages *qp = walk->private; > unsigned long flags = qp->flags; > int ret; > + bool has_unmovable = false; > pte_t *pte; > spinlock_t *ptl; > > ptl = pmd_trans_huge_lock(pmd, vma); > if (ptl) { > ret = queue_pages_pmd(pmd, ptl, addr, end, walk); > - if (ret > 0) > + switch (ret) { > + /* THP was split, fall through to pte walk */ > + case 0: > + break; > + /* Pages are placed on the right node or queued successfully */ > + case 1: > return 0; > - else if (ret < 0) > + /* > + * Met unmovable pages, MPOL_MF_MOVE* & MPOL_MF_STRICT > + * were specified. > + */ > + case 2: > + return 1; > + case -EIO: > return ret; > + } > } > > if (pmd_trans_unstable(pmd)) > @@ -519,14 +535,21 @@ static int queue_pages_pte_range(pmd_t * > if (!queue_pages_required(page, qp)) > continue; > if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { > - if (!vma_migratable(vma)) > + /* MPOL_MF_STRICT must be specified if we get here */ > + if (!vma_migratable(vma)) { > + has_unmovable |= true; > break; > + } > migrate_page_add(page, qp->pagelist, flags); > } else > break; > } > pte_unmap_unlock(pte - 1, ptl); > cond_resched(); > + > + if (has_unmovable) > + return 1; > + > return addr != end ? -EIO : 0; > } > > @@ -639,7 +662,13 @@ static int queue_pages_test_walk(unsigne > * > * If pages found in a given range are on a set of nodes (determined by > * @nodes and @flags,) it's isolated and queued to the pagelist which is > - * passed via @private.) > + * passed via @private. > + * > + * queue_pages_range() has three possible return values: > + * 1 - there is unmovable page, but MPOL_MF_MOVE* & MPOL_MF_STRICT were > + * specified. > + * 0 - queue pages successfully or no misplaced page. > + * -EIO - there is misplaced page and only MPOL_MF_STRICT was specified. > */ > static int > queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, > @@ -1182,6 +1211,7 @@ static long do_mbind(unsigned long start > struct mempolicy *new; > unsigned long end; > int err; > + int ret; > LIST_HEAD(pagelist); > > if (flags & ~(unsigned long)MPOL_MF_VALID) > @@ -1243,26 +1273,32 @@ static long do_mbind(unsigned long start > if (err) > goto mpol_out; > > - err = queue_pages_range(mm, start, end, nmask, > + ret = queue_pages_range(mm, start, end, nmask, > flags | MPOL_MF_INVERT, &pagelist); > - if (!err) > - err = mbind_range(mm, start, end, new); > > - if (!err) { > - int nr_failed = 0; > + if (ret < 0) > + err = -EIO; > + else { > + err = mbind_range(mm, start, end, new); > > - if (!list_empty(&pagelist)) { > - WARN_ON_ONCE(flags & MPOL_MF_LAZY); > - nr_failed = migrate_pages(&pagelist, new_page, NULL, > - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); > - if (nr_failed) > - putback_movable_pages(&pagelist); > - } > + if (!err) { > + int nr_failed = 0; > > - if (nr_failed && (flags & MPOL_MF_STRICT)) > - err = -EIO; > - } else > - putback_movable_pages(&pagelist); > + if (!list_empty(&pagelist)) { > + WARN_ON_ONCE(flags & MPOL_MF_LAZY); > + nr_failed = migrate_pages(&pagelist, new_page, > + NULL, start, MIGRATE_SYNC, > + MR_MEMPOLICY_MBIND); > + if (nr_failed) > + putback_movable_pages(&pagelist); > + } > + > + if ((ret > 0) || > + (nr_failed && (flags & MPOL_MF_STRICT))) > + err = -EIO; > + } else > + putback_movable_pages(&pagelist); > + } > > up_write(&mm->mmap_sem); > mpol_out: > _
On 7/23/19 7:35 AM, Yang Shi wrote: > > > On 7/22/19 6:02 PM, Andrew Morton wrote: >> On Mon, 22 Jul 2019 09:25:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: >> >>>> since there may be pages off LRU temporarily. We should migrate other >>>> pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some >>>> paged could not be not moved, then return -EIO for mbind() eventually. >>>> >>>> With this change the above test would return -EIO as expected. >>>> >>>> Cc: Vlastimil Babka <vbabka@suse.cz> >>>> Cc: Michal Hocko <mhocko@suse.com> >>>> Cc: Mel Gorman <mgorman@techsingularity.net> >>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> Thanks. >> >> I'm a bit surprised that this doesn't have a cc:stable. Did we >> consider that? > > The VM_BUG just happens on 4.9, and it is enabled only by CONFIG_VM. For > post-4.9 kernel, this fixes the semantics of mbind which should be not a > regression IMHO. 4.9 is a LTS kernel, so perhaps worth trying? >> >> Also, is this patch dependent upon "mm: mempolicy: make the behavior >> consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified"? >> Doesn't look that way.. > > No, it depends on patch #1. > >> >> Also, I have a note that you had concerns with "mm: mempolicy: make the >> behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were >> specified". What is the status now? > > Vlastimil had given his Reviewed-by. Yes, the concerns were resolved.
On Wed, 24 Jul 2019 10:19:34 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > On 7/23/19 7:35 AM, Yang Shi wrote: > > > > > > On 7/22/19 6:02 PM, Andrew Morton wrote: > >> On Mon, 22 Jul 2019 09:25:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >>>> since there may be pages off LRU temporarily. We should migrate other > >>>> pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some > >>>> paged could not be not moved, then return -EIO for mbind() eventually. > >>>> > >>>> With this change the above test would return -EIO as expected. > >>>> > >>>> Cc: Vlastimil Babka <vbabka@suse.cz> > >>>> Cc: Michal Hocko <mhocko@suse.com> > >>>> Cc: Mel Gorman <mgorman@techsingularity.net> > >>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > >>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > >> Thanks. > >> > >> I'm a bit surprised that this doesn't have a cc:stable. Did we > >> consider that? > > > > The VM_BUG just happens on 4.9, and it is enabled only by CONFIG_VM. For > > post-4.9 kernel, this fixes the semantics of mbind which should be not a > > regression IMHO. > > 4.9 is a LTS kernel, so perhaps worth trying? > OK, I'll add cc:stable to mm-mempolicy-make-the-behavior-consistent-when-mpol_mf_move-and-mpol_mf_strict-were-specified.patch and mm-mempolicy-handle-vma-with-unmovable-pages-mapped-correctly-in-mbind.patch Do we have a Fixes: for these patches?
On 7/24/19 5:44 PM, Andrew Morton wrote: > On Wed, 24 Jul 2019 10:19:34 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 7/23/19 7:35 AM, Yang Shi wrote: >>> >>> On 7/22/19 6:02 PM, Andrew Morton wrote: >>>> On Mon, 22 Jul 2019 09:25:09 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: >>>> >>>>>> since there may be pages off LRU temporarily. We should migrate other >>>>>> pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some >>>>>> paged could not be not moved, then return -EIO for mbind() eventually. >>>>>> >>>>>> With this change the above test would return -EIO as expected. >>>>>> >>>>>> Cc: Vlastimil Babka <vbabka@suse.cz> >>>>>> Cc: Michal Hocko <mhocko@suse.com> >>>>>> Cc: Mel Gorman <mgorman@techsingularity.net> >>>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>>>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >>>> Thanks. >>>> >>>> I'm a bit surprised that this doesn't have a cc:stable. Did we >>>> consider that? >>> The VM_BUG just happens on 4.9, and it is enabled only by CONFIG_VM. For >>> post-4.9 kernel, this fixes the semantics of mbind which should be not a >>> regression IMHO. >> 4.9 is a LTS kernel, so perhaps worth trying? >> > OK, I'll add cc:stable to Thanks. > > mm-mempolicy-make-the-behavior-consistent-when-mpol_mf_move-and-mpol_mf_strict-were-specified.patch > > and > > mm-mempolicy-handle-vma-with-unmovable-pages-mapped-correctly-in-mbind.patch > > Do we have a Fixes: for these patches? It looks the problem has existed since very beginning. The oldest commit which I can find is dc9aa5b9d65fd11b1f5246b46ec610ee8b83c6dd ("[PATCH] Swap Migration V5: MPOL_MF_MOVE interface"), which is a 2.6.16 commit.
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 932c268..547cd40 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -403,7 +403,7 @@ void mpol_rebind_mm(struct mm_struct *mm, nodemask_t *new) }, }; -static void migrate_page_add(struct page *page, struct list_head *pagelist, +static int migrate_page_add(struct page *page, struct list_head *pagelist, unsigned long flags); struct queue_pages { @@ -463,12 +463,11 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, flags = qp->flags; /* go to thp migration */ if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { - if (!vma_migratable(walk->vma)) { + if (!vma_migratable(walk->vma) || + migrate_page_add(page, qp->pagelist, flags)) { ret = 1; goto unlock; } - - migrate_page_add(page, qp->pagelist, flags); } else ret = -EIO; unlock: @@ -532,7 +531,14 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr, has_unmovable = true; break; } - migrate_page_add(page, qp->pagelist, flags); + + /* + * Do not abort immediately since there may be + * temporary off LRU pages in the range. Still + * need migrate other LRU pages. + */ + if (migrate_page_add(page, qp->pagelist, flags)) + has_unmovable = true; } else break; } @@ -961,7 +967,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask, /* * page migration, thp tail pages can be passed. */ -static void migrate_page_add(struct page *page, struct list_head *pagelist, +static int migrate_page_add(struct page *page, struct list_head *pagelist, unsigned long flags) { struct page *head = compound_head(page); @@ -974,8 +980,19 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist, mod_node_page_state(page_pgdat(head), NR_ISOLATED_ANON + page_is_file_cache(head), hpage_nr_pages(head)); + } else if (flags & MPOL_MF_STRICT) { + /* + * Non-movable page may reach here. And, there may be + * temporary off LRU pages or non-LRU movable pages. + * Treat them as unmovable pages since they can't be + * isolated, so they can't be moved at the moment. It + * should return -EIO for this case too. + */ + return -EIO; } } + + return 0; } /* page allocation callback for NUMA node migration */ @@ -1178,9 +1195,10 @@ static struct page *new_page(struct page *page, unsigned long start) } #else -static void migrate_page_add(struct page *page, struct list_head *pagelist, +static int migrate_page_add(struct page *page, struct list_head *pagelist, unsigned long flags) { + return -EIO; } int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
When running syzkaller internally, we ran into the below bug on 4.9.x kernel: kernel BUG at mm/huge_memory.c:2124! invalid opcode: 0000 [#1] SMP KASAN Dumping ftrace buffer: (ftrace buffer empty) Modules linked in: CPU: 0 PID: 1518 Comm: syz-executor107 Not tainted 4.9.168+ #2 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.5.1 01/01/2011 task: ffff880067b34900 task.stack: ffff880068998000 RIP: 0010:[<ffffffff81895d6b>] [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124 RSP: 0018:ffff88006899f980 EFLAGS: 00010286 RAX: 0000000000000000 RBX: ffffea00018f1700 RCX: 0000000000000000 RDX: 1ffffd400031e2e7 RSI: 0000000000000001 RDI: ffffea00018f1738 RBP: ffff88006899f9e8 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: fffffbfff0d8b13e R12: ffffea00018f1400 R13: ffffea00018f1400 R14: ffffea00018f1720 R15: ffffea00018f1401 FS: 00007fa333996740(0000) GS:ffff88006c600000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000040 CR3: 0000000066b9c000 CR4: 00000000000606f0 Stack: 0000000000000246 ffff880067b34900 0000000000000000 ffff88007ffdc000 0000000000000000 ffff88006899f9e8 ffffffff812b4015 ffff880064c64e18 ffffea00018f1401 dffffc0000000000 ffffea00018f1700 0000000020ffd000 Call Trace: [<ffffffff818490f1>] split_huge_page include/linux/huge_mm.h:100 [inline] [<ffffffff818490f1>] queue_pages_pte_range+0x7e1/0x1480 mm/mempolicy.c:538 [<ffffffff817ed0da>] walk_pmd_range mm/pagewalk.c:50 [inline] [<ffffffff817ed0da>] walk_pud_range mm/pagewalk.c:90 [inline] [<ffffffff817ed0da>] walk_pgd_range mm/pagewalk.c:116 [inline] [<ffffffff817ed0da>] __walk_page_range+0x44a/0xdb0 mm/pagewalk.c:208 [<ffffffff817edb94>] walk_page_range+0x154/0x370 mm/pagewalk.c:285 [<ffffffff81844515>] queue_pages_range+0x115/0x150 mm/mempolicy.c:694 [<ffffffff8184f493>] do_mbind mm/mempolicy.c:1241 [inline] [<ffffffff8184f493>] SYSC_mbind+0x3c3/0x1030 mm/mempolicy.c:1370 [<ffffffff81850146>] SyS_mbind+0x46/0x60 mm/mempolicy.c:1352 [<ffffffff810097e2>] do_syscall_64+0x1d2/0x600 arch/x86/entry/common.c:282 [<ffffffff82ff6f93>] entry_SYSCALL_64_after_swapgs+0x5d/0xdb Code: c7 80 1c 02 00 e8 26 0a 76 01 <0f> 0b 48 c7 c7 40 46 45 84 e8 4c RIP [<ffffffff81895d6b>] split_huge_page_to_list+0x8fb/0x1030 mm/huge_memory.c:2124 RSP <ffff88006899f980> with the below test: ---8<--- uint64_t r[1] = {0xffffffffffffffff}; int main(void) { syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0); intptr_t res = 0; res = syscall(__NR_socket, 0x11, 3, 0x300); if (res != -1) r[0] = res; *(uint32_t*)0x20000040 = 0x10000; *(uint32_t*)0x20000044 = 1; *(uint32_t*)0x20000048 = 0xc520; *(uint32_t*)0x2000004c = 1; syscall(__NR_setsockopt, r[0], 0x107, 0xd, 0x20000040, 0x10); syscall(__NR_mmap, 0x20fed000, 0x10000, 0, 0x8811, r[0], 0); *(uint64_t*)0x20000340 = 2; syscall(__NR_mbind, 0x20ff9000, 0x4000, 0x4002, 0x20000340, 0x45d4, 3); return 0; } ---8<--- Actually the test does: mmap(0x20000000, 16777216, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000 socket(AF_PACKET, SOCK_RAW, 768) = 3 setsockopt(3, SOL_PACKET, PACKET_TX_RING, {block_size=65536, block_nr=1, frame_size=50464, frame_nr=1}, 16) = 0 mmap(0x20fed000, 65536, PROT_NONE, MAP_SHARED|MAP_FIXED|MAP_POPULATE|MAP_DENYWRITE, 3, 0) = 0x20fed000 mbind(..., MPOL_MF_STRICT|MPOL_MF_MOVE) = 0 The setsockopt() would allocate compound pages (16 pages in this test) for packet tx ring, then the mmap() would call packet_mmap() to map the pages into the user address space specified by the mmap() call. When calling mbind(), it would scan the vma to queue the pages for migration to the new node. It would split any huge page since 4.9 doesn't support THP migration, however, the packet tx ring compound pages are not THP and even not movable. So, the above bug is triggered. However, the later kernel is not hit by this issue due to the commit d44d363f65780f2ac2 ("mm: don't assume anonymous pages have SwapBacked flag"), which just removes the PageSwapBacked check for a different reason. But, there is a deeper issue. According to the semantic of mbind(), it should return -EIO if MPOL_MF_MOVE or MPOL_MF_MOVE_ALL was specified and MPOL_MF_STRICT was also specified, but the kernel was unable to move all existing pages in the range. The tx ring of the packet socket is definitely not movable, however, mbind() returns success for this case. Although the most socket file associates with non-movable pages, but XDP may have movable pages from gup. So, it sounds not fine to just check the underlying file type of vma in vma_migratable(). Change migrate_page_add() to check if the page is movable or not, if it is unmovable, just return -EIO. But do not abort pte walk immediately, since there may be pages off LRU temporarily. We should migrate other pages if MPOL_MF_MOVE* is specified. Set has_unmovable flag if some paged could not be not moved, then return -EIO for mbind() eventually. With this change the above test would return -EIO as expected. Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Michal Hocko <mhocko@suse.com> Cc: Mel Gorman <mgorman@techsingularity.net> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- mm/mempolicy.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)