[PATCH] mm: don't lose the SOFT_DIRTY flag on mprotect

[PATCH] mm: don't lose the SOFT_DIRTY flag on mprotect

[Andrey Vagin]

The SOFT_DIRTY bit shows that the content of memory was changed
after a defined point in the past. mprotect() doesn't change the
content of memory, so it must not change the SOFT_DIRTY bit.

This patch does nothing with _PAGE_SWP_SOFT_DIRTY, becase pte_modify()
is called only for present pages.

Fixes: 0f8975ec4db2 (mm: soft-dirty bits for user memory changes tracking)
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 arch/x86/include/asm/pgtable_types.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index a83aa44..1aa9ccd 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -121,7 +121,8 @@
 
 /* Set of bits not changed in pte_modify */
 #define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |		\
-			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY)
+			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
+			 _PAGE_SOFT_DIRTY)
 #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
 
 #define _PAGE_CACHE_MASK	(_PAGE_PCD | _PAGE_PWT)
-- 
1.8.5.3

Re: [PATCH] mm: don't lose the SOFT_DIRTY flag on mprotect

[Andrew Morton]

On Thu, 30 Jan 2014 00:04:59 +0400 Andrey Vagin <avagin@openvz.org> wrote:

> The SOFT_DIRTY bit shows that the content of memory was changed
> after a defined point in the past. mprotect() doesn't change the
> content of memory, so it must not change the SOFT_DIRTY bit.
> 
> This patch does nothing with _PAGE_SWP_SOFT_DIRTY, becase pte_modify()
> is called only for present pages.

Standard complaint: when fixing a bug, please describe the end-user
visible effects of that bug.

afaict the effects are minor: snapshotting will save/copy more pages
than it needs to, but there will be no malfunction.  Hence a -stable
backport is unneeded.  Agree?

Re: [PATCH] mm: don't lose the SOFT_DIRTY flag on mprotect

[Andrew Vagin]

On Wed, Jan 29, 2014 at 12:18:59PM -0800, Andrew Morton wrote:
> On Thu, 30 Jan 2014 00:04:59 +0400 Andrey Vagin <avagin@openvz.org> wrote:
> 
> > The SOFT_DIRTY bit shows that the content of memory was changed
> > after a defined point in the past. mprotect() doesn't change the
> > content of memory, so it must not change the SOFT_DIRTY bit.
> > 
> > This patch does nothing with _PAGE_SWP_SOFT_DIRTY, becase pte_modify()
> > is called only for present pages.
> 
> Standard complaint: when fixing a bug, please describe the end-user
> visible effects of that bug.
> 
> afaict the effects are minor: snapshotting will save/copy more pages
> than it needs to, but there will be no malfunction.  Hence a -stable
> backport is unneeded.  Agree?

There will be malfunction. On the first iteration all pages are dumped.
On other iterations only pages with the SOFT_DIRTY bit are dumped. So if
the SOFT_DIRTY bit is cleared from a page by mistake, the page is not
dumped and its content will be restored incorrectly.
This patch should be cc'ed to stable. Sorry forgot to do this in first
place.

The following program is able to detect the problem:
#include <sys/mman.h>
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

#define PME_SOFT_DIRTY (1ULL << 55)

int main()
{
        unsigned long long v;
        void *p;
        int fd;

        p = mmap(NULL, 4096, PROT_WRITE,
				MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
        if (p == MAP_FAILED)
                return -1;

        fd = open("/proc/self/clear_refs", O_WRONLY);
        if (fd < 0)
                return 1;
        if (write(fd, "4\n", 2) < 2)
                return 1;
        close(fd);

        *((int *) p) = 5;

        fd = open("/proc/self/pagemap", O_RDONLY);
        if (fd < 0)
                return 1;
        mprotect(p, 4096, PROT_READ);
        lseek(fd, (unsigned long) p / 4096 * 8, SEEK_SET);
        read(fd, &v, sizeof(v));
        close(fd);

        printf("%8llx\n", v);

        if (v & PME_SOFT_DIRTY) {
                printf("PASS\n");
                return 0;
        }

        printf("FAIL\n");
        return 1;
}