linux-kernel - Re: [PATCH] mm/migrate.c: also overwrite error when it is bigger than zero

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jan 2020 15:30:18 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Wei Yang <richardw.yang@...ux.intel.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/migrate.c: also overwrite error when it is bigger than zero

On Fri, Jan 17, 2020 at 2:27 PM Wei Yang <richardw.yang@...ux.intel.com> wrote:
>
> On Fri, Jan 17, 2020 at 03:45:34PM +0800, Wei Yang wrote:
> >If we get here after successfully adding page to list, err would be
> >the number of pages in the list.
> >
> >Current code has two problems:
> >
> >  * on success, 0 is not returned
> >  * on error, the real error code is not returned
> >
>
> Well, this breaks the user interface. User would receive 1 even the migration
> succeed.
>
> The change is introduced by e0153fc2c760 ("mm: move_pages: return valid node
> id in status if the page is already on the target node").

Yes, it may return a value which is > 0. But, it seems do_pages_move()
could return > 0 value even before this commit.

For example, if I read the code correctly, it would do:

If we already have some pages on the queue then
add_page_for_migration() return error, then do_move_pages_to_node() is
called, but it may return > 0 value (the number of pages that were
*not* migrated by migrate_pages()), then the code flow would just jump
to "out" and return the value. And, it may happen to be 1.

I'm not sure if it breaks the user interface since the behavior has
been existed for years, and it looks nobody complains about it. Maybe
glibc helps hide it or people just care if it is 0 and the status.

>
> >Signed-off-by: Wei Yang <richardw.yang@...ux.intel.com>
> >---
> > mm/migrate.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/mm/migrate.c b/mm/migrate.c
> >index 557da996b936..c3ef70de5876 100644
> >--- a/mm/migrate.c
> >+++ b/mm/migrate.c
> >@@ -1677,7 +1677,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >       err1 = do_move_pages_to_node(mm, &pagelist, current_node);
> >       if (!err1)
> >               err1 = store_status(status, start, current_node, i - start);
> >-      if (!err)
> >+      if (err >= 0)
> >               err = err1;
> > out:
> >       return err;
> >--
> >2.17.1
>
> --
> Wei Yang
> Help you, Help me
>