[PATCH] swapfile : fix the wrong return value

[PATCH] swapfile : fix the wrong return value

[Huang Shijie]

If the __swap_duplicate returns a negative value except of the -ENOMEM,
but the err is zero at this time, the return value of swap_duplicate is
wrong in this situation.

The caller, such as try_to_unmap_one(), will do the wrong operations too
in this situation.

This patch fix it.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 mm/swapfile.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6c0585b..191d8fa 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2161,7 +2161,7 @@ int swap_duplicate(swp_entry_t entry)
 {
 	int err = 0;
 
-	while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
+	while (!err && (err = __swap_duplicate(entry, 1)) == -ENOMEM)
 		err = add_swap_count_continuation(entry, GFP_ATOMIC);
 	return err;
 }
-- 
1.6.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] swapfile : fix the wrong return value

[Hugh Dickins]

On Tue, 2 Mar 2010, Huang Shijie wrote:

> If the __swap_duplicate returns a negative value except of the -ENOMEM,
> but the err is zero at this time, the return value of swap_duplicate is
> wrong in this situation.
> 
> The caller, such as try_to_unmap_one(), will do the wrong operations too
> in this situation.
> 
> This patch fix it.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> ---
>  mm/swapfile.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 6c0585b..191d8fa 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -2161,7 +2161,7 @@ int swap_duplicate(swp_entry_t entry)
>  {
>  	int err = 0;
>  
> -	while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
> +	while (!err && (err = __swap_duplicate(entry, 1)) == -ENOMEM)
>  		err = add_swap_count_continuation(entry, GFP_ATOMIC);
>  	return err;
>  }
> -- 

I was on the point of Ack'ing your patch, and despairing at my confusion,
when I realized what's actually going on here - the key is (look at 2.6.32)
swap_duplicate() used to be a void function (no error code whatsoever),
until I added the -ENOMEM for swap_count_continuation.  And in fact your
patch is wrong, copy_one_pte() does not want to add swap_count_continuation
in the case when it hits a corrupt pte (one which looks like a swap entry).

But you're absolutely right that it cries out for a comment:


[PATCH] mm: add comment on swap_duplicate's error code

swap_duplicate()'s loop appears to miss out on returning the error code
from __swap_duplicate(), except when that's -ENOMEM.  In fact this is
intentional: prior to -ENOMEM for swap_count_continuation, swap_duplicate()
was void (and the case only occurs when copy_one_pte() hits a corrupt pte).
But that's surprising behaviour, which certainly deserves a comment.

Reported-by: Huang Shijie <shijie8@gmail.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
---

 mm/swapfile.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- 2633/mm/swapfile.c	2010-02-24 18:52:17.000000000 +0000
+++ linux/mm/swapfile.c	2010-03-04 00:11:35.000000000 +0000
@@ -2155,7 +2155,11 @@ void swap_shmem_alloc(swp_entry_t entry)
 }
 
 /*
- * increase reference count of swap entry by 1.
+ * Increase reference count of swap entry by 1.
+ * Returns 0 for success, or -ENOMEM if a swap_count_continuation is required
+ * but could not be atomically allocated.  Returns 0, just as if it succeeded,
+ * if __swap_duplicate() fails for another reason (-EINVAL or -ENOENT), which
+ * might occur if a page table entry has got corrupted.
  */
 int swap_duplicate(swp_entry_t entry)
 {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] swapfile : fix the wrong return value

[Huang Shijie]

>> If the __swap_duplicate returns a negative value except of the -ENOMEM,
>> but the err is zero at this time, the return value of swap_duplicate is
>> wrong in this situation.
>>
>> The caller, such as try_to_unmap_one(), will do the wrong operations too
>> in this situation.
>>
>> This patch fix it.
>>
>> Signed-off-by: Huang Shijie<shijie8@gmail.com>
>> ---
>>   mm/swapfile.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 6c0585b..191d8fa 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -2161,7 +2161,7 @@ int swap_duplicate(swp_entry_t entry)
>>   {
>>   	int err = 0;
>>
>> -	while (!err&&  __swap_duplicate(entry, 1) == -ENOMEM)
>> +	while (!err&&  (err = __swap_duplicate(entry, 1)) == -ENOMEM)
>>   		err = add_swap_count_continuation(entry, GFP_ATOMIC);
>>   	return err;
>>   }
>> -- 
>>      
> I was on the point of Ack'ing your patch, and despairing at my confusion,
> when I realized what's actually going on here - the key is (look at 2.6.32)
> swap_duplicate() used to be a void function (no error code whatsoever),
> until I added the -ENOMEM for swap_count_continuation.  And in fact your
> patch is wrong, copy_one_pte() does not want to add swap_count_continuation
>    
Yes,you are right, my patch is wrong in this situation.
> in the case when it hits a corrupt pte (one which looks like a swap entry).
>
> But you're absolutely right that it cries out for a comment:
>
>
> [PATCH] mm: add comment on swap_duplicate's error code
>
> swap_duplicate()'s loop appears to miss out on returning the error code
> from __swap_duplicate(), except when that's -ENOMEM.  In fact this is
> intentional: prior to -ENOMEM for swap_count_continuation, swap_duplicate()
> was void (and the case only occurs when copy_one_pte() hits a corrupt pte).
>    
only?

