mbind() breaks its API definition since v5.2 by commit d883544515aa (mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF_STRICT were specified)

DISCUSSION Li Xinhai 2 participants 5 messages Oct 30, 2019

Subsystems (3)

Memory management ABI/API MM NUMA placement

lore

Share & tools

lore

Li Xinhai

author

2019-10-30 02:27 UTC #0 lore raw

One change in do_mbind() of this commit has suspicious usage of return value of queue_pages_range(), excerpt as below:

---
@@ -1243,10 +1265,15 @@ static long do_mbind(unsigned long start, unsigned long len, if (err)
goto mpol_out;

- err = queue_pages_range(mm, start, end, nmask,
+ ret = queue_pages_range(mm, start, end, nmask,
flags | MPOL_MF_INVERT, &pagelist);
- if (!err)
- err = mbind_range(mm, start, end, new);
+
+ if (ret < 0) { /////// convert to all possible 'ret' to '-EIO' <<<<
+ err = -EIO;
+ goto up_out;
+ }
+
+ err = mbind_range(mm, start, end, new);

if (!err) {
int nr_failed = 0;
---

Note that inside queue_pages_range(), the call to walk_page_range() may return errors from 'test_walk' of 'struct mm_walk_ops', e.g. -EFAULT. Now, those error codes are no longer reported to user space application.

From user space, the mbind() call need to reported error, with EFAULT, as example:
EFAULT
Part or all of the memory range specified by nodemask and maxnode points outside your accessible address space. Or, there was an unmapped hole in the specified memory range specified by addr and len.

Please correct me if this is the intended change(and will have updated API definition), or something was misunderstood.

-Xinhai

Yang Shi

2019-10-30 02:50 UTC #1 lore raw

Quoted 29 lines · Li Xinhai

One change in do_mbind() of this commit has suspicious usage of return value of queue_pages_range(), excerpt as below:
---
(quoted diff) +8 -4
@@ -1243,10 +1265,15 @@ static long do_mbind(unsigned long start, unsigned long len,
  	if (err)
  		goto mpol_out;
 
-	err = queue_pages_range(mm, start, end, nmask,
+	ret = queue_pages_range(mm, start, end, nmask,
  			  flags | MPOL_MF_INVERT, &pagelist);
-	if (!err)
-		err = mbind_range(mm, start, end, new);
+
+	if (ret < 0) {      /////// convert to all possible 'ret' to '-EIO' <<<<
+		err = -EIO;
+		goto up_out;
+	}
+
+	err = mbind_range(mm, start, end, new);
 
  	if (!err) {
  		int nr_failed = 0;
---
Note that inside queue_pages_range(), the call to walk_page_range() may return errors from 'test_walk' of 'struct mm_walk_ops', e.g. -EFAULT. Now, those error codes are no longer reported to user space application.

Quoted 5 lines

From user space, the mbind() call need to reported error, with EFAULT, as example:
EFAULT
Part or all of the memory range specified by nodemask and maxnode points outside your accessible address space. Or, there was an unmapped hole in the specified memory range specified by addr and len.

Thanks for catching this. That commit was aimed to correct the return values for some corner cases in mbind(), but it should not alter the errno for other failure cases, i.e. -EFAULT.

Could you please try the below patch (build test only)?

mm/mempolicy.c +0 -0

index 4ae967b..99df43a 100644
@@ -1286,7 +1286,7 @@ static long do_mbind(unsigned long start, unsigned long len,

flags | MPOL_MF_INVERT, &pagelist);

        if (ret < 0) {
-               err = -EIO;
+               err = ret;
                goto up_out;
        }

Quoted 5 lines

Please correct me if this is the intended change(and will have updated API definition), or something was misunderstood.
-Xinhai

Li Xinhai

author

2019-10-30 03:12 UTC #2 lore raw

Quoted 51 lines · On 2019-10-30 at 10:50 Yang Shi wrote:

Quoted 29 lines · Li Xinhai
One change in do_mbind() of this commit has suspicious usage of return value of queue_pages_range(), excerpt as below:
---
(quoted diff) +0 -0
@@ -1243,10 +1265,15 @@ static long do_mbind(unsigned long start, unsigned long len,
    	if (err)
    	goto mpol_out;
(quoted diff) +1 -1
-	err = queue_pages_range(mm, start, end, nmask,
+	ret = queue_pages_range(mm, start, end, nmask,
flags | MPOL_MF_INVERT, &pagelist);
(quoted diff) +7 -2
-	if (!err)
-	err = mbind_range(mm, start, end, new);
+
+	if (ret < 0) {      /////// convert to all possible 'ret' to '-EIO' <<<<
+	err = -EIO;
+	goto up_out;
+	}
+
+	err = mbind_range(mm, start, end, new);
if (!err) {
int nr_failed = 0;
---
Note that inside queue_pages_range(), the call to walk_page_range() may return errors from 'test_walk' of 'struct mm_walk_ops', e.g. -EFAULT. Now, those error codes are no longer reported to user space application.
Quoted 5 lines
From user space, the mbind() call need to reported error, with EFAULT, as example:
EFAULT
Part or all of the memory range specified by nodemask and maxnode points outside your accessible address space. Or, there was an unmapped hole in the specified memory range specified by addr and len.
Thanks for catching this. That commit was aimed to correct the return values for some corner cases in mbind(), but it should not alter the errno for other failure cases, i.e. -EFAULT.
Could you please try the below patch (build test only)?
mm/mempolicy.c +0 -0
index 4ae967b..99df43a 100644

mm/mempolicy.c static long do_mbind(unsigned long start, unsigned · line 1292

Quoted 8 lines

mm/mempolicy.c +1 -1

@@ -1286,7 +1286,7 @@ static long do_mbind(unsigned long start, unsigned
 long len,
                           flags | MPOL_MF_INVERT, &pagelist);
 
         if (ret < 0) {
-               err = -EIO;
+               err = ret;
                 goto up_out;
         }

This seems do not work, because the 'pagelist' would have some pages queued into it, need to put back those pages instead of return quickly.

So, we need to remove this page leak as well. <<<<<<

In my understanding, revert the changes as I quoted above may solve it, but not sure the details about changes at end of do_mbind(), should keep them at there without further change?

- Xinhai

Quoted 7 lines

Quoted 5 lines
Please correct me if this is the intended change(and will have updated API definition), or something was misunderstood.
-Xinhai

Yang Shi

2019-10-30 04:32 UTC #3 lore raw

Quoted 61 lines · Li Xinhai

Quoted 49 lines · On 2019-10-30 at 10:50 Yang Shi wrote:
Quoted 29 lines · Li Xinhai
One change in do_mbind() of this commit has suspicious usage of return value of queue_pages_range(), excerpt as below:
---
(quoted diff) +8 -4
@@ -1243,10 +1265,15 @@ static long do_mbind(unsigned long start, unsigned long len,
    	if (err)
    	goto mpol_out;
 
-	err = queue_pages_range(mm, start, end, nmask,
+	ret = queue_pages_range(mm, start, end, nmask,
    	 flags | MPOL_MF_INVERT, &pagelist);
-	if (!err)
-	err = mbind_range(mm, start, end, new);
+
+	if (ret < 0) {      /////// convert to all possible 'ret' to '-EIO' <<<<
+	err = -EIO;
+	goto up_out;
+	}
+
+	err = mbind_range(mm, start, end, new);
 
    	if (!err) {
    	int nr_failed = 0;
---
Note that inside queue_pages_range(), the call to walk_page_range() may return errors from 'test_walk' of 'struct mm_walk_ops', e.g. -EFAULT. Now, those error codes are no longer reported to user space application.
Quoted 5 lines
From user space, the mbind() call need to reported error, with EFAULT, as example:
EFAULT
Part or all of the memory range specified by nodemask and maxnode points outside your accessible address space. Or, there was an unmapped hole in the specified memory range specified by addr and len.
Thanks for catching this. That commit was aimed to correct the return values for some corner cases in mbind(), but it should not alter the errno for other failure cases, i.e. -EFAULT.
Could you please try the below patch (build test only)?
mm/mempolicy.c +0 -0
index 4ae967b..99df43a 100644
mm/mempolicy.c static long do_mbind(unsigned long start, unsigned · line 1292
Quoted 8 lines
mm/mempolicy.c +1 -1
@@ -1286,7 +1286,7 @@ static long do_mbind(unsigned long start, unsigned
 long len,
                           flags | MPOL_MF_INVERT, &pagelist);
 
         if (ret < 0) {
-               err = -EIO;
+               err = ret;
                 goto up_out;
         }
This seems do not work, because the 'pagelist' would have some pages queued into it, need to put back those pages instead of return quickly.

Quoted 5 lines

So, we need to remove this page leak as well. <<<<<<
In my understanding, revert the changes as I quoted above may solve it, but not sure the details about changes at end of do_mbind(), should keep them at there without further change?

Thanks for pointing this out. We don't have to revert this commit to handle the non-empty pagelist correctly. The simplest way is to just put those pages back and I'm supposed this is also the preferred way since mbind_range() is not called to really apply the policy so those pages should not be migrated.

The below patch should solve this:

mm/mempolicy.c +0 -0

index 4ae967b..d80025c 100644
@@ -1286,7 +1286,10 @@ static long do_mbind(unsigned long start, unsigned long len,

flags | MPOL_MF_INVERT, &pagelist);

        if (ret < 0) {
-               err = -EIO;
+               if (!list_empty(&pagelist))
+                       putback_movable_pages(&pagelist); +
+               err = ret;
                goto up_out;
        }

Quoted 8 lines

- Xinhai
Quoted 5 lines
Quoted 4 lines
Please correct me if this is the intended change(and will have updated API definition), or something was misunderstood.
-Xinhai

Li Xinhai

author

2019-10-30 06:00 UTC #4 lore raw

Quoted 85 lines · On 2019-10-30 at 12:32 Yang Shi wrote:

Quoted 61 lines · Li Xinhai
Quoted 49 lines · On 2019-10-30 at 10:50 Yang Shi wrote:
Quoted 29 lines · Li Xinhai
One change in do_mbind() of this commit has suspicious usage of return value of queue_pages_range(), excerpt as below:
---
(quoted diff) +0 -0
@@ -1243,10 +1265,15 @@ static long do_mbind(unsigned long start, unsigned long len,
      	if (err)
      	goto mpol_out;
(quoted diff) +1 -1
-	err = queue_pages_range(mm, start, end, nmask,
+	ret = queue_pages_range(mm, start, end, nmask,
flags | MPOL_MF_INVERT, &pagelist);
(quoted diff) +7 -2
-	if (!err)
-	err = mbind_range(mm, start, end, new);
+
+	if (ret < 0) {      /////// convert to all possible 'ret' to '-EIO' <<<<
+	err = -EIO;
+	goto up_out;
+	}
+
+	err = mbind_range(mm, start, end, new);
if (!err) {
int nr_failed = 0;
---
Note that inside queue_pages_range(), the call to walk_page_range() may return errors from 'test_walk' of 'struct mm_walk_ops', e.g. -EFAULT. Now, those error codes are no longer reported to user space application.
Quoted 5 lines
From user space, the mbind() call need to reported error, with EFAULT, as example:
EFAULT
Part or all of the memory range specified by nodemask and maxnode points outside your accessible address space. Or, there was an unmapped hole in the specified memory range specified by addr and len.
Thanks for catching this. That commit was aimed to correct the return values for some corner cases in mbind(), but it should not alter the errno for other failure cases, i.e. -EFAULT.
Could you please try the below patch (build test only)?
mm/mempolicy.c +0 -0
index 4ae967b..99df43a 100644
mm/mempolicy.c static long do_mbind(unsigned long start, unsigned · line 1292
Quoted 8 lines
mm/mempolicy.c +1 -1
@@ -1286,7 +1286,7 @@ static long do_mbind(unsigned long start, unsigned
 long len,
                             flags | MPOL_MF_INVERT, &pagelist);
 
           if (ret < 0) {
-               err = -EIO;
+               err = ret;
                   goto up_out;
           }
This seems do not work, because the 'pagelist' would have some pages queued into it, need to put back those pages instead of return quickly.
Quoted 5 lines
So, we need to remove this page leak as well. <<<<<<
In my understanding, revert the changes as I quoted above may solve it, but not sure the details about changes at end of do_mbind(), should keep them at there without further change?
Thanks for pointing this out. We don't have to revert this commit to handle the non-empty pagelist correctly. The simplest way is to just put those pages back and I'm supposed this is also the preferred way since mbind_range() is not called to really apply the policy so those pages should not be migrated.
The below patch should solve this:
mm/mempolicy.c +0 -0
index 4ae967b..d80025c 100644
@@ -1286,7 +1286,10 @@ static long do_mbind(unsigned long start,
 unsigned long len,

mm/mempolicy.c static long do_mbind(unsigned long start, · line 1295

Quoted 10 lines

mm/mempolicy.c +4 -1

                           flags | MPOL_MF_INVERT, &pagelist);
 
         if (ret < 0) {
-               err = -EIO;
+               if (!list_empty(&pagelist))
+                       putback_movable_pages(&pagelist);
+
+               err = ret;
                 goto up_out;
         }

Checked the original commit about the 'ret >0' purpose, and understood that was for migrating page in best effort way.

Your patch looks correct to me.

Quoted 10 lines

Quoted 8 lines
- Xinhai
Quoted 5 lines
Quoted 4 lines
Please correct me if this is the intended change(and will have updated API definition), or something was misunderstood.
-Xinhai