| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Dec 2019 14:00:31 -0800
From: Yang Shi <yang.shi@...ux.alibaba.com>
To: John Hubbard <jhubbard@...dia.com>, fabecassis@...dia.com,
mhocko@...e.com, cl@...ux.com, vbabka@...e.cz,
mgorman@...hsingularity.net, akpm@...ux-foundation.org
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [v3 PATCH] mm: move_pages: return valid node id in status if the
page is already on the target node
On 12/5/19 1:27 PM, John Hubbard wrote:
> On 12/5/19 10:54 AM, Yang Shi wrote:
>> Felix Abecassis reports move_pages() would return random status if the
>> pages are already on the target node by the below test program:
>>
>> ---8<---
>>
>> int main(void)
>> {
>> const long node_id = 1;
>> const long page_size = sysconf(_SC_PAGESIZE);
>> const int64_t num_pages = 8;
>>
>> unsigned long nodemask = 1 << node_id;
>> long ret = set_mempolicy(MPOL_BIND, &nodemask, sizeof(nodemask));
>> if (ret < 0)
>> return (EXIT_FAILURE);
>>
>> void **pages = malloc(sizeof(void*) * num_pages);
>> for (int i = 0; i < num_pages; ++i) {
>> pages[i] = mmap(NULL, page_size, PROT_WRITE | PROT_READ,
>> MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS,
>> -1, 0);
>> if (pages[i] == MAP_FAILED)
>> return (EXIT_FAILURE);
>> }
>>
>> ret = set_mempolicy(MPOL_DEFAULT, NULL, 0);
>> if (ret < 0)
>> return (EXIT_FAILURE);
>>
>> int *nodes = malloc(sizeof(int) * num_pages);
>> int *status = malloc(sizeof(int) * num_pages);
>> for (int i = 0; i < num_pages; ++i) {
>> nodes[i] = node_id;
>> status[i] = 0xd0; /* simulate garbage values */
>> }
>>
>> ret = move_pages(0, num_pages, pages, nodes, status, MPOL_MF_MOVE);
>> printf("move_pages: %ld\n", ret);
>> for (int i = 0; i < num_pages; ++i)
>> printf("status[%d] = %d\n", i, status[i]);
>> }
>> ---8<---
>>
>> Then running the program would return nonsense status values:
>> $ ./move_pages_bug
>> move_pages: 0
>> status[0] = 208
>> status[1] = 208
>> status[2] = 208
>> status[3] = 208
>> status[4] = 208
>> status[5] = 208
>> status[6] = 208
>> status[7] = 208
>>
>> This is because the status is not set if the page is already on the
>> target node, but move_pages() should return valid status as long as it
>> succeeds. The valid status may be errno or node id.
>>
>> We can't simply initialize status array to zero since the pages may be
>> not on node 0. Fix it by updating status with node id which the page is
>> already on.
>>
>> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
>> Reported-by: Felix Abecassis <fabecassis@...dia.com>
>> Tested-by: Felix Abecassis <fabecassis@...dia.com>
>> Suggested-by: Michal Hocko <mhocko@...e.com>
>> Cc: John Hubbard <jhubbard@...dia.com>
>> Cc: Christoph Lameter <cl@...ux.com>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Mel Gorman <mgorman@...hsingularity.net>
>> Cc: <stable@...r.kernel.org> 4.17+
>> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
>> ---
>> v3: * Adopted the suggestion from Michal.
>> v2: * Correted the return value when add_page_for_migration() returns 1.
>>
>> mm/migrate.c | 24 ++++++++++++++++++------
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index a8f87cb..9c172f4 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1512,9 +1512,11 @@ static int do_move_pages_to_node(struct mm_struct *mm,
>> /*
>> * Resolves the given address to a struct page, isolates it from the LRU and
>> * puts it to the given pagelist.
>> - * Returns -errno if the page cannot be found/isolated or 0 when it has been
>> - * queued or the page doesn't need to be migrated because it is already on
>> - * the target node
>> + * Returns:
>> + * errno - if the page cannot be found/isolated
>> + * 0 - when it doesn't have to be migrated because it is already on the
>> + * target node
>> + * 1 - when it has been queued
>> */
>> static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>> int node, struct list_head *pagelist, bool migrate_all)
>> @@ -1553,7 +1555,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>> if (PageHuge(page)) {
>> if (PageHead(page)) {
>> isolate_huge_page(page, pagelist);
>> - err = 0;
>> + err = 1;
>> }
>> } else {
>> struct page *head;
>> @@ -1563,7 +1565,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>> if (err)
>> goto out_putpage;
>>
>> - err = 0;
>> + err = 1;
>> list_add_tail(&head->lru, pagelist);
>> mod_node_page_state(page_pgdat(head),
>> NR_ISOLATED_ANON + page_is_file_cache(head),
>> @@ -1578,6 +1580,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>> put_page(page);
>> out:
>> up_read(&mm->mmap_sem);
>> +
>> return err;
>> }
>>
>> @@ -1640,8 +1643,17 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>> */
>> err = add_page_for_migration(mm, addr, current_node,
>> &pagelist, flags & MPOL_MF_MOVE_ALL);
>> - if (!err)
>> +
>> + /* The page is already on the target node */
>> + if (!err) {
>> + err = store_status(status, i, current_node, 1);
>> + if (err)
>> + goto out_flush;
>> continue;
>> + /* The page is successfully queued */
> Nit: could you move this comment down one line, so that it's clear that it
> applies to the "else" branch? Like this:
>
> } else if (err > 0) {
> /* The page is successfully queued for migration */
> continue;
> }
Sure.
>
>
>> + } else if (err > 0) {
>> + continue;
>> + }
>>
>> err = store_status(status, i, err, 1);
>> if (err)
>>
> Also agree with Christopher's requests, but all of these don't affect the
> correct operation of the patch, so you can add:
>
> Reviewed-by: John Hubbard <jhubbard@...dia.com>
>
> thanks,
Powered by blists - more mailing lists