All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrea Arcangeli <aarcange@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, Rik van Riel <riel@redhat.com>,
	Hugh Dickins <hughd@google.com>,
	Mel Gorman <mgorman@techsingularity.net>,
	Jan Vorlicek <janvorli@microsoft.com>,
	Aditya Mandaleeka <adityam@microsoft.com>
Subject: Re: [PATCH 1/1] mm: vma_merge: fix vm_page_prot SMP race condition against rmap_walk
Date: Sun, 18 Sep 2016 02:36:54 +0200	[thread overview]
Message-ID: <20160918003654.GA25048@redhat.com> (raw)
In-Reply-To: <1474128315-22726-2-git-send-email-aarcange@redhat.com>

On Sat, Sep 17, 2016 at 06:05:15PM +0200, Andrea Arcangeli wrote:
> +	if (remove_next == 1) {
> +		/*
> +		 * vm_page_prot and vm_flags can be read by the
> +		 * rmap_walk, for example in remove_migration_ptes(),
> +		 * so before releasing the rmap locks the permissions
> +		 * of the expanded vmas must be already the correct
> +		 * one for the whole merged range.
> +		 *
> +		 * mprotect case 8 (which sets remove_next == 1) needs
> +		 * special handling to provide the above guarantee, as
> +		 * it is the only case where the "vma" that is being
> +		 * expanded is the one with the wrong permissions for
> +		 * the whole merged region. So copy the right
> +		 * permissions from the next one that is getting
> +		 * removed before releasing the rmap locks.
> +		 */
> +		vma->vm_page_prot = next->vm_page_prot;
> +		vma->vm_flags = next->vm_flags;
> +	}
>  	if (start != vma->vm_start) {

One more thought, doesn't remove_next get set to 1 also in case 7?

I assumed this could be fixed within vma_adjust but case 7 is
indistinguishable from case 8 from within vma_adjust. So the fix has
to move up one level in vma_merge where it's possible to differentiate
case 7 and case 8.

The fact no available testcase is exercising the race with any other
cases of vma_merge except case 8, makes the testing prone for false
negatives (accidentally upstream also initially passed as a false
negative thanks to the pmd_modify in do_numa_page that hidden the most
visible side effect of the bug even in case 8). All I can easily
verify with the testcase is that case 8 is fixed by monitoring any
erroneous do_numa_page execution on non-NUMA guests, and sure thing
case 8 was fixed.

I'll also reconsider how much more complex it is to remove the "area"
vma in case 8, instead of the "next", so that case 8 changes from
PPPPNNNNXXXX->PPPPNNNNNNNN to PPPPNNNNXXXX->PPPPXXXXXXXX, in turn
removing the oddness factor from case 8.

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

  reply	other threads:[~2016-09-18  0:36 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-15 17:41 [PATCH 0/2] vma_merge vs rmap_walk SMP race condition fix Andrea Arcangeli
2016-09-15 17:41 ` [PATCH 1/2] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE Andrea Arcangeli
2016-09-15 18:27   ` Rik van Riel
2016-09-15 17:41 ` [PATCH 2/2] mm: vma_merge: fix race vm_page_prot race condition against rmap_walk Andrea Arcangeli
2016-09-15 18:28   ` Rik van Riel
2016-09-16 18:42   ` Hugh Dickins
2016-09-16 20:54     ` Andrea Arcangeli
2016-09-17 16:05       ` [PATCH 0/1] mm: vma_merge: fix vm_page_prot SMP race condition against rmap_walk v2 Andrea Arcangeli
2016-09-17 16:05         ` [PATCH 1/1] mm: vma_merge: fix vm_page_prot SMP race condition against rmap_walk Andrea Arcangeli
2016-09-18  0:36           ` Andrea Arcangeli [this message]
2016-09-19 18:25             ` [PATCH 1/2] " Andrea Arcangeli
2016-09-19 18:25               ` [PATCH 2/2] mm: vma_adjust: remove superfluous check for next not NULL Andrea Arcangeli
2016-09-22 10:36               ` [PATCH 1/2] mm: vma_merge: fix vm_page_prot SMP race condition against rmap_walk Hugh Dickins
2016-09-23 19:18                 ` Andrea Arcangeli
2016-09-23 20:25                   ` Hugh Dickins
2016-09-28  5:09               ` [mm] 2129957506: kernel BUG at mm/mmap.c:329! kernel test robot
2016-09-28  5:09                 ` [lkp] " kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160918003654.GA25048@redhat.com \
    --to=aarcange@redhat.com \
    --cc=adityam@microsoft.com \
    --cc=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=janvorli@microsoft.com \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=riel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.