[PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Lennart Poettering]

(Nobody commented so far, but here's an updated version of the
patch. Basically the only change is that the SCHED_RESET_ON_FORK flag
is now reset on fork()).)

This patch introduces a new flag SCHED_RESET_ON_FORK which can be passed
to the kernel via sched_setscheduler(), ORed in the policy parameter. If
set this will make sure that when the process forks a) the scheduling
priority is reset to DEFAULT_PRIO if it was higher and b) the scheduling
policy is reset to SCHED_NORMAL if it was either SCHED_FIFO or SCHED_RR.

Why have this?

Currently, if a process is real-time scheduled this will 'leak' to all
its child processes. For security reasons it is often (always?) a good
idea to make sure that if a process acquires RT scheduling this is
confined to this process and only this process. More specifically this
makes the per-process resource limit RLIMIT_RTTIME useful for security
purposes, because it makes it impossible to use a fork bomb to
circumvent the per-process RLIMIT_RTTIME accounting.

This feature is also useful for tools like 'renice' which can then
change the nice level of a process without having this spill to all its
child processes.

Why expose this via sched_setscheduler() and not other syscalls such as
prctl() or sched_setparam()?

prctl() does not take a pid parameter. Due to that it would be
impossible to modify this flag for other processes than the current one.

The struct passed to sched_setparam() can unfortunately not be extended
without breaking compatibility, since sched_setparam() lacks a size
parameter.

How to use this from userspace? In your RT program simply replace this:

  sched_setscheduler(pid, SCHED_FIFO, &param);

by this:

  sched_setscheduler(pid, SCHED_FIFO|SCHED_RESET_ON_FORK, &param);

This current patch compiles but is not otherwise tested. For now, I am
sending this primarily as a request for comments. So please, I'd love to
hear your comments!

Lennart
---
 include/linux/sched.h |    4 ++++
 kernel/sched.c        |   47 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b4c38bc..395ebc4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -38,6 +38,8 @@
 #define SCHED_BATCH		3
 /* SCHED_ISO: reserved but not implemented yet */
 #define SCHED_IDLE		5
+/* Can be ORed in to make sure the process is reverted back to SCHED_NORMAL on fork */
+#define SCHED_RESET_ON_FORK     0x40000000
 
 #ifdef __KERNEL__
 
@@ -1429,6 +1431,8 @@ struct task_struct {
 	/* state flags for use by tracers */
 	unsigned long trace;
 #endif
+	/* Revert to default priority/policy when forking */
+	unsigned sched_reset_on_fork:1;
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/sched.c b/kernel/sched.c
index 26efa47..2a2eb07 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2529,12 +2529,26 @@ void sched_fork(struct task_struct *p, int clone_flags)
 	set_task_cpu(p, cpu);
 
 	/*
-	 * Make sure we do not leak PI boosting priority to the child:
+	 * Revert to default priority/policy on fork if requested. Make sure we
+	 * do not leak PI boosting priority to the child.
 	 */
-	p->prio = current->normal_prio;
+	if (current->sched_reset_on_fork &&
+			(p->policy == SCHED_FIFO || p->policy == SCHED_RR))
+		p->policy = SCHED_NORMAL;
+
+	if (current->sched_reset_on_fork &&
+			(current->normal_prio < DEFAULT_PRIO))
+		p->prio = DEFAULT_PRIO;
+	else
+		p->prio = current->normal_prio;
+
 	if (!rt_prio(p->prio))
 		p->sched_class = &fair_sched_class;
 
+	/* We don't need the reset flag anymore after the fork. It has
+	 * fulfilled its duty. */
+	p->sched_reset_on_fork = 0;
+
 #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
 	if (likely(sched_info_on()))
 		memset(&p->sched_info, 0, sizeof(p->sched_info));
@@ -5807,17 +5821,25 @@ static int __sched_setscheduler(struct task_struct *p, int policy,
 	unsigned long flags;
 	const struct sched_class *prev_class = p->sched_class;
 	struct rq *rq;
+	int reset_on_fork;
 
 	/* may grab non-irq protected spin_locks */
 	BUG_ON(in_interrupt());
 recheck:
 	/* double check policy once rq lock held */
-	if (policy < 0)
+	if (policy < 0) {
+		reset_on_fork = p->sched_reset_on_fork;
 		policy = oldpolicy = p->policy;
-	else if (policy != SCHED_FIFO && policy != SCHED_RR &&
-			policy != SCHED_NORMAL && policy != SCHED_BATCH &&
-			policy != SCHED_IDLE)
-		return -EINVAL;
+	} else {
+		reset_on_fork = !!(policy & SCHED_RESET_ON_FORK);
+		policy &= ~SCHED_RESET_ON_FORK;
+
+		if (policy != SCHED_FIFO && policy != SCHED_RR &&
+				policy != SCHED_NORMAL && policy != SCHED_BATCH &&
+				policy != SCHED_IDLE)
+			return -EINVAL;
+	}
+
 	/*
 	 * Valid priorities for SCHED_FIFO and SCHED_RR are
 	 * 1..MAX_USER_RT_PRIO-1, valid priority for SCHED_NORMAL,
@@ -5861,6 +5883,10 @@ recheck:
 		/* can't change other user's priorities */
 		if (!check_same_owner(p))
 			return -EPERM;
+
+		/* normal users shall not reset the sched_reset_on_fork flag */
+		if (p->sched_reset_on_fork && !reset_on_fork)
+			return -EPERM;
 	}
 
 	if (user) {
@@ -5904,6 +5930,8 @@ recheck:
 	if (running)
 		p->sched_class->put_prev_task(rq, p);
 
+	p->sched_reset_on_fork = reset_on_fork;
+
 	oldprio = p->prio;
 	__setscheduler(rq, p, policy, param->sched_priority);
 
@@ -6020,14 +6048,15 @@ SYSCALL_DEFINE1(sched_getscheduler, pid_t, pid)
 	if (p) {
 		retval = security_task_getscheduler(p);
 		if (!retval)
-			retval = p->policy;
+			retval = p->policy
+				| (p->sched_reset_on_fork ? SCHED_RESET_ON_FORK : 0);
 	}
 	read_unlock(&tasklist_lock);
 	return retval;
 }
 
 /**
- * sys_sched_getscheduler - get the RT priority of a thread
+ * sys_sched_getparam - get the RT priority of a thread
  * @pid: the pid in question.
  * @param: structure containing the RT priority.
  */
-- 
1.6.2.2



Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Peter Zijlstra]

