Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 24 Jan 2020 08:21:27 +0100
From:   Michal Hocko <mhocko@kernel.org>
To:     Wei Yang <richardw.yang@linux.intel.com>
Cc:     akpm@linux-foundation.org, yang.shi@linux.alibaba.com,
        jhubbard@nvidia.com, vbabka@suse.cz, cl@linux.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger
 than zero
Message-ID: <20200124072127.GO29276@dhcp22.suse.cz>
References: <20200119065753.21694-1-richardw.yang@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200119065753.21694-1-richardw.yang@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

[Sorry I have missed this patch previously]

On Sun 19-01-20 14:57:53, Wei Yang wrote:
> If we get here after successfully adding page to list, err would be
> 1 to indicate the page is queued in the list.
> 
> Current code has two problems:
> 
>   * on success, 0 is not returned
>   * on error, if add_page_for_migratioin() return 1, and the following err1
>     from do_move_pages_to_node() is set, the err1 is not returned since err
>     is 1

This made my really scratch my head to grasp. So essentially err > 0
will happen when we reach the end of the loop and rely on the
out_flush flushing to migrate the batch. Then err contains the
add_page_for_migratioin return value. And that would leak to the
userspace.

What would you say about the following wording instead?
"
out_flush part of do_pages_move is responsible for migrating the last
batch that accumulated while processing the input in the loop.
do_move_pages_to_node return value is supposed to override any
preexisting error (e.g. when the user input is garbage) but the current
logic is wrong because add_page_for_migration returns 1 when
successfully adding a page into the batch and therefore this will be the
last err value after the loop is processed without any actual error.
We want to override that value of course because do_pages_move would
return 1 to the userspace even without any errors.
"

> And these behaviors break the user interface.
> 
> Fixes: e0153fc2c760 ("mm: move_pages: return valid node id in status if the
> page is already on the target node").
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> 
> ---
> v2:
>   * put more words to explain the error case
> ---
>  mm/migrate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 86873b6f38a7..430fdccc733e 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1676,7 +1676,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
> 

-- 
Michal Hocko
SUSE Labs