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

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

mm/memory.c +1 -1

@@ -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);

.

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

Quoted 2 lines · 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 :(]

Quoted 9 lines · Andrew Morton

Quoted 2 lines · 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.

Quoted 5 lines · Andrew Morton

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.

Quoted 6 lines

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

Quoted 6 lines

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?