Prevent max_threads from exceeding PID_MAX

PATCH In review Patrick O'Rourke 2 participants 5 messages Mar 7, 2002 – Mar 8, 2002

Subsystems (3)

MM core Scheduler exec and binfmt

lore

Share & tools

lore

Patrick O'Rourke

author

2002-03-07 20:45 UTC #0 lore raw

PATCH 1/1 Prevent max_threads from exceeding PID_MAX

It is possible on large memory systems that the default process limits can exceed PID_MAX. This will allow a non-root user to consume all pids resulting in the kernel to basically hang in get_pid().

The following patch to fork_init() will prevent max_threads from exceeding PID_MAX.

Pat

kernel/fork.c +5 -0

@@ -74,6 +74,11 @@
 	 */
 	max_threads = mempages / (THREAD_SIZE/PAGE_SIZE) / 8;
 
+	/* don't let threads go beyond PID_MAX */
+	if (max_threads > PID_MAX) {
+		max_threads = PID_MAX;
+	}
+
 	init_task.rlim[RLIMIT_NPROC].rlim_cur = max_threads/2;
 	init_task.rlim[RLIMIT_NPROC].rlim_max = max_threads/2;
 }

Paul Larson

2002-03-07 22:12 UTC #1 lore raw

Review

Quoted 10 lines · Patrick O'Rourke

It is possible on large memory systems that the default process limits can exceed PID_MAX. This will allow a non-root user to consume all pids resulting in the kernel to basically hang in get_pid().
(quoted diff) +5 -0
+	/* don't let threads go beyond PID_MAX */
+	if (max_threads > PID_MAX) {
+		max_threads = PID_MAX;
+	}
+

The problem with this approach is that it doesn't take into account pgrp, tgids, etc... I submitted the following patch a couple of weeks ago that fixes the problem a better way.

Thanks,
Paul Larson

kernel/fork.c +14 -1

diff -Naur linux-2.4.18-rc2/kernel/fork.c linux-getpid/kernel/fork.c
@@ -20,6 +20,7 @@
 #include <linux/vmalloc.h>
 #include <linux/completion.h>
 #include <linux/personality.h>
+#include <linux/compiler.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -85,12 +86,13 @@
 {
 	static int next_safe = PID_MAX;
 	struct task_struct *p;
-	int pid;
+	int pid, beginpid;
 
 	if (flags & CLONE_PID)
 		return current->pid;
 
 	spin_lock(&lastpid_lock);
+	beginpid = last_pid;
 	if((++last_pid) & 0xffff8000) {
 		last_pid = 300;		/* Skip daemons etc. */
 		goto inside;

@@ -110,12 +112,16 @@
 						last_pid = 300;
 					next_safe = PID_MAX;
 				}
+				if(unlikely(last_pid == beginpid))
+					goto nomorepids;
 				goto repeat;
 			}
 			if(p->pid > last_pid && next_safe > p->pid)
 				next_safe = p->pid;
 			if(p->pgrp > last_pid && next_safe > p->pgrp)
 				next_safe = p->pgrp;
+			if(p->tgid > last_pid && next_safe > p->tgid)
+				next_safe = p->tgid;
 			if(p->session > last_pid && next_safe > p->session)
 				next_safe = p->session;
 		}

@@ -125,6 +131,11 @@
 	spin_unlock(&lastpid_lock);
 
 	return pid;
+
+nomorepids:
+	read_unlock(&tasklist_lock);
+	spin_unlock(&lastpid_lock);
+	return 0;
 }
 
 static inline int dup_mmap(struct mm_struct * mm)

@@ -620,6 +631,8 @@
 
 	copy_flags(clone_flags, p);
 	p->pid = get_pid(clone_flags);
+	if (p->pid == 0 && current->pid != 0)
+		goto bad_fork_cleanup;
 
 	p->run_list.next = NULL;
 	p->run_list.prev = NULL;

Paul Larson

2002-03-07 22:25 UTC #2 lore raw

PATCH 1/1 Fix for get_pid hang

Quoted 15 lines · Paul Larson

Review
Quoted 10 lines · Patrick O'Rourke
It is possible on large memory systems that the default process limits can exceed PID_MAX. This will allow a non-root user to consume all pids resulting in the kernel to basically hang in get_pid().
(quoted diff) +5 -0
+	/* don't let threads go beyond PID_MAX */
+	if (max_threads > PID_MAX) {
+		max_threads = PID_MAX;
+	}
+
The problem with this approach is that it doesn't take into account pgrp, tgids, etc... I submitted the following patch a couple of weeks ago that fixes the problem a better way.

