Re: [PATCH] protect migration/%d etc from sched_setaffinity

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Andi Kleen]

Joe Korty <joe.korty@ccur.com> writes:
>  
> diff -Nura linux-2.6.0-test2/include/linux/sched.h.orig linux-2.6.0-test2/include/linux/sched.h
> --- linux-2.6.0-test2/include/linux/sched.h.orig	2003-07-27 12:57:39.000000000 -0400
> +++ linux-2.6.0-test2/include/linux/sched.h	2003-07-31 15:52:25.000000000 -0400
> @@ -488,6 +488,7 @@
>  #define PF_LESS_THROTTLE 0x01000000	/* Throttle me less: I clena memory */
>  #define PF_SYNCWRITE	0x00200000	/* I am doing a sync write */
>  #define PF_READAHEAD	0x00400000	/* I am doing read-ahead */
> +#define PF_CPULOCK	0x00800000	/* lock users out from changing cpus_allowed */

It would be probably better to just check for ->mm == NULL

This should catch all kernel threads that use daemonize

-Andi

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

On Fri, Aug 01, 2003 at 01:17:34AM +0200, Andi Kleen wrote:
> Joe Korty <joe.korty@ccur.com> writes:
> >  
> > diff -Nura linux-2.6.0-test2/include/linux/sched.h.orig linux-2.6.0-test2/include/linux/sched.h
> > --- linux-2.6.0-test2/include/linux/sched.h.orig	2003-07-27 12:57:39.000000000 -0400
> > +++ linux-2.6.0-test2/include/linux/sched.h	2003-07-31 15:52:25.000000000 -0400
> > @@ -488,6 +488,7 @@
> >  #define PF_LESS_THROTTLE 0x01000000	/* Throttle me less: I clena memory */
> >  #define PF_SYNCWRITE	0x00200000	/* I am doing a sync write */
> >  #define PF_READAHEAD	0x00400000	/* I am doing read-ahead */
> > +#define PF_CPULOCK	0x00800000	/* lock users out from changing cpus_allowed */
> 
> It would be probably better to just check for ->mm == NULL
> 
> This should catch all kernel threads that use daemonize
> 
> -Andi

That is what Robert suggested and it is acceptable to me, though suboptimal..it blocks
every daemon, not just the ones that have to be blocked for system survivability.
Joe

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Mikael Pettersson]

On Thu, 31 Jul 2003 15:47:40 -0700, Andrew Morton <akpm@osdl.org> wrote:
>Joe Korty <joe.korty@ccur.com> wrote:
>>
>> Lock out users from changing the cpu affinity of those per-cpu system
>> daemons which cannot survive such a change, such as migration/%d.
>
>Generally we prefer to not add code which purely protects root from making
>mistakes.  Once the sysadmin has nuked his box he'll learn to not do it
>again.
>
>Or do you have some deeper reaon for needing this?

I have a different reason for wanting something like what Korty's
patch achieves.

