mm/migrate.c: also overwrite error when it is bigger than zero

PATCH Applied Wei Yang 4 participants 12 messages Jan 19, 2020 – Jan 24, 2020

Subsystems (2)

Memory management MM migration

lore

Share & tools

lore

Wei Yang

author

2020-01-19 06:57 UTC #0 lore raw

PATCH v2 1/1 mm/migrate.c: also overwrite error when it is bigger than zero

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 <[email protected]>

---
v2:
* put more words to explain the error case

---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/migrate.c +1 -1

index 86873b6f38a7..430fdccc733e 100644
@@ -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;

Signature · 1 line

2.17.1

Wei Yang

author

2020-01-21 01:53 UTC #1 lore raw

Quoted 28 lines · 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 <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +0 -0
index 86873b6f38a7..430fdccc733e 100644

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680

Quoted 6 lines

mm/migrate.c +1 -1

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

Quoted 4 lines

out:
	return err;
Signature · 1 line
2.17.1

Signature · 2 lines

Wei Yang
Help you, Help me

Wei Yang

author

2020-01-21 02:34 UTC #2 lore raw

Quoted 38 lines · Wei Yang

Quoted 28 lines · 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 <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +0 -0
index 86873b6f38a7..430fdccc733e 100644
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 6 lines
mm/migrate.c +1 -1
@@ -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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1
Signature · 2 lines
Wei Yang
Help you, Help me

Signature · 2 lines

Wei Yang
Help you, Help me

John Hubbard

2020-01-21 19:33 UTC #3 lore raw

Quoted 12 lines · Wei Yang

Quoted 11 lines · Wei Yang
Quoted 10 lines · 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

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
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

Quoted 23 lines

Quoted 23 lines
Quoted 14 lines
Signed-off-by Wei Yang <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +0 -0
index 86873b6f38a7..430fdccc733e 100644
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 6 lines
mm/migrate.c +1 -1
@@ -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.

Quoted 18 lines

Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1
Signature · 2 lines
Wei Yang
Help you, Help me

thanks,

Signature · 2 lines

John Hubbard
NVIDIA

Wei Yang

author

2020-01-22 00:42 UTC #4 lore raw

Quoted 13 lines · John Hubbard

Quoted 12 lines · Wei Yang
Quoted 11 lines · Wei Yang
Quoted 10 lines · 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

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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().

Quoted 52 lines

thanks,
--
John Hubbard
NVIDIA
Quoted 23 lines
Quoted 23 lines
Quoted 14 lines
Signed-off-by Wei Yang <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +0 -0
index 86873b6f38a7..430fdccc733e 100644
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 6 lines
mm/migrate.c +1 -1
@@ -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.
Quoted 18 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1
Signature · 2 lines
Wei Yang
Help you, Help me
thanks,
Signature · 2 lines
John Hubbard
NVIDIA

Signature · 2 lines

Wei Yang
Help you, Help me

John Hubbard

2020-01-22 01:29 UTC #5 lore raw

Quoted 19 lines · Wei Yang

Quoted 18 lines · John Hubbard
Quoted 16 lines · Wei Yang
Quoted 15 lines · Wei Yang
Quoted 14 lines · 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").

Quoted 7 lines

Quoted 2 lines
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,

Signature · 2 lines

John Hubbard
NVIDIA

Quoted 48 lines

Quoted 47 lines
Quoted 24 lines
Quoted 24 lines
Quoted 15 lines
Signed-off-by Wei Yang <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +0 -0
index 86873b6f38a7..430fdccc733e 100644
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 6 lines
mm/migrate.c +1 -1
@@ -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.
Quoted 18 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1
Signature · 2 lines
Wei Yang
Help you, Help me
thanks,
Signature · 2 lines
John Hubbard
NVIDIA

Yang Shi

Acked-by

2020-01-21 19:30 UTC #6 lore raw

In reply to Wei Yang

Quoted 24 lines · Wei Yang

Quoted 23 lines · 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 <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 11 lines
mm/migrate.c +1 -1
index 86873b6f38a7..430fdccc733e 100644
@@ -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 <[email protected]>

Quoted 8 lines

Sounds this regression should be fixed in another place. Let me send out another patch.
Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1

Wei Yang

author

2020-01-22 00:40 UTC #7 lore raw

Quoted 40 lines · Yang Shi

Quoted 24 lines · Wei Yang
Quoted 23 lines · 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 <[email protected]>
---
v2:
* put more words to explain the error case
---
mm/migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 11 lines
mm/migrate.c +1 -1
index 86873b6f38a7..430fdccc733e 100644
@@ -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.

Quoted 8 lines

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 <[email protected]>

Thanks

Quoted 9 lines

Quoted 9 lines
Sounds this regression should be fixed in another place. Let me send out another patch.
Quoted 4 lines
out:
	return err;
Signature · 1 line
2.17.1

Signature · 2 lines

Wei Yang
Help you, Help me

Michal Hocko

Acked-by on v2 1/1

2020-01-24 07:21 UTC #8 lore raw

In reply to Wei Yang

[Sorry I have missed this patch previously]

Quoted 9 lines · 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

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

Quoted 5 lines

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 <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 24 lines

---
v2:
* put more words to explain the error case

---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/migrate.c +1 -1

index 86873b6f38a7..430fdccc733e 100644
@@ -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;

Signature · 1 line

2.17.1

Signature · 2 lines

Michal Hocko
SUSE Labs

Wei Yang

author

2020-01-24 14:15 UTC #9 lore raw

Quoted 2 lines · Michal Hocko

[Sorry I have missed this patch previously]

No problem, thanks for your comment.

Quoted 17 lines · Wei Yang

Quoted 9 lines
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.

Quoted 6 lines

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?

Quoted 43 lines

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.
"
Quoted 5 lines
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 <[email protected]>
Acked-by Michal Hocko <[email protected]>
Quoted 24 lines
---
v2:
* put more words to explain the error case
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +1 -1
index 86873b6f38a7..430fdccc733e 100644
@@ -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;
Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko
SUSE Labs

Signature · 2 lines

Wei Yang
Help you, Help me

Michal Hocko

2020-01-24 14:46 UTC #10 lore raw

Quoted 30 lines · Wei Yang

Quoted 2 lines · Michal Hocko
[Sorry I have missed this patch previously]
No problem, thanks for your comment.
Quoted 17 lines · Wei Yang
Quoted 9 lines
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.
Quoted 6 lines
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

Quoted 5 lines

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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Wei Yang

author

2020-01-24 15:28 UTC #11 lore raw

Quoted 33 lines · Michal Hocko

Quoted 32 lines · Wei Yang
Quoted 2 lines · Michal Hocko
[Sorry I have missed this patch previously]
No problem, thanks for your comment.
Quoted 17 lines · Wei Yang
Quoted 9 lines
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.
Quoted 6 lines
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.

Quoted 7 lines

Quoted 3 lines
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 :-)

Quoted 3 lines

Signature · 2 lines
Michal Hocko
SUSE Labs

Signature · 2 lines

Wei Yang
Help you, Help me