On Fri, 2009-05-29 at 10:38 +0200, Lennart Poettering wrote:

> This patch introduces a new flag SCHED_RESET_ON_FORK which can be passed
> to the kernel via sched_setscheduler(), ORed in the policy parameter. If
> set this will make sure that when the process forks a) the scheduling
> priority is reset to DEFAULT_PRIO if it was higher and b) the scheduling
> policy is reset to SCHED_NORMAL if it was either SCHED_FIFO or SCHED_RR.

Maybe SCHED_OTHER_ON_FORK, or SCHED_DEFAULT_ON_FORK?

> Why have this?
> 
> Currently, if a process is real-time scheduled this will 'leak' to all
> its child processes. For security reasons it is often (always?) a good
> idea to make sure that if a process acquires RT scheduling this is
> confined to this process and only this process. More specifically this
> makes the per-process resource limit RLIMIT_RTTIME useful for security
> purposes, because it makes it impossible to use a fork bomb to
> circumvent the per-process RLIMIT_RTTIME accounting.
> 
> This feature is also useful for tools like 'renice' which can then
> change the nice level of a process without having this spill to all its
> child processes.
> 
> Why expose this via sched_setscheduler() and not other syscalls such as
> prctl() or sched_setparam()?
> 
> prctl() does not take a pid parameter. Due to that it would be
> impossible to modify this flag for other processes than the current one.

Yeah, same goes for rlimits, I think someone once proposed to allow
changing them in /proc/$pid/limits but I'm not sure what happened to
that proposal.

> The struct passed to sched_setparam() can unfortunately not be extended
> without breaking compatibility, since sched_setparam() lacks a size
> parameter.

Yeah, we're going to have to cross that bridge at some point through,
advanced scheduling policies really need more information. But yeah,
using the upper bits of the policy might work for this.

> How to use this from userspace? In your RT program simply replace this:
> 
>   sched_setscheduler(pid, SCHED_FIFO, &param);
> 
> by this:
> 
>   sched_setscheduler(pid, SCHED_FIFO|SCHED_RESET_ON_FORK, &param);
> 
> This current patch compiles but is not otherwise tested. For now, I am
> sending this primarily as a request for comments. So please, I'd love to
> hear your comments!

Seems reasonable, Ingo?

comments on the implementation below.

> Lennart
> ---
>  include/linux/sched.h |    4 ++++
>  kernel/sched.c        |   47 ++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b4c38bc..395ebc4 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -38,6 +38,8 @@
>  #define SCHED_BATCH		3
>  /* SCHED_ISO: reserved but not implemented yet */
>  #define SCHED_IDLE		5
> +/* Can be ORed in to make sure the process is reverted back to SCHED_NORMAL on fork */
> +#define SCHED_RESET_ON_FORK     0x40000000
>  
>  #ifdef __KERNEL__
>  
> @@ -1429,6 +1431,8 @@ struct task_struct {
>  	/* state flags for use by tracers */
>  	unsigned long trace;
>  #endif
> +	/* Revert to default priority/policy when forking */
> +	unsigned sched_reset_on_fork:1;
>  };