My problem is that the perfctr driver needs to enforce CPU affinity
masks on HT P4s due to the fact that they actually are _asymmetric_
MPs (perfctr HW wasn't duplicated as the rest of the state was) which
leads to dynamic scheduling restrictions.

Setting p->cpus_allowed fixes this by replacing the dynamic scheduling
restriction with a static one, which Linux' scheduler handles nicely.
(I'm telling it to use only thread #0 in each processor package.)

The problem is that some _other_ process can change a process'
->cpus_allowed via sys_sched_setaffinity(). Users can shoot themselves,
fine, but in doing so they also mess up the state of the driver and
the perfctrs of other processes.

I could add checks in set_cpus_allowed() and kill the target
process' perfctrs, but another problem is that set_cpus_allowed()
doesn't synchronize with the target process before messing with
its task_struct. So this doesn't work.

Right now my only safe solution is to do additional checks in
switch_to()'s resume path, which slows down context-switches.

/Mikael

[PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

Lock out users from changing the cpu affinity of those per-cpu system
daemons which cannot survive such a change, such as migration/%d.

Passes basic handtest of sched_setaffinity(2) on various locked and
unlocked processes on a i386, otherwise untested except by eyeball.

Except for one line in i386, no arch needed any changes to support
this patch.

Joe


 arch/i386/kernel/apm.c |    1 +
 include/linux/sched.h  |    1 +
 kernel/fork.c          |    2 +-
 kernel/module.c        |    2 ++
 kernel/sched.c         |    4 ++++
 kernel/softirq.c       |    4 ++--
 kernel/workqueue.c     |    1 +
 mm/vmscan.c            |    5 ++++-
 8 files changed, 16 insertions(+), 4 deletions(-)


diff -Nura linux-2.6.0-test2/arch/i386/kernel/apm.c.orig linux-2.6.0-test2/arch/i386/kernel/apm.c
--- linux-2.6.0-test2/arch/i386/kernel/apm.c.orig	2003-07-31 10:36:23.000000000 -0400
+++ linux-2.6.0-test2/arch/i386/kernel/apm.c	2003-07-31 15:52:25.000000000 -0400
@@ -1705,6 +1705,7 @@
 	 * Method suggested by Ingo Molnar.
 	 */
 	set_cpus_allowed(current, 1UL << 0);
+	current->flags |= PF_CPULOCK;
 	BUG_ON(smp_processor_id() != 0);
 #endif
 
diff -Nura linux-2.6.0-test2/include/linux/sched.h.orig linux-2.6.0-test2/include/linux/sched.h
--- linux-2.6.0-test2/include/linux/sched.h.orig	2003-07-27 12:57:39.000000000 -0400
+++ linux-2.6.0-test2/include/linux/sched.h	2003-07-31 15:52:25.000000000 -0400
@@ -488,6 +488,7 @@
 #define PF_LESS_THROTTLE 0x01000000	/* Throttle me less: I clena memory */
 #define PF_SYNCWRITE	0x00200000	/* I am doing a sync write */
 #define PF_READAHEAD	0x00400000	/* I am doing read-ahead */
+#define PF_CPULOCK	0x00800000	/* lock users out from changing cpus_allowed */
 
 #ifdef CONFIG_SMP
 extern int set_cpus_allowed(task_t *p, unsigned long new_mask);
diff -Nura linux-2.6.0-test2/kernel/sched.c.orig linux-2.6.0-test2/kernel/sched.c
--- linux-2.6.0-test2/kernel/sched.c.orig	2003-07-27 13:07:14.000000000 -0400
+++ linux-2.6.0-test2/kernel/sched.c	2003-07-31 18:16:20.000000000 -0400
@@ -1930,6 +1930,9 @@
 	if ((current->euid != p->euid) && (current->euid != p->uid) &&
 			!capable(CAP_SYS_NICE))
 		goto out_unlock;
+	if (p->flags & PF_CPULOCK) {
+		goto out_unlock;
+	}
 
 	retval = set_cpus_allowed(p, new_mask);
 
@@ -2380,6 +2383,7 @@
 	 * serially).
 	 */
 	set_cpus_allowed(current, 1UL << cpu);
+	current->flags |= PF_CPULOCK;
 
 	ret = setscheduler(0, SCHED_FIFO, &param);
 
diff -Nura linux-2.6.0-test2/kernel/softirq.c.orig linux-2.6.0-test2/kernel/softirq.c
--- linux-2.6.0-test2/kernel/softirq.c.orig	2003-07-27 12:58:53.000000000 -0400
+++ linux-2.6.0-test2/kernel/softirq.c	2003-07-31 15:52:25.000000000 -0400
@@ -323,8 +323,8 @@
 
 	/* Migrate to the right CPU */
 	set_cpus_allowed(current, 1UL << cpu);
-	if (smp_processor_id() != cpu)
-		BUG();
+	current->flags |= PF_CPULOCK;
+	BUG_ON(smp_processor_id() != cpu);
 
 	__set_current_state(TASK_INTERRUPTIBLE);
 	mb();
diff -Nura linux-2.6.0-test2/kernel/module.c.orig linux-2.6.0-test2/kernel/module.c
--- linux-2.6.0-test2/kernel/module.c.orig	2003-07-27 13:06:11.000000000 -0400
+++ linux-2.6.0-test2/kernel/module.c	2003-07-31 15:52:25.000000000 -0400
@@ -483,6 +483,7 @@
 	setscheduler(current->pid, SCHED_FIFO, &param);
 #endif
 	set_cpus_allowed(current, 1UL << (unsigned long)cpu);
+	current->flags |= PF_CPULOCK;
 
 	/* Ack: we are alive */
 	atomic_inc(&stopref_thread_ack);
@@ -544,6 +545,7 @@
 	/* FIXME: racy with set_cpus_allowed. */
 	old_allowed = current->cpus_allowed;
 	set_cpus_allowed(current, 1UL << (unsigned long)cpu);
+	current->flags |= PF_CPULOCK;
 
 	atomic_set(&stopref_thread_ack, 0);
 	stopref_num_threads = 0;
diff -Nura linux-2.6.0-test2/kernel/fork.c.orig linux-2.6.0-test2/kernel/fork.c
--- linux-2.6.0-test2/kernel/fork.c.orig	2003-07-27 12:57:39.000000000 -0400
+++ linux-2.6.0-test2/kernel/fork.c	2003-07-31 15:52:25.000000000 -0400
@@ -705,7 +705,7 @@
 {
 	unsigned long new_flags = p->flags;
 
-	new_flags &= ~PF_SUPERPRIV;
+	new_flags &= ~(PF_SUPERPRIV | PF_CPULOCK);
 	new_flags |= PF_FORKNOEXEC;
 	if (!(clone_flags & CLONE_PTRACE))
 		p->ptrace = 0;
diff -Nura linux-2.6.0-test2/kernel/workqueue.c.orig linux-2.6.0-test2/kernel/workqueue.c
--- linux-2.6.0-test2/kernel/workqueue.c.orig	2003-07-27 13:11:08.000000000 -0400
+++ linux-2.6.0-test2/kernel/workqueue.c	2003-07-31 15:52:25.000000000 -0400
@@ -177,6 +177,7 @@
 
 	set_user_nice(current, -10);
 	set_cpus_allowed(current, 1UL << cpu);
+	current->flags |= PF_CPULOCK;
 
 	complete(&startup->done);
 
diff -Nura linux-2.6.0-test2/mm/vmscan.c.orig linux-2.6.0-test2/mm/vmscan.c
--- linux-2.6.0-test2/mm/vmscan.c.orig	2003-07-27 12:57:44.000000000 -0400
+++ linux-2.6.0-test2/mm/vmscan.c	2003-07-31 15:52:25.000000000 -0400
@@ -960,8 +960,11 @@
 
 	daemonize("kswapd%d", pgdat->node_id);
 	cpumask = node_to_cpumask(pgdat->node_id);
-	if (cpumask)
+	if (cpumask) {
 		set_cpus_allowed(tsk, cpumask);
+		/* FIXME: a node version of PF_CPULOCK would be cool */
+		current->flags |= PF_CPULOCK;
+	}
 	current->reclaim_state = &reclaim_state;
 
 	/*

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> Lock out users from changing the cpu affinity of those per-cpu system
> daemons which cannot survive such a change, such as migration/%d.

Generally we prefer to not add code which purely protects root from making
mistakes.  Once the sysadmin has nuked his box he'll learn to not do it
again.

Or do you have some deeper reaon for needing this?

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

> Joe Korty <joe.korty@ccur.com> wrote:
> >
> > Lock out users from changing the cpu affinity of those per-cpu system
> > daemons which cannot survive such a change, such as migration/%d.
> 
> Generally we prefer to not add code which purely protects root from making
> mistakes.  Once the sysadmin has nuked his box he'll learn to not do it
> again.

I'd like to be able to write shell scrips that operate on the set of
/proc/[0-9]* without having to know which of the ever-changing list
of processes need to be avoided and which not.

And it's not really root.  It's a SYS_CAP_NICE capability.  All
realtime apps or their admins will have that capability as a matter
of course in their normal daily operations.

Joe

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Andrew Morton]

Joe Korty <joe.korty@ccur.com> wrote:
>
> I'd like to be able to write shell scrips that operate on the set of
> /proc/[0-9]* without having to know which of the ever-changing list
> of processes need to be avoided and which not.

Like this?

#!/bin/sh

#
# can_set_affinity pid
#
can_set_affinity()
{
	if [ "$(cat /proc/$1/maps)" != "" ]
	then
		return 0
	fi
	if head -1 /proc/$1/status | egrep "events|migration"
	then
		return 1
	else
		return 0
	fi
}

if can_set_affinity $1
then
	echo can set affinity of pid $1
else
	echo cannot set affinity of pid $1
fi

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Robert Love]

On Thu, 2003-07-31 at 16:17, Andrew Morton wrote:

> Like this?

You are so damn cunning, Andrew.

Even better, since Joe complained of the "ever changing" list, why not
check for "\[*/[0-9*]\]" ?

	Robert Love

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

On Thu, Jul 31, 2003 at 04:17:03PM -0700, Andrew Morton wrote:
> Joe Korty <joe.korty@ccur.com> wrote:
> >
> > I'd like to be able to write shell scrips that operate on the set of
> > /proc/[0-9]* without having to know which of the ever-changing list
> > of processes need to be avoided and which not.
> 
> Like this?
> 
> #!/bin/sh
> 
> #
> # can_set_affinity pid
> #
> can_set_affinity()
> {
> 	if [ "$(cat /proc/$1/maps)" != "" ]
> 	then
> 		return 0
> 	fi
> 	if head -1 /proc/$1/status | egrep "events|migration"
> 	then
> 		return 1
> 	else
> 		return 0
> 	fi
> }
> 
> if can_set_affinity $1
> then
> 	echo can set affinity of pid $1
> else
> 	echo cannot set affinity of pid $1
> fi


Good stuff.  I reduced it to (having trouble easily reading the original
output):

    name=$(head -1 /proc/$1/status | awk '{print $2}')
    echo -n '[' $1 '] ' $name ' '
    if [ "$(cat /proc/$1/maps)" == "" ]
    then
	    echo SAFE
    else
	    echo changeable
    fi

It catches all those that need catching, plus denies changes to some
daemons that could survive sched_setaffinity: khubd, kirqd, pdflush*,
kswapd0, scsi_eh_[01], ahc_dv_[01], kseriod, and kjournald*.

Joe

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Robert Love]

On Thu, 2003-07-31 at 15:46, Joe Korty wrote:

> Lock out users from changing the cpu affinity of those per-cpu system
> daemons which cannot survive such a change, such as migration/%d.
> 
> Passes basic handtest of sched_setaffinity(2) on various locked and
> unlocked processes on a i386, otherwise untested except by eyeball.
> 
> Except for one line in i386, no arch needed any changes to support
> this patch.

I have been wondering what to do about processor affinity and kernel
threads. I just concluded "only root can change it, and we can let root
stab herself if she really wants to."

But if this is really an issue, why not just do:

	ret = -EINVAL;
	if (!p->mm)
		goto out_unlock;

in sys_sched_setaffinity ?

	Robert Love

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

> On Thu, 2003-07-31 at 15:46, Joe Korty wrote:
> 
> > Lock out users from changing the cpu affinity of those per-cpu system
> > daemons which cannot survive such a change, such as migration/%d.
> > 
> > Passes basic handtest of sched_setaffinity(2) on various locked and
> > unlocked processes on a i386, otherwise untested except by eyeball.
> > 
> > Except for one line in i386, no arch needed any changes to support
> > this patch.
> 
> I have been wondering what to do about processor affinity and kernel
> threads. I just concluded "only root can change it, and we can let root
> stab herself if she really wants to."
> 
> But if this is really an issue, why not just do:
> 
> 	ret = -EINVAL;
> 	if (!p->mm)
> 		goto out_unlock;
> 
> in sys_sched_setaffinity ?
> 
> 	Robert Love


It's not all system daemons, just some of them that need protection.

Keeping the set of locked-down daemons to the smallest possible is
important when one wants to 'set aside' cpus for use only by
specific, need-the-lowest-latency-possible realtime applications.

Joe

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Robert Love]

