Otherwise we may not notice that pte was softdirty because pte_mksoft_dirty
helper _returns_ new pte but not modifies argument.
CC: Pavel Emelyanov <[email protected]>
CC: Andrew Morton <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
---
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);
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 <[email protected]>
> CC: Andrew Morton <[email protected]>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
Acked-by: Pavel Emelyanov <[email protected]>
> ---
> 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);
> .
>
On Tue, 8 Jul 2014 23:21:51 +0400 Cyrill Gorcunov <[email protected]> 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 :(]
On Tue, Jul 08, 2014 at 01:19:20PM -0700, Andrew Morton wrote:
> On Tue, 8 Jul 2014 23:21:51 +0400 Cyrill Gorcunov <[email protected]> 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.
On Wed, 9 Jul 2014 00:40:17 +0400 Cyrill Gorcunov <[email protected]> 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 <[email protected]> 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.
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).
On Wed, 9 Jul 2014 00:54:48 +0400 Cyrill Gorcunov <[email protected]> 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?
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.