[PATCH] mm: Don't forget to set softdirty on file mapped fault

[PATCH] mm: Don't forget to set softdirty on file mapped fault

[Cyrill Gorcunov]

Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
helper _returns_ new pte but not modifies argument.

CC: Pavel Emelyanov <xemul@parallels.com>
CC: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 mm/memory.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.git/mm/memory.c
===================================================================
--- linux-2.6.git.orig/mm/memory.c
+++ linux-2.6.git/mm/memory.c
@@ -2744,7 +2744,7 @@ void do_set_pte(struct vm_area_struct *v
 	if (write)
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 	else if (pte_file(*pte) && pte_file_soft_dirty(*pte))
-		pte_mksoft_dirty(entry);
+		entry = pte_mksoft_dirty(entry);
 	if (anon) {
 		inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
 		page_add_new_anon_rmap(page, vma, address);

--
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] mm: Don't forget to set softdirty on file mapped fault

[Pavel Emelyanov]

On 07/08/2014 11:21 PM, Cyrill Gorcunov wrote:
> Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
> helper _returns_ new pte but not modifies argument.
> 
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Acked-by: Pavel Emelyanov <xemul@parallels.com>

> ---
>  mm/memory.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6.git/mm/memory.c
> ===================================================================
> --- linux-2.6.git.orig/mm/memory.c
> +++ linux-2.6.git/mm/memory.c
> @@ -2744,7 +2744,7 @@ void do_set_pte(struct vm_area_struct *v
>  	if (write)
>  		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>  	else if (pte_file(*pte) && pte_file_soft_dirty(*pte))
> -		pte_mksoft_dirty(entry);
> +		entry = pte_mksoft_dirty(entry);
>  	if (anon) {
>  		inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>  		page_add_new_anon_rmap(page, vma, address);
> .
> 

--
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] mm: Don't forget to set softdirty on file mapped fault

[Andrew Morton]

On Tue, 8 Jul 2014 23:21:51 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
> helper _returns_ new pte but not modifies argument.

When fixing a bug, please describe the end-user visible effects of that
bug.

[for the 12,000th time :(]

--
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] mm: Don't forget to set softdirty on file mapped fault

[Cyrill Gorcunov]

On Tue, Jul 08, 2014 at 01:19:20PM -0700, Andrew Morton wrote:
> On Tue, 8 Jul 2014 23:21:51 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
> > helper _returns_ new pte but not modifies argument.
> 
> When fixing a bug, please describe the end-user visible effects of that
> bug.
> 
> [for the 12,000th time :(]

"we may not notice that pte was softdirty" I thought it's enough, because
that's the effect user sees -- pte is not dirtified where it should.

Really sorry Andrew if I were not clear enough. What about: In case if page
fault happend on dirty filemapping the newly created pte may not
notice if old one were already softdirtified because pte_mksoft_dirty
doesn't modify its argument but rather returns new pte value.

--
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] mm: Don't forget to set softdirty on file mapped fault

[Andrew Morton]

On Wed, 9 Jul 2014 00:40:17 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Tue, Jul 08, 2014 at 01:19:20PM -0700, Andrew Morton wrote:
> > On Tue, 8 Jul 2014 23:21:51 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > 
> > > Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
> > > helper _returns_ new pte but not modifies argument.
> > 
> > When fixing a bug, please describe the end-user visible effects of that
> > bug.
> > 
> > [for the 12,000th time :(]
> 
> "we may not notice that pte was softdirty" I thought it's enough, because
> that's the effect user sees -- pte is not dirtified where it should.
> 
> Really sorry Andrew if I were not clear enough. What about: In case if page
> fault happend on dirty filemapping the newly created pte may not
> notice if old one were already softdirtified because pte_mksoft_dirty
> doesn't modify its argument but rather returns new pte value.

The user doesn't know or care about pte bits.

What actually *happens*?  Does criu migration hang?  Does it lose data?
Does it take longer?

IOW, what would an end-user's bug report look like?

It's important to think this way because a year from now some person
we've never heard of may be looking at a user's bug report and
wondering whether backporting this patch will fix it.  Amongst other
reasons.

--
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] mm: Don't forget to set softdirty on file mapped fault

[Cyrill Gorcunov]

On Tue, Jul 08, 2014 at 01:45:11PM -0700, Andrew Morton wrote:
> 
> The user doesn't know or care about pte bits.
> 
> What actually *happens*?  Does criu migration hang?  Does it lose data?
> Does it take longer?

Ah, I see. Yes, the softdirty bit might be lost that usespace program
won't see that a page was modified. So data lose is possible.

> IOW, what would an end-user's bug report look like?
> 
> It's important to think this way because a year from now some person
> we've never heard of may be looking at a user's bug report and
> wondering whether backporting this patch will fix it.  Amongst other
> reasons.

Here is updated changelog, sounds better?
---

In case if page fault happend on dirty filemapping the newly created pte
may loose softdirty bit thus if a userspace program is tracking memory
changes with help of a memory tracker (CONFIG_MEM_SOFT_DIRTY) it might
miss modification of a memory page (which in worts case may lead to
data inconsistency).

--
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] mm: Don't forget to set softdirty on file mapped fault

[Andrew Morton]

On Wed, 9 Jul 2014 00:54:48 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Tue, Jul 08, 2014 at 01:45:11PM -0700, Andrew Morton wrote:
> > 
> > The user doesn't know or care about pte bits.
> > 
> > What actually *happens*?  Does criu migration hang?  Does it lose data?
> > Does it take longer?
> 
> Ah, I see. Yes, the softdirty bit might be lost that usespace program
> won't see that a page was modified. So data lose is possible.
> 
> > IOW, what would an end-user's bug report look like?
> > 
> > It's important to think this way because a year from now some person
> > we've never heard of may be looking at a user's bug report and
> > wondering whether backporting this patch will fix it.  Amongst other
> > reasons.
> 
> Here is updated changelog, sounds better?
> ---
> 
> In case if page fault happend on dirty filemapping the newly created pte
> may loose softdirty bit thus if a userspace program is tracking memory
> changes with help of a memory tracker (CONFIG_MEM_SOFT_DIRTY) it might
> miss modification of a memory page (which in worts case may lead to
> data inconsistency).

Much better, thanks.

It's a rather gross-looking bug and data inconsistency sounds serious. 
Do you think a -stable backport is needed?  

--
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] mm: Don't forget to set softdirty on file mapped fault

[Cyrill Gorcunov]

On Tue, Jul 08, 2014 at 02:05:01PM -0700, Andrew Morton wrote:
> > 
> > In case if page fault happend on dirty filemapping the newly created pte
> > may loose softdirty bit thus if a userspace program is tracking memory
> > changes with help of a memory tracker (CONFIG_MEM_SOFT_DIRTY) it might
> > miss modification of a memory page (which in worts case may lead to
> > data inconsistency).
> 
> Much better, thanks.
> 
> It's a rather gross-looking bug and data inconsistency sounds serious. 
> Do you think a -stable backport is needed?

It seems the memory tracker is not that widespread in userspace
programs (I mean at the moment as far as I know only we use it
intensively) so I don't consider it as critical but moving it
into stable won't hurt. Still I fear in 3.16 the mm/memory.c
code has been significantly reworked so this patch won't apply
on its own. I can prepare a patch for 3.15 though, just say
a word.

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