[PATCH 1/1] threads_max: Simple lockout prevention patch

[PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Simple attempt to provide a backdoor in a process lockout situation.

echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max limit.

Signed-off-by: Al Boldi <a1426z@gawab.com>

---
(patch against 2.6.14)

--- kernel/fork.c.orig  2005-11-14 20:55:33.000000000 +0300
+++ kernel/fork.c       2005-11-14 20:58:25.000000000 +0300
@@ -57,6 +57,7 @@
 int nr_threads;                /* The idle threads do not count.. */
 
 int max_threads;               /* tunable limit on nr_threads */
+int su_pid;                    /* BackDoor pid to exceed limit on nr_threads 
*/
 
 DEFINE_PER_CPU(unsigned long, process_counts) = 0;
 
@@ -926,6 +927,7 @@
         * to stop root fork bombs.
         */
        if (nr_threads >= max_threads)
+       if (p->pid != su_pid)
                goto bad_fork_cleanup_count;
 
        if (!try_module_get(p->thread_info->exec_domain->module))


--- kernel/sysctl.c.orig        2005-11-14 20:58:45.000000000 +0300
+++ kernel/sysctl.c     2005-11-14 21:01:20.000000000 +0300
@@ -57,6 +57,7 @@
 extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern int max_threads;
+extern int su_pid;
 extern int sysrq_enabled;
 extern int core_uses_pid;
 extern int suid_dumpable;
@@ -509,6 +510,14 @@
                .proc_handler   = &proc_dointvec,
        },
        {
+               .ctl_name       = KERN_SU_PID,
+               .procname       = "su-pid",
+               .data           = &su_pid,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = &proc_dointvec,
+       },
+       {
                .ctl_name       = KERN_RANDOM,
                .procname       = "random",
                .mode           = 0555,


--- include/linux/sysctl.h.orig 2005-11-14 20:54:55.000000000 +0300
+++ include/linux/sysctl.h      2005-11-14 20:55:15.000000000 +0300
@@ -146,6 +146,7 @@
        KERN_RANDOMIZE=68, /* int: randomize virtual address space */
        KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core 
*/
        KERN_SPIN_RETRY=70,     /* int: number of spinlock retries */
+       KERN_SU_PID=71, /* int: BackDoor pid to exceed Maximum nr of threads 
in the system */
 };

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

This is a resend, which was ignored before w/o comment.
A comment this time would be helpful. Thanks!

-

Simple attempt to provide a backdoor in a process lockout situation.

echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max limit.

Signed-off-by: Al Boldi <a1426z@gawab.com>

---
(patch against 2.6.14)

--- kernel/fork.c.orig  2005-11-14 20:55:33.000000000 +0300
+++ kernel/fork.c       2005-11-14 20:58:25.000000000 +0300
@@ -57,6 +57,7 @@
 int nr_threads;                /* The idle threads do not count.. */
 
 int max_threads;               /* tunable limit on nr_threads */
+int su_pid;		/* BackDoor pid to exceed limit on nr_threads */
 
 DEFINE_PER_CPU(unsigned long, process_counts) = 0;
 
@@ -926,6 +927,7 @@
         * to stop root fork bombs.
         */
        if (nr_threads >= max_threads)
+       if (p->pid != su_pid)
                goto bad_fork_cleanup_count;
 
        if (!try_module_get(p->thread_info->exec_domain->module))


--- kernel/sysctl.c.orig        2005-11-14 20:58:45.000000000 +0300
+++ kernel/sysctl.c     2005-11-14 21:01:20.000000000 +0300
@@ -57,6 +57,7 @@
 extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern int max_threads;
+extern int su_pid;
 extern int sysrq_enabled;
 extern int core_uses_pid;
 extern int suid_dumpable;
