* [PATCH] swapfile : fix the wrong return value @ 2010-03-02 3:38 Huang Shijie 2010-03-04 0:37 ` Hugh Dickins 2010-03-04 7:14 ` [PATCH] swapfile : export more return values for swap_duplicate() Huang Shijie 0 siblings, 2 replies; 6+ messages in thread From: Huang Shijie @ 2010-03-02 3:38 UTC (permalink / raw) To: akpm; +Cc: linux-mm, hugh.dickins, 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] swapfile : fix the wrong return value 2010-03-02 3:38 [PATCH] swapfile : fix the wrong return value Huang Shijie @ 2010-03-04 0:37 ` Hugh Dickins 2010-03-04 7:00 ` Huang Shijie 2010-03-04 7:14 ` [PATCH] swapfile : export more return values for swap_duplicate() Huang Shijie 1 sibling, 1 reply; 6+ messages in thread From: Hugh Dickins @ 2010-03-04 0:37 UTC (permalink / raw) To: Huang Shijie; +Cc: akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] swapfile : fix the wrong return value 2010-03-04 0:37 ` Hugh Dickins @ 2010-03-04 7:00 ` Huang Shijie 2010-03-04 7:28 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Huang Shijie @ 2010-03-04 7:00 UTC (permalink / raw) To: Hugh Dickins; +Cc: akpm, linux-mm >> 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] swapfile : fix the wrong return value 2010-03-04 7:00 ` Huang Shijie @ 2010-03-04 7:28 ` Hugh Dickins 2010-03-04 7:48 ` Huang Shijie 0 siblings, 1 reply; 6+ messages in thread From: Hugh Dickins @ 2010-03-04 7:28 UTC (permalink / raw) To: Huang Shijie; +Cc: akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] swapfile : fix the wrong return value 2010-03-04 7:28 ` Hugh Dickins @ 2010-03-04 7:48 ` Huang Shijie 0 siblings, 0 replies; 6+ messages in thread From: Huang Shijie @ 2010-03-04 7:48 UTC (permalink / raw) To: Hugh Dickins; +Cc: akpm, linux-mm > 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] swapfile : export more return values for swap_duplicate() 2010-03-02 3:38 [PATCH] swapfile : fix the wrong return value Huang Shijie 2010-03-04 0:37 ` Hugh Dickins @ 2010-03-04 7:14 ` Huang Shijie 1 sibling, 0 replies; 6+ messages in thread From: Huang Shijie @ 2010-03-04 7:14 UTC (permalink / raw) To: akpm; +Cc: hugh.dickins, linux-mm, 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-04 7:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-02 3:38 [PATCH] swapfile : fix the wrong return value Huang Shijie 2010-03-04 0:37 ` Hugh Dickins 2010-03-04 7:00 ` Huang Shijie 2010-03-04 7:28 ` Hugh Dickins 2010-03-04 7:48 ` Huang Shijie 2010-03-04 7:14 ` [PATCH] swapfile : export more return values for swap_duplicate() Huang Shijie
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).