* [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
@ 2020-01-19 6:57 Wei Yang
2020-01-21 1:53 ` Wei Yang
2020-01-24 7:21 ` Michal Hocko
0 siblings, 2 replies; 12+ messages in thread
From: Wei Yang @ 2020-01-19 6:57 UTC (permalink / raw)
To: akpm, yang.shi, jhubbard, vbabka, cl; +Cc: linux-mm, linux-kernel, Wei Yang
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
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>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-19 6:57 [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero Wei Yang
@ 2020-01-21 1:53 ` Wei Yang
2020-01-21 2:34 ` Wei Yang
2020-01-21 19:30 ` Yang Shi
2020-01-24 7:21 ` Michal Hocko
1 sibling, 2 replies; 12+ messages in thread
From: Wei Yang @ 2020-01-21 1:53 UTC (permalink / raw)
To: Wei Yang
Cc: akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel, mhocko
On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>
>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>
>
>---
>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;
Ok, as mentioned by Yang and Michal, only err == 0 means no error.
Sounds this regression should be fixed in another place. Let me send out
another patch.
> out:
> return err;
>--
>2.17.1
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-21 1:53 ` Wei Yang
@ 2020-01-21 2:34 ` Wei Yang
2020-01-21 19:33 ` John Hubbard
2020-01-21 19:30 ` Yang Shi
1 sibling, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-01-21 2:34 UTC (permalink / raw)
To: Wei Yang
Cc: akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel, mhocko
On Tue, Jan 21, 2020 at 09:53:26AM +0800, Wei Yang wrote:
>On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>>
>>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>
>>
>>---
>>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;
>
>Ok, as mentioned by Yang and Michal, only err == 0 means no error.
>
>Sounds this regression should be fixed in another place. Let me send out
>another patch.
>
Hmm... I took another look into the case, this fix should work.
But yes, the semantic here is a little confusion. Look forward your comments
here.
>> out:
>> return err;
>>--
>>2.17.1
>
>--
>Wei Yang
>Help you, Help me
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-21 1:53 ` Wei Yang
2020-01-21 2:34 ` Wei Yang
@ 2020-01-21 19:30 ` Yang Shi
2020-01-22 0:41 ` Wei Yang
1 sibling, 1 reply; 12+ messages in thread
From: Yang Shi @ 2020-01-21 19:30 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, jhubbard, vbabka, cl, linux-mm, linux-kernel, mhocko
On 1/20/20 5:53 PM, Wei Yang wrote:
> On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>>
>> 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>
>>
>> ---
>> 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;
> Ok, as mentioned by Yang and Michal, only err == 0 means no error.
I think Michal means do_move_pages_to_node() only,
add_page_for_migration() returns 1 on purpose.
But, actually the syscall may still return > 0 value since err1 might be
> 0. Anyway, this is another regression. The patch itself looks correct
to me.
Acked-by: Yang Shi <yang.shi@linux.alibaba.com>
>
> Sounds this regression should be fixed in another place. Let me send out
> another patch.
>
>> out:
>> return err;
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-21 2:34 ` Wei Yang
@ 2020-01-21 19:33 ` John Hubbard
2020-01-22 0:42 ` Wei Yang
0 siblings, 1 reply; 12+ messages in thread
From: John Hubbard @ 2020-01-21 19:33 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, yang.shi, vbabka, cl, linux-mm, linux-kernel, mhocko
On 1/20/20 6:34 PM, Wei Yang wrote:
> On Tue, Jan 21, 2020 at 09:53:26AM +0800, Wei Yang wrote:
>> On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>>>
>>> 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").
The Fixes tag should be different, right? Because I don't think that
commit introduced this problem.
thanks,
--
John Hubbard
NVIDIA
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.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;
>>
>> Ok, as mentioned by Yang and Michal, only err == 0 means no error.
>>
>> Sounds this regression should be fixed in another place. Let me send out
>> another patch.
>>
>
> Hmm... I took another look into the case, this fix should work.
>
> But yes, the semantic here is a little confusion. Look forward your comments
> here.
>
>>> out:
>>> return err;
>>> --
>>> 2.17.1
>>
>> --
>> Wei Yang
>> Help you, Help me
>
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-21 19:30 ` Yang Shi
@ 2020-01-22 0:41 ` Wei Yang
0 siblings, 0 replies; 12+ messages in thread
From: Wei Yang @ 2020-01-22 0:41 UTC (permalink / raw)
To: Yang Shi
Cc: Wei Yang, akpm, jhubbard, vbabka, cl, linux-mm, linux-kernel, mhocko
On Tue, Jan 21, 2020 at 11:30:03AM -0800, Yang Shi wrote:
>
>
>On 1/20/20 5:53 PM, Wei Yang wrote:
>> On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>> >
>> > 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>
>> >
>> > ---
>> > 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;
>> Ok, as mentioned by Yang and Michal, only err == 0 means no error.
>
>I think Michal means do_move_pages_to_node() only, add_page_for_migration()
>returns 1 on purpose.
>
>But, actually the syscall may still return > 0 value since err1 might be > 0.
>Anyway, this is another regression. The patch itself looks correct to me.
>
>Acked-by: Yang Shi <yang.shi@linux.alibaba.com>
>
Thanks
>
>>
>> Sounds this regression should be fixed in another place. Let me send out
>> another patch.
>>
>> > out:
>> > return err;
>> > --
>> > 2.17.1
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-21 19:33 ` John Hubbard
@ 2020-01-22 0:42 ` Wei Yang
2020-01-22 1:29 ` John Hubbard
0 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-01-22 0:42 UTC (permalink / raw)
To: John Hubbard
Cc: Wei Yang, akpm, yang.shi, vbabka, cl, linux-mm, linux-kernel, mhocko
On Tue, Jan 21, 2020 at 11:33:16AM -0800, John Hubbard wrote:
>On 1/20/20 6:34 PM, Wei Yang wrote:
>> On Tue, Jan 21, 2020 at 09:53:26AM +0800, Wei Yang wrote:
>> > On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>> > >
>> > > 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").
>
>The Fixes tag should be different, right? Because I don't think that
>commit introduced this problem.
This is the correct one.
Before this, we don't return 1 from add_page_for_migration().
>
>thanks,
>--
>John Hubbard
>NVIDIA
>
>> > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.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;
>> >
>> > Ok, as mentioned by Yang and Michal, only err == 0 means no error.
>> >
>> > Sounds this regression should be fixed in another place. Let me send out
>> > another patch.
>> >
>>
>> Hmm... I took another look into the case, this fix should work.
>>
>> But yes, the semantic here is a little confusion. Look forward your comments
>> here.
>>
>> > > out:
>> > > return err;
>> > > --
>> > > 2.17.1
>> >
>> > --
>> > Wei Yang
>> > Help you, Help me
>>
>
>thanks,
>--
>John Hubbard
>NVIDIA
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-22 0:42 ` Wei Yang
@ 2020-01-22 1:29 ` John Hubbard
0 siblings, 0 replies; 12+ messages in thread
From: John Hubbard @ 2020-01-22 1:29 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, yang.shi, vbabka, cl, linux-mm, linux-kernel, mhocko
On 1/21/20 4:42 PM, Wei Yang wrote:
> On Tue, Jan 21, 2020 at 11:33:16AM -0800, John Hubbard wrote:
>> On 1/20/20 6:34 PM, Wei Yang wrote:
>>> On Tue, Jan 21, 2020 at 09:53:26AM +0800, Wei Yang wrote:
>>>> On Sun, Jan 19, 2020 at 02:57:53PM +0800, 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
>>>>>
>>>>> 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").
>>
>> The Fixes tag should be different, right? Because I don't think that
>> commit introduced this problem.
>
> This is the correct one.
>
> Before this, we don't return 1 from add_page_for_migration().
>
OK, then. :)
thanks,
--
John Hubbard
NVIDIA
>>
>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.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;
>>>>
>>>> Ok, as mentioned by Yang and Michal, only err == 0 means no error.
>>>>
>>>> Sounds this regression should be fixed in another place. Let me send out
>>>> another patch.
>>>>
>>>
>>> Hmm... I took another look into the case, this fix should work.
>>>
>>> But yes, the semantic here is a little confusion. Look forward your comments
>>> here.
>>>
>>>>> out:
>>>>> return err;
>>>>> --
>>>>> 2.17.1
>>>>
>>>> --
>>>> Wei Yang
>>>> Help you, Help me
>>>
>>
>> thanks,
>> --
>> John Hubbard
>> NVIDIA
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-19 6:57 [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero Wei Yang
2020-01-21 1:53 ` Wei Yang
@ 2020-01-24 7:21 ` Michal Hocko
2020-01-24 14:15 ` Wei Yang
1 sibling, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2020-01-24 7:21 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel
[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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-24 7:21 ` Michal Hocko
@ 2020-01-24 14:15 ` Wei Yang
2020-01-24 14:46 ` Michal Hocko
0 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-01-24 14:15 UTC (permalink / raw)
To: Michal Hocko
Cc: Wei Yang, akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel
On Fri, Jan 24, 2020 at 08:21:27AM +0100, Michal Hocko wrote:
>[Sorry I have missed this patch previously]
>
No problem, thanks for your comment.
>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
I am afraid I have a different understanding here.
If we jump to out_flush on the test of node_isset(), err is -EACCESS. Current
logic would return this instead of the error from do_move_pages_to_node().
Seems we don't override -EACCESS.
Is my understanding correct?
>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
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-24 14:15 ` Wei Yang
@ 2020-01-24 14:46 ` Michal Hocko
2020-01-24 15:28 ` Wei Yang
0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2020-01-24 14:46 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel
On Fri 24-01-20 22:15:38, Wei Yang wrote:
> On Fri, Jan 24, 2020 at 08:21:27AM +0100, Michal Hocko wrote:
> >[Sorry I have missed this patch previously]
> >
>
> No problem, thanks for your comment.
>
> >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
>
> I am afraid I have a different understanding here.
>
> If we jump to out_flush on the test of node_isset(), err is -EACCESS. Current
> logic would return this instead of the error from do_move_pages_to_node().
> Seems we don't override -EACCESS.
And this is the expected logic. The unexpected behavior is the one you
have fixed by this patch because err = 1 wouldn't get overriden and that
should have been.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero
2020-01-24 14:46 ` Michal Hocko
@ 2020-01-24 15:28 ` Wei Yang
0 siblings, 0 replies; 12+ messages in thread
From: Wei Yang @ 2020-01-24 15:28 UTC (permalink / raw)
To: Michal Hocko
Cc: Wei Yang, akpm, yang.shi, jhubbard, vbabka, cl, linux-mm, linux-kernel
On Fri, Jan 24, 2020 at 03:46:43PM +0100, Michal Hocko wrote:
>On Fri 24-01-20 22:15:38, Wei Yang wrote:
>> On Fri, Jan 24, 2020 at 08:21:27AM +0100, Michal Hocko wrote:
>> >[Sorry I have missed this patch previously]
>> >
>>
>> No problem, thanks for your comment.
>>
>> >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
>>
>> I am afraid I have a different understanding here.
>>
>> If we jump to out_flush on the test of node_isset(), err is -EACCESS. Current
>> logic would return this instead of the error from do_move_pages_to_node().
>> Seems we don't override -EACCESS.
>
>And this is the expected logic. The unexpected behavior is the one you
>have fixed by this patch because err = 1 wouldn't get overriden and that
>should have been.
Ok, if the sentence cover this case, the wording looks good to me.
Thanks :-)
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-01-24 15:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-19 6:57 [Patch v2] mm/migrate.c: also overwrite error when it is bigger than zero Wei Yang
2020-01-21 1:53 ` Wei Yang
2020-01-21 2:34 ` Wei Yang
2020-01-21 19:33 ` John Hubbard
2020-01-22 0:42 ` Wei Yang
2020-01-22 1:29 ` John Hubbard
2020-01-21 19:30 ` Yang Shi
2020-01-22 0:41 ` Wei Yang
2020-01-24 7:21 ` Michal Hocko
2020-01-24 14:15 ` Wei Yang
2020-01-24 14:46 ` Michal Hocko
2020-01-24 15:28 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).