| Message ID | 1560797290-42267-1-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind | expand |
[Cc networking people - see a question about setsockopt below] On Tue 18-06-19 02:48:10, 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! What is the BUG_ON because I do not see any BUG_ON neither in v4.9 nor the latest stable/linux-4.9.y > invalid opcode: 0000 [#1] SMP KASAN [...] > 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 Ughh. Do I get it right that that this setsockopt allows an arbitrary contiguous memory allocation size to be requested by a unpriviledged user? Or am I missing something that restricts there any restriction? > 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 specifed 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 > d44d363f65780f2ac2ec672164555af54896d40d ("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 > 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. We don't have to check non-LRU movable > pages since just zsmalloc and virtio-baloon support this. And, they > should be not able to reach here. You are not checking whether the page is movable, right? You only rely on PageLRU check which is not really an equivalent thing. There are movable pages which are not LRU and also pages might be off LRU temporarily for many reasons so this could lead to false positives. So I do not think this fix is correct. Blowing up on a BUG_ON is definitely not a right thing to do but we should rely on migrate_pages to fail the migration and report the failure based on that. > With this change the above test would return -EIO as expected. > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > --- > include/linux/mempolicy.h | 3 ++- > mm/mempolicy.c | 22 +++++++++++++++++----- > 2 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h > index 5228c62..cce7ba3 100644 > --- a/include/linux/mempolicy.h > +++ b/include/linux/mempolicy.h > @@ -198,7 +198,8 @@ static inline bool vma_migratable(struct vm_area_struct *vma) > if (vma->vm_file && > gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) > < policy_zone) > - return false; > + return false; > + Any reason to make this change? > return true; > } > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 2219e74..4d9e17d 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 { > @@ -467,7 +467,9 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, > goto unlock; > } > > - migrate_page_add(page, qp->pagelist, flags); > + ret = migrate_page_add(page, qp->pagelist, flags); > + if (ret) > + goto unlock; > } else > ret = -EIO; > unlock: > @@ -521,7 +523,9 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr, > if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { > if (!vma_migratable(vma)) > break; > - migrate_page_add(page, qp->pagelist, flags); > + ret = migrate_page_add(page, qp->pagelist, flags); > + if (ret) > + break; > } else > break; > } > @@ -940,10 +944,15 @@ 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); > + > + /* Non-movable page may reach here. */ > + if (!PageLRU(head)) > + return -EIO; > + > /* > * Avoid migrating a page that is shared with others. > */ > @@ -955,6 +964,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist, > hpage_nr_pages(head)); > } > } > + > + return 0; > } > > /* page allocation callback for NUMA node migration */ > @@ -1157,9 +1168,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, > -- > 1.8.3.1 >
On 6/18/19 6:02 AM, Michal Hocko wrote: > [Cc networking people - see a question about setsockopt below] > > On Tue 18-06-19 02:48:10, 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! > What is the BUG_ON because I do not see any BUG_ON neither in v4.9 nor > the latest stable/linux-4.9.y The line number might be not exactly same with upstream 4.9 since there might be some our internal patches. It is line 2096 at mm/huge_memory.c in 4.9.182. > >> invalid opcode: 0000 [#1] SMP KASAN > [...] >> 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 > Ughh. Do I get it right that that this setsockopt allows an arbitrary > contiguous memory allocation size to be requested by a unpriviledged > user? Or am I missing something that restricts there any restriction? It needs CAP_NET_RAW to call socket() to set socket type to RAW. The test is run by root user. > >> 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 specifed 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 >> d44d363f65780f2ac2ec672164555af54896d40d ("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 >> 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. We don't have to check non-LRU movable >> pages since just zsmalloc and virtio-baloon support this. And, they >> should be not able to reach here. > You are not checking whether the page is movable, right? You only rely > on PageLRU check which is not really an equivalent thing. There are > movable pages which are not LRU and also pages might be off LRU > temporarily for many reasons so this could lead to false positives. I'm supposed non-LRU movable pages could not reach here. Since most of them are not mmapable, i.e. virtio-balloon, zsmalloc. zram device is mmapable, but the page fault to that vma would end up allocating user space pages which are on LRU. If I miss something please let me know. The migrate_page_add() also just checks PageLRU(), non-LRU pages will be *not* put on the migration list at all. See migrate_page_add() -> isolate_lru_page(). > So I do not think this fix is correct. Blowing up on a BUG_ON is > definitely not a right thing to do but we should rely on migrate_pages > to fail the migration and report the failure based on that. The BUG_ON was removed by commit d44d363f65780f2ac2ec672164555af54896d40d ("mm: don't assume anonymous pages have SwapBacked flag") since 4.12. Actually, those pages will *not* be put on the migration list at all since they are !PageLRU. So, we can't rely on migrate_pages(). This is why I added the check in migrate_page_add(). > >> With this change the above test would return -EIO as expected. >> >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >> --- >> include/linux/mempolicy.h | 3 ++- >> mm/mempolicy.c | 22 +++++++++++++++++----- >> 2 files changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h >> index 5228c62..cce7ba3 100644 >> --- a/include/linux/mempolicy.h >> +++ b/include/linux/mempolicy.h >> @@ -198,7 +198,8 @@ static inline bool vma_migratable(struct vm_area_struct *vma) >> if (vma->vm_file && >> gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) >> < policy_zone) >> - return false; >> + return false; >> + > Any reason to make this change? Just a indent fix by hand. > >> return true; >> } >> >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> index 2219e74..4d9e17d 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 { >> @@ -467,7 +467,9 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, >> goto unlock; >> } >> >> - migrate_page_add(page, qp->pagelist, flags); >> + ret = migrate_page_add(page, qp->pagelist, flags); >> + if (ret) >> + goto unlock; >> } else >> ret = -EIO; >> unlock: >> @@ -521,7 +523,9 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr, >> if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { >> if (!vma_migratable(vma)) >> break; >> - migrate_page_add(page, qp->pagelist, flags); >> + ret = migrate_page_add(page, qp->pagelist, flags); >> + if (ret) >> + break; >> } else >> break; >> } >> @@ -940,10 +944,15 @@ 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); >> + >> + /* Non-movable page may reach here. */ >> + if (!PageLRU(head)) >> + return -EIO; >> + >> /* >> * Avoid migrating a page that is shared with others. >> */ >> @@ -955,6 +964,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist, >> hpage_nr_pages(head)); >> } >> } >> + >> + return 0; >> } >> >> /* page allocation callback for NUMA node migration */ >> @@ -1157,9 +1168,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, >> -- >> 1.8.3.1 >>
On Tue 18-06-19 10:06:54, Yang Shi wrote: > > > On 6/18/19 6:02 AM, Michal Hocko wrote: > > [Cc networking people - see a question about setsockopt below] > > > > On Tue 18-06-19 02:48:10, 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! > > What is the BUG_ON because I do not see any BUG_ON neither in v4.9 nor > > the latest stable/linux-4.9.y > > The line number might be not exactly same with upstream 4.9 since there > might be some our internal patches. > > It is line 2096 at mm/huge_memory.c in 4.9.182. So it is VM_BUG_ON_PAGE(!PageSwapBacked(page), page); that is later mentioned that has been removed. Good. Thanks for the clarification! > > > invalid opcode: 0000 [#1] SMP KASAN > > [...] > > > 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 > > Ughh. Do I get it right that that this setsockopt allows an arbitrary > > contiguous memory allocation size to be requested by a unpriviledged > > user? Or am I missing something that restricts there any restriction? > > It needs CAP_NET_RAW to call socket() to set socket type to RAW. The test is > run by root user. OK, good. That is much better. I just didn't see the capability check. I can see one in packet_create but I do not see any in setsockopt. Maybe I just got lost in indirection or implied security model. [...] > > > Change migrate_page_add() to check if the page is movable or not, if it > > > is unmovable, just return -EIO. We don't have to check non-LRU movable > > > pages since just zsmalloc and virtio-baloon support this. And, they > > > should be not able to reach here. > > You are not checking whether the page is movable, right? You only rely > > on PageLRU check which is not really an equivalent thing. There are > > movable pages which are not LRU and also pages might be off LRU > > temporarily for many reasons so this could lead to false positives. > > I'm supposed non-LRU movable pages could not reach here. Since most of them > are not mmapable, i.e. virtio-balloon, zsmalloc. zram device is mmapable, > but the page fault to that vma would end up allocating user space pages > which are on LRU. If I miss something please let me know. That might be true right now but it is a very subtle assumption that might break easily in the future. The point is still that even LRU pages might be isolated from the LRU list temporarily and you do not want this to cause the failure easily.
On 6/18/19 11:28 AM, Michal Hocko wrote: > On Tue 18-06-19 10:06:54, Yang Shi wrote: >> >> On 6/18/19 6:02 AM, Michal Hocko wrote: >>> [Cc networking people - see a question about setsockopt below] >>> >>> On Tue 18-06-19 02:48:10, 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! >>> What is the BUG_ON because I do not see any BUG_ON neither in v4.9 nor >>> the latest stable/linux-4.9.y >> The line number might be not exactly same with upstream 4.9 since there >> might be some our internal patches. >> >> It is line 2096 at mm/huge_memory.c in 4.9.182. > So it is > VM_BUG_ON_PAGE(!PageSwapBacked(page), page); > that is later mentioned that has been removed. Good. Thanks for the > clarification! > >>>> invalid opcode: 0000 [#1] SMP KASAN >>> [...] >>>> 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 >>> Ughh. Do I get it right that that this setsockopt allows an arbitrary >>> contiguous memory allocation size to be requested by a unpriviledged >>> user? Or am I missing something that restricts there any restriction? >> It needs CAP_NET_RAW to call socket() to set socket type to RAW. The test is >> run by root user. > OK, good. That is much better. I just didn't see the capability check. I > can see one in packet_create but I do not see any in setsockopt. Maybe I > just got lost in indirection or implied security model. > > [...] >>>> Change migrate_page_add() to check if the page is movable or not, if it >>>> is unmovable, just return -EIO. We don't have to check non-LRU movable >>>> pages since just zsmalloc and virtio-baloon support this. And, they >>>> should be not able to reach here. >>> You are not checking whether the page is movable, right? You only rely >>> on PageLRU check which is not really an equivalent thing. There are >>> movable pages which are not LRU and also pages might be off LRU >>> temporarily for many reasons so this could lead to false positives. >> I'm supposed non-LRU movable pages could not reach here. Since most of them >> are not mmapable, i.e. virtio-balloon, zsmalloc. zram device is mmapable, >> but the page fault to that vma would end up allocating user space pages >> which are on LRU. If I miss something please let me know. > That might be true right now but it is a very subtle assumption that > might break easily in the future. The point is still that even LRU pages > might be isolated from the LRU list temporarily and you do not want this > to cause the failure easily. I used to have !__PageMovable(page), but it was removed since the aforementioned reason. I could add it back. For the temporary off LRU page, I did a quick search, it looks the most paths have to acquire mmap_sem, so it can't race with us here. Page reclaim/compaction looks like the only race. But, since the mapping should be preserved even though the page is off LRU temporarily unless the page is reclaimed, so we should be able to exclude temporary off LRU pages by calling page_mapping() and page_anon_vma(). So, the fix may look like: if (!PageLRU(head) && !__PageMovable(page)) { if (!(page_mapping(page) || page_anon_vma(page))) return -EIO; } >
On Tue 18-06-19 14:13:16, Yang Shi wrote: [...] > > > > > Change migrate_page_add() to check if the page is movable or not, if it > > > > > is unmovable, just return -EIO. We don't have to check non-LRU movable > > > > > pages since just zsmalloc and virtio-baloon support this. And, they > > > > > should be not able to reach here. > > > > You are not checking whether the page is movable, right? You only rely > > > > on PageLRU check which is not really an equivalent thing. There are > > > > movable pages which are not LRU and also pages might be off LRU > > > > temporarily for many reasons so this could lead to false positives. > > > I'm supposed non-LRU movable pages could not reach here. Since most of them > > > are not mmapable, i.e. virtio-balloon, zsmalloc. zram device is mmapable, > > > but the page fault to that vma would end up allocating user space pages > > > which are on LRU. If I miss something please let me know. > > That might be true right now but it is a very subtle assumption that > > might break easily in the future. The point is still that even LRU pages > > might be isolated from the LRU list temporarily and you do not want this > > to cause the failure easily. > > I used to have !__PageMovable(page), but it was removed since the > aforementioned reason. I could add it back. > > For the temporary off LRU page, I did a quick search, it looks the most > paths have to acquire mmap_sem, so it can't race with us here. Page > reclaim/compaction looks like the only race. But, since the mapping should > be preserved even though the page is off LRU temporarily unless the page is > reclaimed, so we should be able to exclude temporary off LRU pages by > calling page_mapping() and page_anon_vma(). > > So, the fix may look like: > > if (!PageLRU(head) && !__PageMovable(page)) { > if (!(page_mapping(page) || page_anon_vma(page))) > return -EIO; This is getting even more muddy TBH. Is there any reason that we have to handle this problem during the isolation phase rather the migration?
On 6/18/19 7:06 PM, Yang Shi wrote: > The BUG_ON was removed by commit > d44d363f65780f2ac2ec672164555af54896d40d ("mm: don't assume anonymous > pages have SwapBacked flag") since 4.12. Perhaps that commit should be sent to stable@ ? Although with VM_BUG_ON() this is less critical than plain BUG_ON().
On 6/19/19 7:21 AM, Michal Hocko wrote: > On Tue 18-06-19 14:13:16, Yang Shi wrote: > [...] >> >> I used to have !__PageMovable(page), but it was removed since the >> aforementioned reason. I could add it back. >> >> For the temporary off LRU page, I did a quick search, it looks the most >> paths have to acquire mmap_sem, so it can't race with us here. Page >> reclaim/compaction looks like the only race. But, since the mapping should >> be preserved even though the page is off LRU temporarily unless the page is >> reclaimed, so we should be able to exclude temporary off LRU pages by >> calling page_mapping() and page_anon_vma(). >> >> So, the fix may look like: >> >> if (!PageLRU(head) && !__PageMovable(page)) { >> if (!(page_mapping(page) || page_anon_vma(page))) >> return -EIO; > > This is getting even more muddy TBH. Is there any reason that we have to > handle this problem during the isolation phase rather the migration? I think it was already said that if pages can't be isolated, then migration phase won't process them, so they're just ignored. However I think the patch is wrong to abort immediately when encountering such page that cannot be isolated (AFAICS). IMHO it should still try to migrate everything it can, and only then return -EIO.
On 6/19/19 1:22 AM, Vlastimil Babka wrote: > On 6/19/19 7:21 AM, Michal Hocko wrote: >> On Tue 18-06-19 14:13:16, Yang Shi wrote: >> [...] >>> I used to have !__PageMovable(page), but it was removed since the >>> aforementioned reason. I could add it back. >>> >>> For the temporary off LRU page, I did a quick search, it looks the most >>> paths have to acquire mmap_sem, so it can't race with us here. Page >>> reclaim/compaction looks like the only race. But, since the mapping should >>> be preserved even though the page is off LRU temporarily unless the page is >>> reclaimed, so we should be able to exclude temporary off LRU pages by >>> calling page_mapping() and page_anon_vma(). >>> >>> So, the fix may look like: >>> >>> if (!PageLRU(head) && !__PageMovable(page)) { >>> if (!(page_mapping(page) || page_anon_vma(page))) >>> return -EIO; >> This is getting even more muddy TBH. Is there any reason that we have to >> handle this problem during the isolation phase rather the migration? > I think it was already said that if pages can't be isolated, then > migration phase won't process them, so they're just ignored. Yes，exactly. > However I think the patch is wrong to abort immediately when > encountering such page that cannot be isolated (AFAICS). IMHO it should > still try to migrate everything it can, and only then return -EIO. It is fine too. I don't see mbind semantics define how to handle such case other than returning -EIO.
On 6/19/19 1:18 AM, Vlastimil Babka wrote: > On 6/18/19 7:06 PM, Yang Shi wrote: >> The BUG_ON was removed by commit >> d44d363f65780f2ac2ec672164555af54896d40d ("mm: don't assume anonymous >> pages have SwapBacked flag") since 4.12. > Perhaps that commit should be sent to stable@ ? Although with > VM_BUG_ON() this is less critical than plain BUG_ON(). I don't think we have to. I agree it is less critical, VM_DEBUG should be not enabled for production environment. And, it doesn't actually break anything since split_huge_page would just return error, and those unmovable pages are silently ignored by isolate.
On 6/19/19 9:21 AM, Yang Shi wrote: > > > On 6/19/19 1:22 AM, Vlastimil Babka wrote: >> On 6/19/19 7:21 AM, Michal Hocko wrote: >>> On Tue 18-06-19 14:13:16, Yang Shi wrote: >>> [...] >>>> I used to have !__PageMovable(page), but it was removed since the >>>> aforementioned reason. I could add it back. >>>> >>>> For the temporary off LRU page, I did a quick search, it looks the >>>> most >>>> paths have to acquire mmap_sem, so it can't race with us here. Page >>>> reclaim/compaction looks like the only race. But, since the mapping >>>> should >>>> be preserved even though the page is off LRU temporarily unless the >>>> page is >>>> reclaimed, so we should be able to exclude temporary off LRU pages by >>>> calling page_mapping() and page_anon_vma(). >>>> >>>> So, the fix may look like: >>>> >>>> if (!PageLRU(head) && !__PageMovable(page)) { >>>> if (!(page_mapping(page) || page_anon_vma(page))) >>>> return -EIO; >>> This is getting even more muddy TBH. Is there any reason that we >>> have to >>> handle this problem during the isolation phase rather the migration? >> I think it was already said that if pages can't be isolated, then >> migration phase won't process them, so they're just ignored. > > Yes，exactly. > >> However I think the patch is wrong to abort immediately when >> encountering such page that cannot be isolated (AFAICS). IMHO it should >> still try to migrate everything it can, and only then return -EIO. > > It is fine too. I don't see mbind semantics define how to handle such > case other than returning -EIO. By looking into the code, it looks not that easy as what I thought. do_mbind() would check the return value of queue_pages_range(), it just applies the policy and manipulates vmas as long as the return value is 0 (success), then migrate pages on the list. We could put the movable pages on the list by not breaking immediately, but they will be ignored. If we migrate the pages regardless of the return value, it may break the policy since the policy will *not* be applied at all. > >
On 6/19/19 8:19 PM, Yang Shi wrote: >>>> This is getting even more muddy TBH. Is there any reason that we >>>> have to >>>> handle this problem during the isolation phase rather the migration? >>> I think it was already said that if pages can't be isolated, then >>> migration phase won't process them, so they're just ignored. >> >> Yes，exactly. >> >>> However I think the patch is wrong to abort immediately when >>> encountering such page that cannot be isolated (AFAICS). IMHO it should >>> still try to migrate everything it can, and only then return -EIO. >> >> It is fine too. I don't see mbind semantics define how to handle such >> case other than returning -EIO. I think it does. There's: If MPOL_MF_MOVE is specified in flags, then the kernel *will attempt to move all the existing pages* ... If MPOL_MF_STRICT is also specified, then the call fails with the error *EIO if some pages could not be moved* Aborting immediately would be against the attempt to move all. > By looking into the code, it looks not that easy as what I thought. > do_mbind() would check the return value of queue_pages_range(), it just > applies the policy and manipulates vmas as long as the return value is 0 > (success), then migrate pages on the list. We could put the movable > pages on the list by not breaking immediately, but they will be ignored. > If we migrate the pages regardless of the return value, it may break the > policy since the policy will *not* be applied at all. I think we just need to remember if there was at least one page that failed isolation or migration, but keep working, and in the end return EIO if there was such page(s). I don't think it breaks the policy. Once pages are allocated in a mapping, changing the policy is a best effort thing anyway. >> >> >
On 6/20/19 12:18 AM, Vlastimil Babka wrote: > On 6/19/19 8:19 PM, Yang Shi wrote: >>>>> This is getting even more muddy TBH. Is there any reason that we >>>>> have to >>>>> handle this problem during the isolation phase rather the migration? >>>> I think it was already said that if pages can't be isolated, then >>>> migration phase won't process them, so they're just ignored. >>> Yes，exactly. >>> >>>> However I think the patch is wrong to abort immediately when >>>> encountering such page that cannot be isolated (AFAICS). IMHO it should >>>> still try to migrate everything it can, and only then return -EIO. >>> It is fine too. I don't see mbind semantics define how to handle such >>> case other than returning -EIO. > I think it does. There's: > If MPOL_MF_MOVE is specified in flags, then the kernel *will attempt to > move all the existing pages* ... If MPOL_MF_STRICT is also specified, > then the call fails with the error *EIO if some pages could not be moved* > > Aborting immediately would be against the attempt to move all. > >> By looking into the code, it looks not that easy as what I thought. >> do_mbind() would check the return value of queue_pages_range(), it just >> applies the policy and manipulates vmas as long as the return value is 0 >> (success), then migrate pages on the list. We could put the movable >> pages on the list by not breaking immediately, but they will be ignored. >> If we migrate the pages regardless of the return value, it may break the >> policy since the policy will *not* be applied at all. > I think we just need to remember if there was at least one page that > failed isolation or migration, but keep working, and in the end return > EIO if there was such page(s). I don't think it breaks the policy. Once > pages are allocated in a mapping, changing the policy is a best effort > thing anyway. The current behavior is: If queue_pages_range() return -EIO (vma is not migratable, ignore other conditions since we just focus on page migration), the policy won't be set and no page will be migrated. However, the problem here is the vma might look migratable, but some or all the underlying pages are unmovable. So, my patch assumes the vma is *not* migratable if at least one page is unmovable. I'm not sure if it is possible to have both movable and unmovable pages for the same mapping or not, I'm supposed the vma would be split much earlier. If we don't abort immediately, then we record if there is unmovable page, then we could do: #1. Still follows the current behavior (then why not abort immediately?) #2. Set mempolicy then migrate all the migratable pages. But, we may end up with the pages on node A, but the policy says node B. Doesn't it break the policy? > >>>
On 6/20/19 6:08 PM, Yang Shi wrote: > > > On 6/20/19 12:18 AM, Vlastimil Babka wrote: >> On 6/19/19 8:19 PM, Yang Shi wrote: >>>>>> This is getting even more muddy TBH. Is there any reason that we >>>>>> have to >>>>>> handle this problem during the isolation phase rather the migration? >>>>> I think it was already said that if pages can't be isolated, then >>>>> migration phase won't process them, so they're just ignored. >>>> Yes，exactly. >>>> >>>>> However I think the patch is wrong to abort immediately when >>>>> encountering such page that cannot be isolated (AFAICS). IMHO it should >>>>> still try to migrate everything it can, and only then return -EIO. >>>> It is fine too. I don't see mbind semantics define how to handle such >>>> case other than returning -EIO. >> I think it does. There's: >> If MPOL_MF_MOVE is specified in flags, then the kernel *will attempt to >> move all the existing pages* ... If MPOL_MF_STRICT is also specified, >> then the call fails with the error *EIO if some pages could not be moved* >> >> Aborting immediately would be against the attempt to move all. >> >>> By looking into the code, it looks not that easy as what I thought. >>> do_mbind() would check the return value of queue_pages_range(), it just >>> applies the policy and manipulates vmas as long as the return value is 0 >>> (success), then migrate pages on the list. We could put the movable >>> pages on the list by not breaking immediately, but they will be ignored. >>> If we migrate the pages regardless of the return value, it may break the >>> policy since the policy will *not* be applied at all. >> I think we just need to remember if there was at least one page that >> failed isolation or migration, but keep working, and in the end return >> EIO if there was such page(s). I don't think it breaks the policy. Once >> pages are allocated in a mapping, changing the policy is a best effort >> thing anyway. > > The current behavior is: > If queue_pages_range() return -EIO (vma is not migratable, ignore other > conditions since we just focus on page migration), the policy won't be > set and no page will be migrated. Ah, I see. IIUC the current behavior is due to your recent commit a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when MPOL_MF_STRICT is specified") in order to fix commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()"), which caused -EIO to be not returned enough. But I think you went too far and instead return -EIO too much. If I look at the code in parent commit of 6f4576e3687b, I can see in queue_pages_range(): if ((flags & MPOL_MF_STRICT) || ((flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) && vma_migratable(vma))) { err = queue_pages_pgd_range(vma, start, endvma, nodes, flags, private); if (err) break; } and in queue_pages_pte_range(): if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) migrate_page_add(page, private, flags); else break; So originally, there was no returning of -EIO due to !vma_migratable() - as long as MPOL_MF_STRICT and MPOL_MF_MOVE* was specified, the code tried to queue for migration everything it could and didn't ever abort, AFAICS. And I still think that's the best possible behavior. > However, the problem here is the vma might look migratable, but some or > all the underlying pages are unmovable. So, my patch assumes the vma is > *not* migratable if at least one page is unmovable. I'm not sure if it > is possible to have both movable and unmovable pages for the same > mapping or not, I'm supposed the vma would be split much earlier. > > If we don't abort immediately, then we record if there is unmovable > page, then we could do: > #1. Still follows the current behavior (then why not abort immediately?) See above how the current behavior differs from the original one. > #2. Set mempolicy then migrate all the migratable pages. But, we may end > up with the pages on node A, but the policy says node B. Doesn't it > break the policy? The policy can already be "broken" (violated is probably better word) by migrate_pages() failing. If that happens, we don't rollback the migrated pages and reset the policy back, right? I think the manpage is clear that MPOL_MF_MOVE is a best-effort. 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. >> >>>> >
On 6/21/19 4:33 AM, Vlastimil Babka wrote: > On 6/20/19 6:08 PM, Yang Shi wrote: >> >> On 6/20/19 12:18 AM, Vlastimil Babka wrote: >>> On 6/19/19 8:19 PM, Yang Shi wrote: >>>>>>> This is getting even more muddy TBH. Is there any reason that we >>>>>>> have to >>>>>>> handle this problem during the isolation phase rather the migration? >>>>>> I think it was already said that if pages can't be isolated, then >>>>>> migration phase won't process them, so they're just ignored. >>>>> Yes，exactly. >>>>> >>>>>> However I think the patch is wrong to abort immediately when >>>>>> encountering such page that cannot be isolated (AFAICS). IMHO it should >>>>>> still try to migrate everything it can, and only then return -EIO. >>>>> It is fine too. I don't see mbind semantics define how to handle such >>>>> case other than returning -EIO. >>> I think it does. There's: >>> If MPOL_MF_MOVE is specified in flags, then the kernel *will attempt to >>> move all the existing pages* ... If MPOL_MF_STRICT is also specified, >>> then the call fails with the error *EIO if some pages could not be moved* >>> >>> Aborting immediately would be against the attempt to move all. >>> >>>> By looking into the code, it looks not that easy as what I thought. >>>> do_mbind() would check the return value of queue_pages_range(), it just >>>> applies the policy and manipulates vmas as long as the return value is 0 >>>> (success), then migrate pages on the list. We could put the movable >>>> pages on the list by not breaking immediately, but they will be ignored. >>>> If we migrate the pages regardless of the return value, it may break the >>>> policy since the policy will *not* be applied at all. >>> I think we just need to remember if there was at least one page that >>> failed isolation or migration, but keep working, and in the end return >>> EIO if there was such page(s). I don't think it breaks the policy. Once >>> pages are allocated in a mapping, changing the policy is a best effort >>> thing anyway. >> The current behavior is: >> If queue_pages_range() return -EIO (vma is not migratable, ignore other >> conditions since we just focus on page migration), the policy won't be >> set and no page will be migrated. > Ah, I see. IIUC the current behavior is due to your recent commit > a7f40cfe3b7a ("mm: mempolicy: make mbind() return -EIO when > MPOL_MF_STRICT is specified") in order to fix commit 6f4576e3687b > ("mempolicy: apply page table walker on queue_pages_range()"), which > caused -EIO to be not returned enough. But I think you went too far and > instead return -EIO too much. If I look at the code in parent commit of > 6f4576e3687b, I can see in queue_pages_range(): > > if ((flags & MPOL_MF_STRICT) || > ((flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) && > vma_migratable(vma))) { > > err = queue_pages_pgd_range(vma, start, endvma, nodes, > flags, private); > if (err) > break; > } > > and in queue_pages_pte_range(): > > if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) > migrate_page_add(page, private, flags); > else > break; > > So originally, there was no returning of -EIO due to !vma_migratable() - > as long as MPOL_MF_STRICT and MPOL_MF_MOVE* was specified, the code > tried to queue for migration everything it could and didn't ever abort, > AFAICS. And I still think that's the best possible behavior. > >> However, the problem here is the vma might look migratable, but some or >> all the underlying pages are unmovable. So, my patch assumes the vma is >> *not* migratable if at least one page is unmovable. I'm not sure if it >> is possible to have both movable and unmovable pages for the same >> mapping or not, I'm supposed the vma would be split much earlier. >> >> If we don't abort immediately, then we record if there is unmovable >> page, then we could do: >> #1. Still follows the current behavior (then why not abort immediately?) > See above how the current behavior differs from the original one. > >> #2. Set mempolicy then migrate all the migratable pages. But, we may end >> up with the pages on node A, but the policy says node B. Doesn't it >> break the policy? > The policy can already be "broken" (violated is probably better word) by > migrate_pages() failing. If that happens, we don't rollback the migrated > pages and reset the policy back, right? I think the manpage is clear > that MPOL_MF_MOVE is a best-effort. 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. I see your point. It makes some sense to me. So, the policy should be set if MPOL_MF_MOVE* is specified even though no page is migrated so that we have consistent behavior for different cases: * vma is not migratable * vma is migratable, but pages are unmovable * vma is migratable, pages are movable, but migrate_pages() fails >
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h index 5228c62..cce7ba3 100644 --- a/include/linux/mempolicy.h +++ b/include/linux/mempolicy.h @@ -198,7 +198,8 @@ static inline bool vma_migratable(struct vm_area_struct *vma) if (vma->vm_file && gfp_zone(mapping_gfp_mask(vma->vm_file->f_mapping)) < policy_zone) - return false; + return false; + return true; } diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 2219e74..4d9e17d 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 { @@ -467,7 +467,9 @@ static int queue_pages_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr, goto unlock; } - migrate_page_add(page, qp->pagelist, flags); + ret = migrate_page_add(page, qp->pagelist, flags); + if (ret) + goto unlock; } else ret = -EIO; unlock: @@ -521,7 +523,9 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr, if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) { if (!vma_migratable(vma)) break; - migrate_page_add(page, qp->pagelist, flags); + ret = migrate_page_add(page, qp->pagelist, flags); + if (ret) + break; } else break; } @@ -940,10 +944,15 @@ 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); + + /* Non-movable page may reach here. */ + if (!PageLRU(head)) + return -EIO; + /* * Avoid migrating a page that is shared with others. */ @@ -955,6 +964,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist, hpage_nr_pages(head)); } } + + return 0; } /* page allocation callback for NUMA node migration */ @@ -1157,9 +1168,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 specifed 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 d44d363f65780f2ac2ec672164555af54896d40d ("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 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. We don't have to check non-LRU movable pages since just zsmalloc and virtio-baloon support this. And, they should be not able to reach here. With this change the above test would return -EIO as expected. Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- include/linux/mempolicy.h | 3 ++- mm/mempolicy.c | 22 +++++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-)