lkml.org 
[lkml]   [2019]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind
From
Date


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

\
 
 \ /
  Last update: 2019-06-18 19:08    [W:2.606 / U:1.752 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site