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