@@ -509,6 +510,14 @@
                .proc_handler   = &proc_dointvec,
        },
        {
+               .ctl_name       = KERN_SU_PID,
+               .procname       = "su-pid",
+               .data           = &su_pid,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = &proc_dointvec,
+       },
+       {
                .ctl_name       = KERN_RANDOM,
                .procname       = "random",
                .mode           = 0555,


--- include/linux/sysctl.h.orig 2005-11-14 20:54:55.000000000 +0300
+++ include/linux/sysctl.h      2005-11-14 20:55:15.000000000 +0300
@@ -146,6 +146,7 @@
        KERN_RANDOMIZE=68, /* int: randomize virtual address space */
        KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core 
*/
        KERN_SPIN_RETRY=70,     /* int: number of spinlock retries */
+       KERN_SU_PID=71, 	/* int: BackDoor pid to exceed Maximum
+				/*	nr of threads in the system */
 };

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

This is a another resend, which was ignored before w/o comment.
Andrew, can you at least comment on it?  Thanks!

-

Simple attempt to provide a backdoor in a process lockout situation.

echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max limit.

Note that this patch incurs zero runtime-overhead.

Signed-off-by: Al Boldi <a1426z@gawab.com>

---
(patch against 2.6.14)

--- kernel/fork.c.orig  2005-11-14 20:55:33.000000000 +0300
+++ kernel/fork.c       2005-11-14 20:58:25.000000000 +0300
@@ -57,6 +57,7 @@
 int nr_threads;                /* The idle threads do not count.. */
 
 int max_threads;               /* tunable limit on nr_threads */
+int su_pid;		/* BackDoor pid to exceed limit on nr_threads */
 
 DEFINE_PER_CPU(unsigned long, process_counts) = 0;
 
@@ -926,6 +927,7 @@
         * to stop root fork bombs.
         */
        if (nr_threads >= max_threads)
+       if (p->pid != su_pid)
                goto bad_fork_cleanup_count;
 
        if (!try_module_get(p->thread_info->exec_domain->module))


--- kernel/sysctl.c.orig        2005-11-14 20:58:45.000000000 +0300
+++ kernel/sysctl.c     2005-11-14 21:01:20.000000000 +0300
@@ -57,6 +57,7 @@
 extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern int max_threads;
+extern int su_pid;
 extern int sysrq_enabled;
 extern int core_uses_pid;
 extern int suid_dumpable;
@@ -509,6 +510,14 @@
                .proc_handler   = &proc_dointvec,
        },
        {
+               .ctl_name       = KERN_SU_PID,
+               .procname       = "su-pid",
+               .data           = &su_pid,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = &proc_dointvec,
+       },
+       {
                .ctl_name       = KERN_RANDOM,
                .procname       = "random",
                .mode           = 0555,


--- include/linux/sysctl.h.orig 2005-11-14 20:54:55.000000000 +0300
+++ include/linux/sysctl.h      2005-11-14 20:55:15.000000000 +0300
@@ -146,6 +146,7 @@
        KERN_RANDOMIZE=68, /* int: randomize virtual address space */
        KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core 
*/
        KERN_SPIN_RETRY=70,     /* int: number of spinlock retries */
+       KERN_SU_PID=71, 	/* int: BackDoor pid to exceed Maximum
+				/*	nr of threads in the system */
 };

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Andrew Morton]

Al Boldi <a1426z@gawab.com> wrote:
>
> This is a another resend, which was ignored before w/o comment.
> Andrew, can you at least comment on it?  Thanks!
> 

I don't have a clue what it's for.

> 
> Simple attempt to provide a backdoor in a process lockout situation.
> 
> echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max limit.
> 
> Note that this patch incurs zero runtime-overhead.
> 
> Signed-off-by: Al Boldi <a1426z@gawab.com>
> 
> ---
> (patch against 2.6.14)
> 
> --- kernel/fork.c.orig  2005-11-14 20:55:33.000000000 +0300
> +++ kernel/fork.c       2005-11-14 20:58:25.000000000 +0300

