Re: [PATCH] Repeated fork() causes SLAB to grow without bound

[Konstantin Khlebnikov <koct9i@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

On Wed, Nov 19, 2014 at 1:15 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
> On Tue, Nov 18, 2014 at 11:19 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon, 17 Nov 2014 21:41:57 -0500 Rik van Riel <riel@redhat.com> wrote:
>>
>>> > Because of the serial forking there does indeed end up being an
>>> > infinite number of vmas.  The initial vma can never be deleted
>>> > (even though the initial parent process has long since terminated)
>>> > because the initial vma is referenced by the children.
>>>
>>> There is a finite number of VMAs, but an infite number of
>>> anon_vmas.
>>>
>>> Subtle, yet deadly...
>>
>> Well, we clearly have the data structures screwed up.  I've forgotten
>> enough about this code for me to be unable to work out what the fixed
>> up data structures would look like :( But surely there is some proper
>> solution here.  Help?
>
> Not sure if it's right but probably we could reuse on fork an old anon_vma
> from the chain if it's already lost all vmas which points to it.
> For endlessly forking exploit this should work mostly like proposed patch
> which stops branching after some depth but without magic constant.

Something like this. I leave proper comment for tomorrow.

>
>>
>>> > I can't say, but it only affects users who fork more than five
>>> > levels deep without doing an exec.  On the other hand, there are at
>>> > least three users (Tim Hartrick, Michal Hocko, and myself) who have
>>> > real world applications where the consequence of no patch is a
>>> > crashed system.
>>> >
>>> > I would suggest reading the thread starting with my initial bug
>>> > report for what others have had to say about this.
>>>
>>> I suspect what Andrew is hinting at is that the
>>> changelog for the patch should contain a detailed
>>> description of exactly what the bug is, how it is
>>> triggered, what the symptoms are, and how the
>>> patch avoids it.
>>>
>>> That way people can understand what the code does
>>> simply by looking at the changelog - no need to go
>>> find old linux-kernel mailing list threads.
>>
>> Yes please, there's a ton of stuff here which we should attempt to
>> capture.
>>
>> https://lkml.org/lkml/2012/8/15/765 is useful.
>>
>> I'm assuming that with the "foo < 5" hack, an application which forked
>> 5 times then did a lot of work would still trigger the "catastrophic
>> issue at page reclaim time" issue which Rik identified at
>> https://lkml.org/lkml/2012/8/20/265?
>>
>> There are real-world workloads which are triggering this slab growth
>> problem, yes?  (Detail them in the changelog, please).
>>
>> This bug snuck under my radar last time - we're permitting unprivileged
>> userspace to exhaust memory and that's bad.  I'm OK with the foo<5
>> thing for -stable kernels, as it is simple.  But I'm reluctant to merge
>> (or at least to retain) it in mainline because then everyone will run
>> away and think about other stuff and this bug will never get fixed
>> properly.
>>
>> --
>> 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>

[-- Attachment #2: mm-reuse-old-anon_vma-if-it-s-lost-all-vmas --]
[-- Type: application/octet-stream, Size: 2362 bytes --]

mm: reuse old anon_vma if it's lost all vmas

From: Konstantin Khlebnikov <koct9i@gmail.com>

Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
---
 include/linux/rmap.h |    2 ++
 mm/rmap.c            |   14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index c0c2bce..d40ca08 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -36,6 +36,8 @@ struct anon_vma {
 	 */
 	atomic_t refcount;
 
+	int nr_vmas;	/* Number of direct references from vmas */
+
 	/*
 	 * NOTE: the LSB of the rb_root.rb_node is set by
 	 * mm_take_all_locks() _after_ taking the above lock. So the
diff --git a/mm/rmap.c b/mm/rmap.c
index 19886fb..ced4754 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -72,6 +72,7 @@ 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->nr_vmas = 1;
 		/*
 		 * Initialise the anon_vma root to point to itself. If called
 		 * from fork, the root will be reset to the parents anon_vma.
@@ -256,7 +257,11 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 		anon_vma = pavc->anon_vma;
 		root = lock_anon_vma_root(root, anon_vma);
 		anon_vma_chain_link(dst, avc, anon_vma);
+		if (!dst->anon_vma && !anon_vma->nr_vmas)
+			dst->anon_vma = anon_vma;
 	}
+	if (dst->anon_vma)
+		dst->anon_vma->nr_vmas++;
 	unlock_anon_vma_root(root);
 	return 0;
 
@@ -279,6 +284,9 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	if (!pvma->anon_vma)
 		return 0;
 
+	/* Drop parent anon_vma, we want find or allocate our own. */
+	vma->anon_vma = NULL;
+
 	/*
 	 * First, attach the new VMA to the parent VMA's anon_vmas,
 	 * so rmap can find non-COWed pages in child processes.
@@ -286,6 +294,10 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	if (anon_vma_clone(vma, pvma))
 		return -ENOMEM;
 
+	/* Old anon_vma has been reused. */
+	if (vma->anon_vma)
+		return 0;
+
 	/* Then add our own anon_vma. */
 	anon_vma = anon_vma_alloc();
 	if (!anon_vma)
@@ -345,6 +357,8 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
 		list_del(&avc->same_vma);
 		anon_vma_chain_free(avc);
 	}
+	if (vma->anon_vma)
+		vma->anon_vma->nr_vmas--;
 	unlock_anon_vma_root(root);
 
 	/*