* [PATCH] mm: fix potential anon_vma locking issue in mprotect() @ 2012-09-04 23:39 Michel Lespinasse 2012-09-04 23:45 ` Andrea Arcangeli 2012-09-04 23:46 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Michel Lespinasse @ 2012-09-04 23:39 UTC (permalink / raw) To: linux-mm, akpm; +Cc: aarcange This change fixes an anon_vma locking issue in the following situation: - vma has no anon_vma - next has an anon_vma - vma is being shrunk / next is being expanded, due to an mprotect call We need to take next's anon_vma lock to avoid races with rmap users (such as page migration) while next is being expanded. Signed-off-by: Michel Lespinasse <walken@google.com> --- mm/mmap.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index 3edfcdfa42d9..6fd7afa0e651 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -578,8 +578,12 @@ again: remove_next = 1 + (end > next->vm_end); */ if (vma->anon_vma && (importer || start != vma->vm_start)) { anon_vma = vma->anon_vma; + VM_BUG_ON(adjust_next && next->anon_vma && + anon_vma != next->anon_vma); + } else if (adjust_next && next->anon_vma) + anon_vma = next->anon_vma; + if (anon_vma) anon_vma_lock(anon_vma); - } if (root) { flush_dcache_mmap_lock(mapping); -- 1.7.7.3 -- 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: fix potential anon_vma locking issue in mprotect() 2012-09-04 23:39 [PATCH] mm: fix potential anon_vma locking issue in mprotect() Michel Lespinasse @ 2012-09-04 23:45 ` Andrea Arcangeli 2012-09-04 23:46 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Andrea Arcangeli @ 2012-09-04 23:45 UTC (permalink / raw) To: Michel Lespinasse; +Cc: linux-mm, akpm On Tue, Sep 04, 2012 at 04:39:49PM -0700, Michel Lespinasse wrote: > This change fixes an anon_vma locking issue in the following situation: > - vma has no anon_vma > - next has an anon_vma > - vma is being shrunk / next is being expanded, due to an mprotect call > > We need to take next's anon_vma lock to avoid races with rmap users > (such as page migration) while next is being expanded. > > Signed-off-by: Michel Lespinasse <walken@google.com> Reviewed-by: Andrea Arcangeli <aarcange@redhat.com> -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: fix potential anon_vma locking issue in mprotect() 2012-09-04 23:39 [PATCH] mm: fix potential anon_vma locking issue in mprotect() Michel Lespinasse 2012-09-04 23:45 ` Andrea Arcangeli @ 2012-09-04 23:46 ` Andrew Morton 2012-09-05 0:02 ` Michel Lespinasse 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2012-09-04 23:46 UTC (permalink / raw) To: Michel Lespinasse; +Cc: linux-mm, aarcange On Tue, 4 Sep 2012 16:39:49 -0700 Michel Lespinasse <walken@google.com> wrote: > This change fixes an anon_vma locking issue in the following situation: > - vma has no anon_vma > - next has an anon_vma > - vma is being shrunk / next is being expanded, due to an mprotect call > > We need to take next's anon_vma lock to avoid races with rmap users > (such as page migration) while next is being expanded. > > ... > > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -578,8 +578,12 @@ again: remove_next = 1 + (end > next->vm_end); > */ > if (vma->anon_vma && (importer || start != vma->vm_start)) { > anon_vma = vma->anon_vma; > + VM_BUG_ON(adjust_next && next->anon_vma && > + anon_vma != next->anon_vma); > + } else if (adjust_next && next->anon_vma) > + anon_vma = next->anon_vma; > + if (anon_vma) > anon_vma_lock(anon_vma); > - } > > if (root) { > flush_dcache_mmap_lock(mapping); hm, OK. How serious was that bug? I'm suspecting "only needed in 3.7". If we want to fix this in 3.6 and perhaps -stable, I'm a bit worried about that new VM_BUG_ON(). Not that I don't trust you or anything, but these things tend to go bang when people least expected it. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: fix potential anon_vma locking issue in mprotect() 2012-09-04 23:46 ` Andrew Morton @ 2012-09-05 0:02 ` Michel Lespinasse 2012-09-05 10:11 ` Andrea Arcangeli 0 siblings, 1 reply; 6+ messages in thread From: Michel Lespinasse @ 2012-09-05 0:02 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-mm, aarcange On Tue, Sep 4, 2012 at 4:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 4 Sep 2012 16:39:49 -0700 > Michel Lespinasse <walken@google.com> wrote: > >> This change fixes an anon_vma locking issue in the following situation: >> - vma has no anon_vma >> - next has an anon_vma >> - vma is being shrunk / next is being expanded, due to an mprotect call >> >> We need to take next's anon_vma lock to avoid races with rmap users >> (such as page migration) while next is being expanded. > > hm, OK. How serious was that bug? I'm suspecting "only needed in > 3.7". That was my starting position as well. I'd expect the biggest issue would be page migration races, and we do have assertions for that case, and we've not been hitting them (that I know of). So, this should not be a high frequency issue AFAICT. I don't want to push for -stable backports myself, but I do think it's nice to do a minimal patch so that it can easily be backported if we decide to. -- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: fix potential anon_vma locking issue in mprotect() 2012-09-05 0:02 ` Michel Lespinasse @ 2012-09-05 10:11 ` Andrea Arcangeli 2012-09-05 19:24 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Andrea Arcangeli @ 2012-09-05 10:11 UTC (permalink / raw) To: Michel Lespinasse; +Cc: Andrew Morton, linux-mm On Tue, Sep 04, 2012 at 05:02:49PM -0700, Michel Lespinasse wrote: > On Tue, Sep 4, 2012 at 4:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Sep 2012 16:39:49 -0700 > > Michel Lespinasse <walken@google.com> wrote: > > > >> This change fixes an anon_vma locking issue in the following situation: > >> - vma has no anon_vma > >> - next has an anon_vma > >> - vma is being shrunk / next is being expanded, due to an mprotect call > >> > >> We need to take next's anon_vma lock to avoid races with rmap users > >> (such as page migration) while next is being expanded. > > > > hm, OK. How serious was that bug? I'm suspecting "only needed in > > 3.7". Agreed. > That was my starting position as well. I'd expect the biggest issue > would be page migration races, and we do have assertions for that > case, and we've not been hitting them (that I know of). So, this > should not be a high frequency issue AFAICT. I exclude it's reproducible with real load too, the window is far too small. A malicious load might reproduce it, but the worst case would be to trigger the BUG_ON assertion in migration_entry_to_page like you mentioned above or to "gracefully" hang in migration_entry_wait, or to trigger one of the BUG_ONs in split_huge_page with no risk of memory corruption or anything. The only two places in the VM that depends on full accuracy in finding all ptes from the rmap walk are remove_migration_ptes and split_huge_page and they both are (and must remain) robust enough not to generate memory corruption or any other adverse side effects if the rmap walk actually wasn't 100% accurate because of some race condition like in this case. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: fix potential anon_vma locking issue in mprotect() 2012-09-05 10:11 ` Andrea Arcangeli @ 2012-09-05 19:24 ` Hugh Dickins 0 siblings, 0 replies; 6+ messages in thread From: Hugh Dickins @ 2012-09-05 19:24 UTC (permalink / raw) To: Andrea Arcangeli; +Cc: Michel Lespinasse, Andrew Morton, Dave Jones, linux-mm On Wed, 5 Sep 2012, Andrea Arcangeli wrote: > On Tue, Sep 04, 2012 at 05:02:49PM -0700, Michel Lespinasse wrote: > > On Tue, Sep 4, 2012 at 4:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > > > On Tue, 4 Sep 2012 16:39:49 -0700 > > > Michel Lespinasse <walken@google.com> wrote: > > > > > >> This change fixes an anon_vma locking issue in the following situation: > > >> - vma has no anon_vma > > >> - next has an anon_vma > > >> - vma is being shrunk / next is being expanded, due to an mprotect call > > >> > > >> We need to take next's anon_vma lock to avoid races with rmap users > > >> (such as page migration) while next is being expanded. Mmm, good catch by Michel. I got interested in the history of this, and checked back: we had this locking right before 2.6.34, when it got lost somewhere in the succession of changes for the anon_vma chains. In 2.6.33, we were also locking next anon_vma for the remove_next case, and I got worried that Michel's patch might be incomplete: but no, nowadays the unlinking of the anon_vma is handled further down, getting that lock quite separately in unlink_anon_vmas() (from "anon_vma_merge"). Acked-by: Hugh Dickins <hughd@google.com> > > > > > > hm, OK. How serious was that bug? I'm suspecting "only needed in > > > 3.7". > > Agreed. > > > That was my starting position as well. I'd expect the biggest issue > > would be page migration races, and we do have assertions for that > > case, and we've not been hitting them (that I know of). So, this > > should not be a high frequency issue AFAICT. > > I exclude it's reproducible with real load too, the window is far too > small. > > A malicious load might reproduce it, but the worst case would be to > trigger the BUG_ON assertion in migration_entry_to_page like you > mentioned above or to "gracefully" hang in migration_entry_wait, or to > trigger one of the BUG_ONs in split_huge_page with no risk of memory > corruption or anything. Yes, that malicious Mr Dave Jones is the most likely to trigger it. I'll defer to you and Andrew and Michel as to whether the fix should go to stable: you think not, and I agree it's a very narrow window, whose only danger is triggering one of those BUG_ONs. But certainly I'm glad that Michel and Andrew have separated it out as a fix which can be applied independent of his series. Hugh > > The only two places in the VM that depends on full accuracy in finding > all ptes from the rmap walk are remove_migration_ptes and > split_huge_page and they both are (and must remain) robust enough not > to generate memory corruption or any other adverse side effects if the > rmap walk actually wasn't 100% accurate because of some race condition > like in this case. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-05 19:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-04 23:39 [PATCH] mm: fix potential anon_vma locking issue in mprotect() Michel Lespinasse 2012-09-04 23:45 ` Andrea Arcangeli 2012-09-04 23:46 ` Andrew Morton 2012-09-05 0:02 ` Michel Lespinasse 2012-09-05 10:11 ` Andrea Arcangeli 2012-09-05 19:24 ` Hugh Dickins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).