Please prepare patches in `patch -p1' form.

> @@ -57,6 +57,7 @@
>  int nr_threads;                /* The idle threads do not count.. */
>  
>  int max_threads;               /* tunable limit on nr_threads */
> +int su_pid;		/* BackDoor pid to exceed limit on nr_threads */
>  
>  DEFINE_PER_CPU(unsigned long, process_counts) = 0;
>  
> @@ -926,6 +927,7 @@
>          * to stop root fork bombs.
>          */
>         if (nr_threads >= max_threads)
> +       if (p->pid != su_pid)
>                 goto bad_fork_cleanup_count;

We don't lay code out in that manner.  Not even vaguely.

This check comes after the RLIMIT_PROC check, which is supposed to
eliminate "process lockout situations", although you haven't really defined
that.

>         if (!try_module_get(p->thread_info->exec_domain->module))

Your email client replaces tabs with spaces.

>         KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid core 
> */

And it wordwraps.

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Andrew Morton wrote:
> Al Boldi <a1426z@gawab.com> wrote:
> > This is a another resend, which was ignored before w/o comment.
> > Andrew, can you at least comment on it?  Thanks!
>
> I don't have a clue what it's for.

Quoting from the 'Resource limits' thread on lkml on 27/09/05:
>>>>> Consider this dilemma:
>>>>> Runaway proc/s hit the limit.
>>>>> Try to kill some and you are denied due to the resource limit.
>>>>> Use some previously running app like top, hope it hasn't been killed
>>>>> by some OOM situation, try killing some procs and another one takes
>>>>> it's place because of the runaway situation.
>>>>> Raise the limit, and it gets filled by the runaways.
>>>>> You are pretty much stuck.
>>>>
>>>> Not really, this is the sort of thing ulimit is meant for.  To keep
>>>> processes from any one user from running away.  It lets you limit the
>>>> damage it can do, until such time as you can control it and fix the
>>>> runaway application.
>>>
>>> threads-max = 1024
>>> ulimit = 100 forks
>>> 11 runaway procs hitting the threads-max limit
>>
>> This is incorrect.  If you ulimit a user to 100 forks, and 11 processes
>> running with that uid
> 
> Different uid.
> 
Then yes, if you set a system-wide limit that is less than the sum of the 
limits imposed on each accountable part of the system you can have lock out.  
But thats your fault for misconfiguring the system.  Don't do that.

-- end of quote

> > --- kernel/fork.c.orig  2005-11-14 20:55:33.000000000 +0300
> > +++ kernel/fork.c       2005-11-14 20:58:25.000000000 +0300
>
> Please prepare patches in `patch -p1' form.

Ok.

> >         if (nr_threads >= max_threads)
> > +       if (p->pid != su_pid)
> >                 goto bad_fork_cleanup_count;
>
> We don't lay code out in that manner.  Not even vaguely.

Like so?
	if (nr_threads >= max_threads)
		if (p->pid != su_pid)
			goto bad_fork_cleanup_count;

> This check comes after the RLIMIT_PROC check, which is supposed to
> eliminate "process lockout situations", although you haven't really
> defined that.

RLIMIT_PROC is per user, this patch solves the global limit.

> >         if (!try_module_get(p->thread_info->exec_domain->module))
>
> Your email client replaces tabs with spaces.
>
> >         KERN_SETUID_DUMPABLE=69, /* int: behaviour of dumps for setuid
> > core */
>
> And it wordwraps.

I am using kmail, it does this when the mail is queued before sending it.  
Maybe somebody can point out how to instruct kmail not to do that?

Or maybe use an attachment?

Theodore Ts'o wrote:
> On Mon, Apr 24, 2006 at 07:56:42AM +0300, Al Boldi wrote:
>> 
>> Simple attempt to provide a backdoor in a process lockout situation.
>> 
>> echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max
>> limit.
>
> Why not just have the root process do:
>
> echo {larger number} > /proc/sys/kernel/nr-threads instead?

Could do that by:

	# echo 1 > /proc/sys/kernel/su-pid

