threads_max: Simple lockout prevention patch

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

fork.c 2005-11-14 20:58:25.000000000 +0300 renamed from fork.c.orig 2005-11-14 20:55:33.000000000 +0300 +1 -0

@@ -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;
 

fork.c 2005-11-14 20:58:25.000000000 +0300 renamed from fork.c.orig 2005-11-14 20:55:33.000000000 +0300 +0 -0

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

*/

Quoted 2 lines · Al Boldi

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.

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 8 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

fork.c 2005-11-14 20:58:25.000000000 +0300

Quoted 2 lines

fork.c 2005-11-14 20:58:25.000000000 +0300 renamed from fork.c.orig 2005-11-14 20:55:33.000000000 +0300 +0 -0

Please prepare patches in `patch -p1' form.

Quoted 3 lines

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.

Quoted 1 line

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

Your email client replaces tabs with spaces.

Quoted 2 lines

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

And it wordwraps.

Quoted 5 lines · 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.

Why not just have the root process do:

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

Like so?

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

Quoted 4 lines · Al Boldi

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 &&.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines · Al Boldi

Quoted 5 lines

Quoted 4 lines

Like so?

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

Quoted 5 lines

Quoted 1 line

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.

What compiler optimizations are you referring to?

Did you study the generated object code?

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.

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.

Quoted 8 lines · Al Boldi

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?

Quoted 11 lines · Nick Piggin

Quoted 8 lines · Al Boldi

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.

Quoted 1 line

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.

Quoted 18 lines · Al Boldi

Quoted 11 lines · Nick Piggin

Quoted 8 lines · Al Boldi

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.

Quoted 2 lines

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.

Quoted 5 lines · Andrew Morton

Quoted 2 lines · Al Boldi

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:

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.