There seems to be a bit hole around in_execve, maybe add it there.
 
>  /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 26efa47..2a2eb07 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -2529,12 +2529,26 @@ void sched_fork(struct task_struct *p, int clone_flags)
>  	set_task_cpu(p, cpu);
>  
>  	/*
> -	 * Make sure we do not leak PI boosting priority to the child:
> +	 * Revert to default priority/policy on fork if requested. Make sure we
> +	 * do not leak PI boosting priority to the child.
>  	 */
> -	p->prio = current->normal_prio;
> +	if (current->sched_reset_on_fork &&
> +			(p->policy == SCHED_FIFO || p->policy == SCHED_RR))
> +		p->policy = SCHED_NORMAL;
> +
> +	if (current->sched_reset_on_fork &&
> +			(current->normal_prio < DEFAULT_PRIO))
> +		p->prio = DEFAULT_PRIO;
> +	else
> +		p->prio = current->normal_prio;

This bit hurt my brain, what's wrong with:

	p->prio = current->normal_prio; /* don't leak boosted prio */
	if (current->sched_reset_on_fork) {
		p->policy = SCHED_NORMAL;
		p->prio = p->normal_prio = p->static_prio = DEFAULT_PRIO;
	}

>  	if (!rt_prio(p->prio))
>  		p->sched_class = &fair_sched_class;
>  
> +	/* We don't need the reset flag anymore after the fork. It has
> +	 * fulfilled its duty. */
> +	p->sched_reset_on_fork = 0;
> +
>  #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
>  	if (likely(sched_info_on()))
>  		memset(&p->sched_info, 0, sizeof(p->sched_info));

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Lennart Poettering]

On Fri, 29.05.09 11:28, Peter Zijlstra (peterz@infradead.org) wrote:

> > This patch introduces a new flag SCHED_RESET_ON_FORK which can be passed
> > to the kernel via sched_setscheduler(), ORed in the policy parameter. If
> > set this will make sure that when the process forks a) the scheduling
> > priority is reset to DEFAULT_PRIO if it was higher and b) the scheduling
> > policy is reset to SCHED_NORMAL if it was either SCHED_FIFO or SCHED_RR.
> 
> Maybe SCHED_OTHER_ON_FORK, or SCHED_DEFAULT_ON_FORK?

Hmm, I think this would be misleading, since it does more than just
reset to SCHED_OTHER. It also resets the nice level.

> > +	if (current->sched_reset_on_fork &&
> > +			(p->policy == SCHED_FIFO || p->policy == SCHED_RR))
> > +		p->policy = SCHED_NORMAL;
> > +
> > +	if (current->sched_reset_on_fork &&
> > +			(current->normal_prio < DEFAULT_PRIO))
> > +		p->prio = DEFAULT_PRIO;
> > +	else
> > +		p->prio = current->normal_prio;
> 
> This bit hurt my brain, what's wrong with:
> 
> 	p->prio = current->normal_prio; /* don't leak boosted prio */
> 	if (current->sched_reset_on_fork) {
> 		p->policy = SCHED_NORMAL;
> 		p->prio = p->normal_prio = p->static_prio = DEFAULT_PRIO;
> 	}

This would do something very different. i.e. with this suggested code
of yours SCHED_IDLE/SCHED_BATCH would be upgraded to SCHED_OTHER on
fork and a process with a positive nice level would be upgraded to to
nice level 0. My original patch would only downgrade, never upgrade.

It is of course a question what makes more sense here: a logic that
makes sure that only "elevated" scheduling gets reset to normal
scheduling, or one that would reset both 'higher' and 'lower'
scheduling settings to normal.

Given that this is supposed to be a security feature it makes more
sense to me to only 'decrease' scheduling settings, not 'increase'
them. Dunno, what do you think?

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Lennart Poettering]

On Fri, 29.05.09 10:38, Lennart Poettering (mzxreary@0pointer.de) wrote:

> This patch introduces a new flag SCHED_RESET_ON_FORK which can be passed
> to the kernel via sched_setscheduler(), ORed in the policy parameter. If
> set this will make sure that when the process forks a) the scheduling
> priority is reset to DEFAULT_PRIO if it was higher and b) the scheduling
> policy is reset to SCHED_NORMAL if it was either SCHED_FIFO or SCHED_RR.

[...]

> This current patch compiles but is not otherwise tested. For now, I am
> sending this primarily as a request for comments. So please, I'd love to
> hear your comments!

