[PATCH] Prevent max_threads from exceeding PID_MAX

[PATCH] Prevent max_threads from exceeding PID_MAX

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

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

Pat

--- linux-2.4.19-pre2-x/kernel/fork.c	Mon Feb 25 14:38:13 2002
+++ linux-2.4.19-pre2/kernel/fork.c	Wed Mar  6 09:15:17 2002
@@ -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;
 }

Re: [PATCH] Prevent max_threads from exceeding PID_MAX

[Paul Larson]

On Thu, 2002-03-07 at 14:45, Patrick O'Rourke wrote:
> 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().
> 
  
> +	/* 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

diff -Naur linux-2.4.18-rc2/kernel/fork.c linux-getpid/kernel/fork.c
--- linux-2.4.18-rc2/kernel/fork.c	Wed Feb 20 09:54:39 2002
+++ linux-getpid/kernel/fork.c	Fri Feb 22 15:52:52 2002
@@ -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;

[PATCH] Fix for get_pid hang

[Paul Larson]

On Thu, 2002-03-07 at 16:11, Paul Larson wrote:
> On Thu, 2002-03-07 at 14:45, Patrick O'Rourke wrote:
> > 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().
> > 
>   
> > +	/* 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

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

[PATCH] Fix for get_pid hang

[Paul Larson]

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

On Thu, 2002-03-07 at 16:11, Paul Larson wrote:
> On Thu, 2002-03-07 at 14:45, Patrick O'Rourke wrote:
> > 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().
> > 
>   
> > +	/* 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

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

diff -Naur linux-2.4.18-rc2/kernel/fork.c linux-getpid/kernel/fork.c
--- linux-2.4.18-rc2/kernel/fork.c	Wed Feb 20 09:54:39 2002
+++ linux-getpid/kernel/fork.c	Fri Feb 22 15:52:52 2002
@@ -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;

Re: [PATCH] Fix for get_pid hang

[Patrick O'Rourke]

Paul Larson wrote:
> 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

-- 
Patrick O'Rourke
porourke@egenera.com