Re: [PATCH 2/7] mm: fix potential anon_vma locking issue in mprotect()
On Tue, Sep 4, 2012 at 3:16 PM, Andrea Arcangeli wrote: > I would suggest to do the strict fix as above in as patch 1/8 and push > it in -mm, and to do only the optimization removal in 3/8. I think > we want it in -stable too later, so it'll make life easier to > cherry-pick the commit if it's merged independently. All right. So I did this and the strict fix got into Andrew's tree as mm-fix-potential-anon_vma-locking-issue-in-mprotect.patch Andrew: when you try applying this series, this patch (2/7) won't apply due to the strict fix being already there. Please just skip it (and replace patch 4/7 with the replacement I'm about to send, so that we end up with the same end state) -- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] mm: fix potential anon_vma locking issue in mprotect()
On Tue, Sep 04, 2012 at 02:53:47PM -0700, Michel Lespinasse wrote:
> I think the minimal fix would actually be:
>
> if (vma->anon_vma && (importer || start != vma->vm_start)) {
> anon_vma = vma->anon_vma;
> + else if (next->anon_vma && adjust_next)
> + anon_vma = next->anon_vma;
Right indeed. The last change required to the above is to check
adjust_next first.
> I suppose if we were to consider adding this fix to the stable series,
> we should probably do it in such a minimal way. I hadn't actually
> considered it, because I was only thinking about this patch series,
> and at patch 4/7 it becomes necessary to lock the anon_vma even if
> only the vm_end side gets modified (so we'd still end up with what I
> proposed in the end)
Ah, that fully explains you removed the optimization :). I was
reviewing the patch as a bugfix for upstream without noticing the
new requirements introduced by the later patches.
I would suggest to do the strict fix as above in as patch 1/8 and push
it in -mm, and to do only the optimization removal in 3/8. I think
we want it in -stable too later, so it'll make life easier to
cherry-pick the commit if it's merged independently.
Thanks!
Andrea
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] mm: fix potential anon_vma locking issue in mprotect()
On Tue, Sep 04, 2012 at 04:27:45PM +0200, Andrea Arcangeli wrote:
> Hi Michel,
>
> On Tue, Sep 04, 2012 at 02:20:52AM -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.
> >
> > This change also removes an optimization which avoided taking anon_vma
> > lock during brk adjustments. We could probably make that optimization
> > work again, but the following anon rmap change would break it,
> > so I kept things as simple as possible here.
>
> Agreed, definitely a bug not to take the lock whenever any
> vm_start/vm_pgoff are moved, regardless if they're the next or current
> vma. Only vm_end can be moved without taking the lock.
>
> I'd prefer to fix it like this though:
>
> - if (vma->anon_vma && (importer || start != vma->vm_start)) {
> + if ((vma->anon_vma && (importer || start != vma->vm_start) ||
> + (adjust_next && next->anon_vma)) {
I think the minimal fix would actually be:
if (vma->anon_vma && (importer || start != vma->vm_start)) {
anon_vma = vma->anon_vma;
+ else if (next->anon_vma && adjust_next)
+ anon_vma = next->anon_vma;
I suppose if we were to consider adding this fix to the stable series,
we should probably do it in such a minimal way. I hadn't actually
considered it, because I was only thinking about this patch series,
and at patch 4/7 it becomes necessary to lock the anon_vma even if
only the vm_end side gets modified (so we'd still end up with what I
proposed in the end)
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/7] mm: fix potential anon_vma locking issue in mprotect()
Hi Michel,
On Tue, Sep 04, 2012 at 02:20:52AM -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.
>
> This change also removes an optimization which avoided taking anon_vma
> lock during brk adjustments. We could probably make that optimization
> work again, but the following anon rmap change would break it,
> so I kept things as simple as possible here.
Agreed, definitely a bug not to take the lock whenever any
vm_start/vm_pgoff are moved, regardless if they're the next or current
vma. Only vm_end can be moved without taking the lock.
I'd prefer to fix it like this though:
- if (vma->anon_vma && (importer || start != vma->vm_start)) {
+ if ((vma->anon_vma && (importer || start != vma->vm_start) ||
+ (adjust_next && next->anon_vma)) {
The strict fix is just to check also if we're moving next->vm_start or
not, and the lock is only needed if next->anon_vma is set (otherwise
there's no page yet set in the vma and we hold the mmap_sem in write
mode clearly that prevents new pages to be instantiated under us).
Plus we know if adjust_next is set, next is not null, so the above
should work. The already existing (optimized) check for the "vma"
should have been ok, so no need to de-optimize it.
Then it's still fine to retain the VM_BUG_ON in the branch where
anon_vma was not null.
Thanks!
Andrea
>
> Signed-off-by: Michel Lespinasse
> ---
> mm/mmap.c | 14 ++
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index cebc346ba0db..5e64c7dfc090 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -570,14 +570,12 @@ again: remove_next = 1 + (end >
> next->vm_end);
>
> vma_adjust_trans_huge(vma, start, end, adjust_next);
>
> - /*
> - * When changing only vma->vm_end, we don't really need anon_vma
> - * lock. This is a fairly rare case by itself, but the anon_vma
> - * lock may be shared between many sibling processes. Skipping
> - * the lock for brk adjustments makes a difference sometimes.
> - */
> - if (vma->anon_vma && (importer || start != vma->vm_start)) {
> - anon_vma = vma->anon_vma;
> + anon_vma = vma->anon_vma;
> + if (!anon_vma && adjust_next)
> + anon_vma = next->anon_vma;
> + if (anon_vma) {
> + VM_BUG_ON(adjust_next && next->anon_vma &&
> + anon_vma != next->anon_vma);
> anon_vma_lock(anon_vma);
> }
>
> --
> 1.7.7.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/7] mm: fix potential anon_vma locking issue in mprotect()
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.
This change also removes an optimization which avoided taking anon_vma
lock during brk adjustments. We could probably make that optimization
work again, but the following anon rmap change would break it,
so I kept things as simple as possible here.
Signed-off-by: Michel Lespinasse
---
mm/mmap.c | 14 ++
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index cebc346ba0db..5e64c7dfc090 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -570,14 +570,12 @@ again:remove_next = 1 + (end >
next->vm_end);
vma_adjust_trans_huge(vma, start, end, adjust_next);
- /*
-* When changing only vma->vm_end, we don't really need anon_vma
-* lock. This is a fairly rare case by itself, but the anon_vma
-* lock may be shared between many sibling processes. Skipping
-* the lock for brk adjustments makes a difference sometimes.
-*/
- if (vma->anon_vma && (importer || start != vma->vm_start)) {
- anon_vma = vma->anon_vma;
+ anon_vma = vma->anon_vma;
+ if (!anon_vma && adjust_next)
+ anon_vma = next->anon_vma;
+ if (anon_vma) {
+ VM_BUG_ON(adjust_next && next->anon_vma &&
+ anon_vma != next->anon_vma);
anon_vma_lock(anon_vma);
}
--
1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
