| Message ID | 20221110201742.27485-1-yuxuan.luo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Jammy] mm/rmap: Fix anon_vma->degree ambiguity leading to double-reuse | expand |
On Thu, Nov 10, 2022 at 03:17:30PM -0500, Yuxuan Luo wrote: > From: Jann Horn <jannh@google.com> > > anon_vma->degree tracks the combined number of child anon_vmas and VMAs > that use the anon_vma as their ->anon_vma. > > anon_vma_clone() then assumes that for any anon_vma attached to > src->anon_vma_chain other than src->anon_vma, it is impossible for it to > be a leaf node of the VMA tree, meaning that for such VMAs ->degree is > elevated by 1 because of a child anon_vma, meaning that if ->degree > equals 1 there are no VMAs that use the anon_vma as their ->anon_vma. > > This assumption is wrong because the ->degree optimization leads to leaf > nodes being abandoned on anon_vma_clone() - an existing anon_vma is > reused and no new parent-child relationship is created. So it is > possible to reuse an anon_vma for one VMA while it is still tied to > another VMA. > > This is an issue because is_mergeable_anon_vma() and its callers assume > that if two VMAs have the same ->anon_vma, the list of anon_vmas > attached to the VMAs is guaranteed to be the same. When this assumption > is violated, vma_merge() can merge pages into a VMA that is not attached > to the corresponding anon_vma, leading to dangling page->mapping > pointers that will be dereferenced during rmap walks. > > Fix it by separately tracking the number of child anon_vmas and the > number of VMAs using the anon_vma as their ->anon_vma. > > Fixes: 7a3ef208e662 ("mm: prevent endless growth of anon_vma hierarchy") > Cc: stable@kernel.org > Acked-by: Michal Hocko <mhocko@suse.com> > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Jann Horn <jannh@google.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (cherry picked from commit 2555283eb40df89945557273121e9393ef9b542b) I think the same comment conflict that you found on bionic was fixed up here. But it has been done correctly, and since it's a minor comment typo fix that causes the conflict (and it is the line that is removed), I am fine either applying as cherry-picked or as backported. Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > CVE-2022-42703 > Signed-off-by: Yuxuan Luo <yuxuan.luo@canonical.com> > --- > include/linux/rmap.h | 7 +++++-- > mm/rmap.c | 29 ++++++++++++++++------------- > 2 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index c976cc6de257..c29d9c13378b 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -39,12 +39,15 @@ struct anon_vma { > atomic_t refcount; > > /* > - * Count of child anon_vmas and VMAs which points to this anon_vma. > + * Count of child anon_vmas. Equals to the count of all anon_vmas that > + * have ->parent pointing to this one, including itself. > * > * This counter is used for making decision about reusing anon_vma > * instead of forking new one. See comments in function anon_vma_clone. > */ > - unsigned degree; > + unsigned long num_children; > + /* Count of VMAs whose ->anon_vma pointer points to this object. */ > + unsigned long num_active_vmas; > > struct anon_vma *parent; /* Parent of this anon_vma */ > > diff --git a/mm/rmap.c b/mm/rmap.c > index 3e340ee380cb..330b361a460e 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -90,7 +90,8 @@ static inline struct anon_vma *anon_vma_alloc(void) > anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL); > if (anon_vma) { > atomic_set(&anon_vma->refcount, 1); > - anon_vma->degree = 1; /* Reference for first vma */ > + anon_vma->num_children = 0; > + anon_vma->num_active_vmas = 0; > anon_vma->parent = anon_vma; > /* > * Initialise the anon_vma root to point to itself. If called > @@ -198,6 +199,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma) > anon_vma = anon_vma_alloc(); > if (unlikely(!anon_vma)) > goto out_enomem_free_avc; > + anon_vma->num_children++; /* self-parent link for new root */ > allocated = anon_vma; > } > > @@ -207,8 +209,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma) > if (likely(!vma->anon_vma)) { > vma->anon_vma = anon_vma; > anon_vma_chain_link(vma, avc, anon_vma); > - /* vma reference or self-parent link for new root */ > - anon_vma->degree++; > + anon_vma->num_active_vmas++; > allocated = NULL; > avc = NULL; > } > @@ -293,19 +294,19 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src) > anon_vma_chain_link(dst, avc, anon_vma); > > /* > - * Reuse existing anon_vma if its degree lower than two, > - * that means it has no vma and only one anon_vma child. > + * Reuse existing anon_vma if it has no vma and only one > + * anon_vma child. > * > - * Do not chose parent anon_vma, otherwise first child > - * will always reuse it. Root anon_vma is never reused: > + * Root anon_vma is never reused: > * it has self-parent reference and at least one child. > */ > if (!dst->anon_vma && src->anon_vma && > - anon_vma != src->anon_vma && anon_vma->degree < 2) > + anon_vma->num_children < 2 && > + anon_vma->num_active_vmas == 0) > dst->anon_vma = anon_vma; > } > if (dst->anon_vma) > - dst->anon_vma->degree++; > + dst->anon_vma->num_active_vmas++; > unlock_anon_vma_root(root); > return 0; > > @@ -355,6 +356,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) > anon_vma = anon_vma_alloc(); > if (!anon_vma) > goto out_error; > + anon_vma->num_active_vmas++; > avc = anon_vma_chain_alloc(GFP_KERNEL); > if (!avc) > goto out_error_free_anon_vma; > @@ -375,7 +377,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) > vma->anon_vma = anon_vma; > anon_vma_lock_write(anon_vma); > anon_vma_chain_link(vma, avc, anon_vma); > - anon_vma->parent->degree++; > + anon_vma->parent->num_children++; > anon_vma_unlock_write(anon_vma); > > return 0; > @@ -407,7 +409,7 @@ void unlink_anon_vmas(struct vm_area_struct *vma) > * to free them outside the lock. > */ > if (RB_EMPTY_ROOT(&anon_vma->rb_root.rb_root)) { > - anon_vma->parent->degree--; > + anon_vma->parent->num_children--; > continue; > } > > @@ -415,7 +417,7 @@ void unlink_anon_vmas(struct vm_area_struct *vma) > anon_vma_chain_free(avc); > } > if (vma->anon_vma) { > - vma->anon_vma->degree--; > + vma->anon_vma->num_active_vmas--; > > /* > * vma would still be needed after unlink, and anon_vma will be prepared > @@ -433,7 +435,8 @@ void unlink_anon_vmas(struct vm_area_struct *vma) > list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) { > struct anon_vma *anon_vma = avc->anon_vma; > > - VM_WARN_ON(anon_vma->degree); > + VM_WARN_ON(anon_vma->num_children); > + VM_WARN_ON(anon_vma->num_active_vmas); > put_anon_vma(anon_vma); > > list_del(&avc->same_vma); > -- > 2.34.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/include/linux/rmap.h b/include/linux/rmap.h index c976cc6de257..c29d9c13378b 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -39,12 +39,15 @@ struct anon_vma { atomic_t refcount; /* - * Count of child anon_vmas and VMAs which points to this anon_vma. + * Count of child anon_vmas. Equals to the count of all anon_vmas that + * have ->parent pointing to this one, including itself. * * This counter is used for making decision about reusing anon_vma * instead of forking new one. See comments in function anon_vma_clone. */ - unsigned degree; + unsigned long num_children; + /* Count of VMAs whose ->anon_vma pointer points to this object. */ + unsigned long num_active_vmas; struct anon_vma *parent; /* Parent of this anon_vma */ diff --git a/mm/rmap.c b/mm/rmap.c index 3e340ee380cb..330b361a460e 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -90,7 +90,8 @@ static inline struct anon_vma *anon_vma_alloc(void) anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL); if (anon_vma) { atomic_set(&anon_vma->refcount, 1); - anon_vma->degree = 1; /* Reference for first vma */ + anon_vma->num_children = 0; + anon_vma->num_active_vmas = 0; anon_vma->parent = anon_vma; /* * Initialise the anon_vma root to point to itself. If called @@ -198,6 +199,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma) anon_vma = anon_vma_alloc(); if (unlikely(!anon_vma)) goto out_enomem_free_avc; + anon_vma->num_children++; /* self-parent link for new root */ allocated = anon_vma; } @@ -207,8 +209,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma) if (likely(!vma->anon_vma)) { vma->anon_vma = anon_vma; anon_vma_chain_link(vma, avc, anon_vma); - /* vma reference or self-parent link for new root */ - anon_vma->degree++; + anon_vma->num_active_vmas++; allocated = NULL; avc = NULL; } @@ -293,19 +294,19 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src) anon_vma_chain_link(dst, avc, anon_vma); /* - * Reuse existing anon_vma if its degree lower than two, - * that means it has no vma and only one anon_vma child. + * Reuse existing anon_vma if it has no vma and only one + * anon_vma child. * - * Do not chose parent anon_vma, otherwise first child - * will always reuse it. Root anon_vma is never reused: + * Root anon_vma is never reused: * it has self-parent reference and at least one child. */ if (!dst->anon_vma && src->anon_vma && - anon_vma != src->anon_vma && anon_vma->degree < 2) + anon_vma->num_children < 2 && + anon_vma->num_active_vmas == 0) dst->anon_vma = anon_vma; } if (dst->anon_vma) - dst->anon_vma->degree++; + dst->anon_vma->num_active_vmas++; unlock_anon_vma_root(root); return 0; @@ -355,6 +356,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) anon_vma = anon_vma_alloc(); if (!anon_vma) goto out_error; + anon_vma->num_active_vmas++; avc = anon_vma_chain_alloc(GFP_KERNEL); if (!avc) goto out_error_free_anon_vma; @@ -375,7 +377,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma) vma->anon_vma = anon_vma; anon_vma_lock_write(anon_vma); anon_vma_chain_link(vma, avc, anon_vma); - anon_vma->parent->degree++; + anon_vma->parent->num_children++; anon_vma_unlock_write(anon_vma); return 0; @@ -407,7 +409,7 @@ void unlink_anon_vmas(struct vm_area_struct *vma) * to free them outside the lock. */ if (RB_EMPTY_ROOT(&anon_vma->rb_root.rb_root)) { - anon_vma->parent->degree--; + anon_vma->parent->num_children--; continue; } @@ -415,7 +417,7 @@ void unlink_anon_vmas(struct vm_area_struct *vma) anon_vma_chain_free(avc); } if (vma->anon_vma) { - vma->anon_vma->degree--; + vma->anon_vma->num_active_vmas--; /* * vma would still be needed after unlink, and anon_vma will be prepared @@ -433,7 +435,8 @@ void unlink_anon_vmas(struct vm_area_struct *vma) list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) { struct anon_vma *anon_vma = avc->anon_vma; - VM_WARN_ON(anon_vma->degree); + VM_WARN_ON(anon_vma->num_children); + VM_WARN_ON(anon_vma->num_active_vmas); put_anon_vma(anon_vma); list_del(&avc->same_vma);