On Thu, 2003-07-31 at 16:06, Joe Korty wrote:

> It's not all system daemons, just some of them that need protection.
> 
> Keeping the set of locked-down daemons to the smallest possible is
> important when one wants to 'set aside' cpus for use only by
> specific, need-the-lowest-latency-possible realtime applications.

Yah, I know. But this is a lot of code just to prevent root from hanging
herself, and she has plenty of other ways with which to do that.

	Robert Love

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

> Yah, I know. But this is a lot of code just to prevent root from hanging
> herself, and she has plenty of other ways with which to do that.

Actually it is only 20 lines of changes .. 16 lines added, 4 deleted.
Joe

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Robert Love]

On Thu, 2003-07-31 at 16:16, Joe Korty wrote:

> Actually it is only 20 lines of changes .. 16 lines added, 4 deleted.

I know. But 16 new lines, including a new process flag, seems overkill.
That is all I am saying. Just my opinion.

There are a _lot_ of things root can do wrong.

	Robert Love

Re: [PATCH] protect migration/%d etc from sched_setaffinity

[Joe Korty]

> On Thu, 2003-07-31 at 16:16, Joe Korty wrote:
> 
> > Actually it is only 20 lines of changes .. 16 lines added, 4 deleted.
> 
> I know. But 16 new lines, including a new process flag, seems overkill.
> That is all I am saying. Just my opinion.
> 
> There are a _lot_ of things root can do wrong.
> 
> 	Robert Love


Hi Robert,
I don't consider SYS_CAP_NICE to be a typical 'root' thing at all.
Everything realtime needs it as part of normal operations.  I think
of it as an intermediate thing between 'ordinary desktop users' and
'root', and as such it should behave nicely, just like the
plain-vanilla services available to ordinary users.

I believe it is suboptimal to lump everything a normal desktop user
wouldn't normally do as 'root -- let the user beware'.

Joe