There are several paths calling the try_to_unmap(), Could you sure that
the swap entries are valid in all the paths ?

For the sake of the stability of the system, I perfer to export all the 
error value,
and check it carefully.

What about my following patch?

> But that's surprising behaviour, which certainly deserves a comment.
>
> Reported-by: Huang Shijie<shijie8@gmail.com>
> Signed-off-by: Hugh Dickins<hughd@google.com>
> ---
>
>   mm/swapfile.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> --- 2633/mm/swapfile.c	2010-02-24 18:52:17.000000000 +0000
> +++ linux/mm/swapfile.c	2010-03-04 00:11:35.000000000 +0000
> @@ -2155,7 +2155,11 @@ void swap_shmem_alloc(swp_entry_t entry)
>   }
>
>   /*
> - * increase reference count of swap entry by 1.
> + * Increase reference count of swap entry by 1.
> + * Returns 0 for success, or -ENOMEM if a swap_count_continuation is required
> + * but could not be atomically allocated.  Returns 0, just as if it succeeded,
> + * if __swap_duplicate() fails for another reason (-EINVAL or -ENOENT), which
> + * might occur if a page table entry has got corrupted.
>    */
>   int swap_duplicate(swp_entry_t entry)
>   {
>
>    

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] swapfile : export more return values for swap_duplicate()

[Huang Shijie]

Exporting more return values for swap_duplicate() is useful for
try_to_unmap(). It could check the swap entry more carefully which
is helpful for the system stability.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 mm/memory.c   |    3 ++-
 mm/swapfile.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 72fb5f3..72d1d1c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -586,7 +586,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		if (!pte_file(pte)) {
 			swp_entry_t entry = pte_to_swp_entry(pte);
 
-			if (swap_duplicate(entry) < 0)
+			/* add_swap_count_continuation() failed ? */
+			if (swap_duplicate(entry) == -ENOMEM)
 				return entry.val;
 
 			/* make sure dst_mm is on swapoff's mmlist. */
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6c0585b..a2720d0 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2161,7 +2161,7 @@ int swap_duplicate(swp_entry_t entry)
 {
 	int err = 0;
 
-	while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
+	while (!err && (err =  __swap_duplicate(entry, 1)) == -ENOMEM)
 		err = add_swap_count_continuation(entry, GFP_ATOMIC);
 	return err;
 }
-- 
1.6.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] swapfile : fix the wrong return value

[Hugh Dickins]

On Thu, 4 Mar 2010, Huang Shijie wrote:
> > 
> > swap_duplicate()'s loop appears to miss out on returning the error code
> > from __swap_duplicate(), except when that's -ENOMEM.  In fact this is
> > intentional: prior to -ENOMEM for swap_count_continuation, swap_duplicate()
> > was void (and the case only occurs when copy_one_pte() hits a corrupt pte).
> >    
> only?
> 
> There are several paths calling the try_to_unmap(), Could you sure that
> the swap entries are valid in all the paths ?

Yes.  Well, we are debating the likelihoods of corruption in different memory
areas here.  I answer "Yes" because the swap entry involved in try_to_unmap_one()
comes from page->private when PageSwapCache is set (and the page is locked):
it requires either an mm bug, or corruption of struct page, for that swap entry
to be invalid for duplication.  Memory corruption of entries in a user page
table seems to have been a more common case, whether because of single-bit memory
errors, or use-after-free bugs: that's the case which copy_one_pte() might meet.
 
> 
> For the sake of the stability of the system, I perfer to export all the error
> value, and check it carefully.

But we were happy with void swap_duplicate() for many years.
If I wanted to make a further change, it would rather be to remove those
error returns from __swap_duplicate() which are not actually made use of.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] swapfile : fix the wrong return value

[Huang Shijie]

> Yes.  Well, we are debating the likelihoods of corruption in different memory
> areas here.  I answer "Yes" because the swap entry involved in try_to_unmap_one()
> comes from page->private when PageSwapCache is set (and the page is locked):
> it requires either an mm bug, or corruption of struct page, for that swap entry
> to be invalid for duplication.  Memory corruption of entries in a user page
> table seems to have been a more common case, whether because of single-bit memory
> errors, or use-after-free bugs: that's the case which copy_one_pte() might meet.
>    
:), ok, thanks a lot for your kind explanations.
>
>    
>> For the sake of the stability of the system, I perfer to export all the error
>> value, and check it carefully.
>>      
> But we were happy with void swap_duplicate() for many years.
> If I wanted to make a further change, it would rather be to remove those
> error returns from __swap_duplicate() which are not actually made use of.
>
> Hugh
>
>    

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>