Quoted 2 lines

Thanks,
Paul Larson

Marcelo, any chance of getting this accepted into the next 2.4.19-pre? It is obviously a bug that is afecting several others as well. I think the LSE team is looking at some performance enhancements to get_pid, but from what I've seen so far they will be working on top of this bug fix. I just verified that the patch still applies cleanly against 2.4.19-pre2.

Thanks,
Paul Larson

Paul Larson

2002-03-07 22:27 UTC #3 lore raw

PATCH 1/1 Fix for get_pid hang

In reply to Paul Larson

Ok, this time with the patch for real. :)

Quoted 15 lines · Paul Larson

Review
Quoted 10 lines · Patrick O'Rourke
It is possible on large memory systems that the default process limits can exceed PID_MAX. This will allow a non-root user to consume all pids resulting in the kernel to basically hang in get_pid().
(quoted diff) +5 -0
+	/* don't let threads go beyond PID_MAX */
+	if (max_threads > PID_MAX) {
+		max_threads = PID_MAX;
+	}
+
The problem with this approach is that it doesn't take into account pgrp, tgids, etc... I submitted the following patch a couple of weeks ago that fixes the problem a better way.

Quoted 2 lines

Thanks,
Paul Larson

Thanks,
Paul Larson

kernel/fork.c +14 -1

diff -Naur linux-2.4.18-rc2/kernel/fork.c linux-getpid/kernel/fork.c
@@ -20,6 +20,7 @@
 #include <linux/vmalloc.h>
 #include <linux/completion.h>
 #include <linux/personality.h>
+#include <linux/compiler.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -85,12 +86,13 @@
 {
 	static int next_safe = PID_MAX;
 	struct task_struct *p;
-	int pid;
+	int pid, beginpid;
 
 	if (flags & CLONE_PID)
 		return current->pid;
 
 	spin_lock(&lastpid_lock);
+	beginpid = last_pid;
 	if((++last_pid) & 0xffff8000) {
 		last_pid = 300;		/* Skip daemons etc. */
 		goto inside;

@@ -110,12 +112,16 @@
 						last_pid = 300;
 					next_safe = PID_MAX;
 				}
+				if(unlikely(last_pid == beginpid))
+					goto nomorepids;
 				goto repeat;
 			}
 			if(p->pid > last_pid && next_safe > p->pid)
 				next_safe = p->pid;
 			if(p->pgrp > last_pid && next_safe > p->pgrp)
 				next_safe = p->pgrp;
+			if(p->tgid > last_pid && next_safe > p->tgid)
+				next_safe = p->tgid;
 			if(p->session > last_pid && next_safe > p->session)
 				next_safe = p->session;
 		}

@@ -125,6 +131,11 @@
 	spin_unlock(&lastpid_lock);
 
 	return pid;
+
+nomorepids:
+	read_unlock(&tasklist_lock);
+	spin_unlock(&lastpid_lock);
+	return 0;
 }
 
 static inline int dup_mmap(struct mm_struct * mm)

@@ -620,6 +631,8 @@
 
 	copy_flags(clone_flags, p);
 	p->pid = get_pid(clone_flags);
+	if (p->pid == 0 && current->pid != 0)
+		goto bad_fork_cleanup;
 
 	p->run_list.next = NULL;
 	p->run_list.prev = NULL;

Patrick O'Rourke

author

2002-03-08 15:05 UTC #4 lore raw

Re: [PATCH] Fix for get_pid hang

In reply to Patrick O'Rourke

Quoted 1 line · Paul Larson

Ok, this time with the patch for real. :)

While I agree with your patch, I still think something should be done in fork_init() to restrict the initial value of max_threads.

We should not rely on the amount of memory alone.

For example, after a fresh reboot on an i386 system with 12gb of ram we get:

[joeuser@simpsons-lisa joeuser]$ id uid=500(joeuser) gid=500(joeuser) groups=500(joeuser) [joeuser@simpsons-lisa joeuser]$ ulimit -a
core file size (blocks) 1000000
data seg size (kbytes) unlimited
file size (blocks) unlimited
max locked memory (kbytes) unlimited
max memory size (kbytes) unlimited
open files 1024
pipe size (512 bytes) 8
stack size (kbytes) 8192
cpu time (seconds) unlimited
max user processes 49152
virtual memory (kbytes) unlimited
[joeuser@simpsons-lisa joeuser]$

I believe it is wrong for max user processes to start off above PID_MAX.

Pat

Signature · 2 lines

Patrick O'Rourke
[email protected]