which would imply nr-threads = 1
	
So maybe introduce /proc/sys/kernel/nr-threads to allow that to be variable, 
but this isn't really critical.

Thanks!

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Pekka Enberg]

On 4/24/06, Al Boldi <a1426z@gawab.com> wrote:
> Like so?
>         if (nr_threads >= max_threads)
>                 if (p->pid != su_pid)
>                         goto bad_fork_cleanup_count;

It's better to combine the two if statements into one with &&.

                                  Pekka

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Pekka Enberg wrote:
> On 4/24/06, Al Boldi <a1426z@gawab.com> wrote:
> > Like so?
> >         if (nr_threads >= max_threads)
> >                 if (p->pid != su_pid)
> >                         goto bad_fork_cleanup_count;
>
> It's better to combine the two if statements into one with &&.

I thought of combining them too, but was afraid of some compile optimization 
issues.  Remember, this code-path is executed for each and every fork in the 
system, thus it's highly performance sensitive.

Thanks!

--
Al

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Pekka Enberg]

On 4/24/06, Al Boldi <a1426z@gawab.com> wrote:
> > > Like so?
> > >         if (nr_threads >= max_threads)
> > >                 if (p->pid != su_pid)
> > >                         goto bad_fork_cleanup_count;
> >
> > It's better to combine the two if statements into one with &&.

On Mon, 2006-04-24 at 16:53 +0300, Al Boldi wrote:
> I thought of combining them too, but was afraid of some compile optimization 
> issues.  Remember, this code-path is executed for each and every fork in the 
> system, thus it's highly performance sensitive.

There shouldn't be any difference. What compiler optimizations are you
referring to? Did you study the generated object code?

				Pekka

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Pekka Enberg wrote:
> On 4/24/06, Al Boldi <a1426z@gawab.com> wrote:
> > > > Like so?
> > > >         if (nr_threads >= max_threads)
> > > >                 if (p->pid != su_pid)
> > > >                         goto bad_fork_cleanup_count;
> > >
> > > It's better to combine the two if statements into one with &&.
>
> > I thought of combining them too, but was afraid of some compile
> > optimization issues.  Remember, this code-path is executed for each and
> > every fork in the system, thus it's highly performance sensitive.
>
> There shouldn't be any difference.

There shouldn't, if things were perfect.

> What compiler optimizations are you referring to?

-O3 at least.

> Did you study the generated object code?

Not really.

But -O3 creates faster code w/ some strange flaws like failing nfs-boot.
Maybe that's fixed in the latest gcc, but gcc-3.2.2 was exhibiting this bug.
So this doesn't really help a developer to be confident about compiler 
optimization, thus taking the safe route for performance sensitive 
code-paths.

Thanks!

--
Al

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Pekka Enberg]

On Mon, 2006-04-24 at 17:46 +0300, Al Boldi wrote:
> But -O3 creates faster code w/ some strange flaws like failing nfs-boot.
> Maybe that's fixed in the latest gcc, but gcc-3.2.2 was exhibiting this bug.
> So this doesn't really help a developer to be confident about compiler 
> optimization, thus taking the safe route for performance sensitive 
> code-paths.

Are you talking about a real GCC bug which you must work around? If not,
please don't obfuscate the code for no good reason.

				Pekka

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Nick Piggin]

Al Boldi wrote:

> Could do that by:
> 
> 	# echo 1 > /proc/sys/kernel/su-pid
> 
> which would imply nr-threads = 1
> 	
> So maybe introduce /proc/sys/kernel/nr-threads to allow that to be variable, 
> but this isn't really critical.

Why not just have su-nr-threads?

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Nick Piggin wrote:
> Al Boldi wrote:
> > Could do that by:
> >
> > 	# echo 1 > /proc/sys/kernel/su-pid
> >
> > which would imply nr-threads = 1
> >
> > So maybe introduce /proc/sys/kernel/nr-threads to allow that to be
> > variable, but this isn't really critical.
>
> Why not just have su-nr-threads?