I have now done some testing to make sure it actually works as
advertised. Everything seems to be fine. For those interested here's a
little test program that verifies that RR scheduling is properly
removed from the child process but is kept enabled in the parent process:

http://0pointer.de/public/reset-on-fork.c
 
Ingo, how can I bribe you into taking this patch, or at least
commenting on it?

Thanks,

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Ingo Molnar]

* Lennart Poettering <mzxreary@0pointer.de> wrote:

> Why expose this via sched_setscheduler() and not other syscalls 
> such as prctl() or sched_setparam()?
> 
> prctl() does not take a pid parameter. Due to that it would be 
> impossible to modify this flag for other processes than the 
> current one.
> 
> The struct passed to sched_setparam() can unfortunately not be 
> extended without breaking compatibility, since sched_setparam() 
> lacks a size parameter.

Well, it could be extended, if we wanted to. Right now 
sched_priority has a valid value of 0 to 100. We could introduce a 
new value '-1' to mean: 'extended struct sched_param'.

A new getparam syscall could then be introduced - only used by new 
user-space.

	Ingo

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Lennart Poettering]

On Fri, 05.06.09 13:54, Ingo Molnar (mingo@elte.hu) wrote:

> 
> 
> * Lennart Poettering <mzxreary@0pointer.de> wrote:
> 
> > Why expose this via sched_setscheduler() and not other syscalls 
> > such as prctl() or sched_setparam()?
> > 
> > prctl() does not take a pid parameter. Due to that it would be 
> > impossible to modify this flag for other processes than the 
> > current one.
> > 
> > The struct passed to sched_setparam() can unfortunately not be 
> > extended without breaking compatibility, since sched_setparam() 
> > lacks a size parameter.
> 
> Well, it could be extended, if we wanted to. Right now 
> sched_priority has a valid value of 0 to 100. We could introduce a 
> new value '-1' to mean: 'extended struct sched_param'.
> 
> A new getparam syscall could then be introduced - only used by new 
> user-space.

The man page of sched_getparam() is pretty explicit in that the
sched_priority field of the struct must lie between
sched_get_priority_min() and sched_get_priority_max(). If you'd
overload sched_priority like this you might end up breaking applications
that rely on this, for example RT watchdogs that go through /proc and
query the scheduling parameters of all threads.

Any further comments on the patch? Could this be merged? Any changes
necessary?

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Ingo Molnar]

* Lennart Poettering <mzxreary@0pointer.de> wrote:

> On Fri, 05.06.09 13:54, Ingo Molnar (mingo@elte.hu) wrote:
> 
> > 
> > 
> > * Lennart Poettering <mzxreary@0pointer.de> wrote:
> > 
> > > Why expose this via sched_setscheduler() and not other syscalls 
> > > such as prctl() or sched_setparam()?
> > > 
> > > prctl() does not take a pid parameter. Due to that it would be 
> > > impossible to modify this flag for other processes than the 
> > > current one.
> > > 
> > > The struct passed to sched_setparam() can unfortunately not be 
> > > extended without breaking compatibility, since sched_setparam() 
> > > lacks a size parameter.
> > 
> > Well, it could be extended, if we wanted to. Right now 
> > sched_priority has a valid value of 0 to 100. We could introduce 
> > a new value '-1' to mean: 'extended struct sched_param'.
> > 
> > A new getparam syscall could then be introduced - only used by 
> > new user-space.
> 
> The man page of sched_getparam() is pretty explicit in that the 
> sched_priority field of the struct must lie between 
> sched_get_priority_min() and sched_get_priority_max(). If you'd 
> overload sched_priority like this you might end up breaking 
> applications that rely on this, for example RT watchdogs that go 
> through /proc and query the scheduling parameters of all threads.

Only the new sched_gtparam syscall would do this - and that syscall 
would have to be propagated to glibc and user-space with appropriate 
care of course. (probably resulting in a separate function not a 
change/breakage of existing functionality)

Probably not worth the pain though.

	Ingo

RE: [PATCH] scheduler: introduce SCHED_RESET_ON_FORK scheduling policy flag, Second try

[Raz]

Lennart  Hello
I had the same problem several years ago, but in the servers arena.
in order to solve it  I partitioned the solution to two:
1. UNI processors machines
2. SMP processors machines

In UNI processor I created a RT kthread that polls over RTC device.
The observed jitter was 400us.
in SMP processors machine we created a new type of scheduling , called offline.
what i did it to offload a processor and assign it task. jitter is
250us under heavy load. solution is open source and can be found at :
http://sos-linux.svn.sourceforge.net/viewvc/sos-linux/offsched/Documentation/

Raz