Unless I am misunderstanding you, even root/root-proc can be hit by a 
runaway, so the threads-max limits this globally which is great, but this 
may lock-you out of being able to control the situation based on uid only.

Thus this patch gives root the ability to allow a certain pid to exceed the 
threads-max limit, while all other pids are still limited.

Your comments are much appreciated!

Thanks!

--
Al

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Nick Piggin]

Al Boldi wrote:
> Nick Piggin wrote:
> 
>>Al Boldi wrote:
>>
>>>Could do that by:
>>>
>>>	# echo 1 > /proc/sys/kernel/su-pid
>>>
>>>which would imply nr-threads = 1
>>>
>>>So maybe introduce /proc/sys/kernel/nr-threads to allow that to be
>>>variable, but this isn't really critical.
>>
>>Why not just have su-nr-threads?
> 
> 
> Unless I am misunderstanding you, even root/root-proc can be hit by a 
> runaway, so the threads-max limits this globally which is great, but this 
> may lock-you out of being able to control the situation based on uid only.
> 
> Thus this patch gives root the ability to allow a certain pid to exceed the 
> threads-max limit, while all other pids are still limited.

But the point is that root is able to get their pids under control,
and can't be DoSed by unpriv users. Right?

Nothing is going to be perfect, I mean the su-pid pid could get "hit
bya runaway" and is arguably worse than nr-threads-su, because it has
no upper limit and coult take down the whole system.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Nick Piggin wrote:
> Al Boldi wrote:
> > Nick Piggin wrote:
> >>Al Boldi wrote:
> >>>Could do that by:
> >>>
> >>>	# echo 1 > /proc/sys/kernel/su-pid
> >>>
> >>>which would imply nr-threads = 1
> >>>
> >>>So maybe introduce /proc/sys/kernel/nr-threads to allow that to be
> >>>variable, but this isn't really critical.
> >>
> >>Why not just have su-nr-threads?
> >
> > Unless I am misunderstanding you, even root/root-proc can be hit by a
> > runaway, so the threads-max limits this globally which is great, but
> > this may lock-you out of being able to control the situation based on
> > uid only.
> >
> > Thus this patch gives root the ability to allow a certain pid to exceed
> > the threads-max limit, while all other pids are still limited.
>
> But the point is that root is able to get their pids under control,
> and can't be DoSed by unpriv users. Right?

Or even by root.

> Nothing is going to be perfect, I mean the su-pid pid could get "hit
> bya runaway" and is arguably worse than nr-threads-su, because it has
> no upper limit and coult take down the whole system.

The su-pid is a temporary measure set by root after evaluating the current 
situation, and additionally limiting this by su-nr(pid)-threads may probably 
be a good idea.  Maybe something like this:

	if (nr_threads >= max_threads)
		if ((p->pid != su_pid) || (nr_threads >= (max_threads + su_pid_threads)))
			goto bad_fork_cleanup_count;

But I really don't think this is critical.

Thanks!

--
Al

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Al Boldi]

Al Boldi wrote:
> Andrew Morton wrote:
> > Al Boldi <a1426z@gawab.com> wrote:
> > > This is a another resend, which was ignored before w/o comment.
> > > Andrew, can you at least comment on it?  Thanks!
> >
> > I don't have a clue what it's for.
>
> Quoting from the 'Resource limits' thread on lkml on 27/09/05:

Has this cleared things up, or is this patch still deemed useless?

Thanks!

--
Al

Re: [PATCH 1/1] threads_max: Simple lockout prevention patch

[Theodore Ts'o]

On Mon, Apr 24, 2006 at 07:56:42AM +0300, Al Boldi wrote:
> 
> Simple attempt to provide a backdoor in a process lockout situation.
> 
> echo $$ > /proc/sys/kernel/su-pid allows pid to exceed the threads_max limit.

Why not just have the root process do:

echo {larger number} > /proc/sys/kernel/nr-threads

instead?  

						- Ted