[PATCH] protect migration/%d etc from sched_setaffinity

[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

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

[J.A. Magallon]

On 08.01, Robert Love wrote:
> 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.
> 

Er, why 'she' ?
In spanish we use root==admin as male, and root==tree or plant root,
as female.

Any pointer (like those of jargon dictionaries) ?

TIA ;)

-- 
J.A. Magallon <jamagallon@able.es>      \                 Software is like
sex:
werewolf.able.es                         \           It's better when it's
free
Mandrake Linux release 9.2 (Cooker) for i586
Linux 2.4.22-pre9-jam1m (gcc 3.3.1 (Mandrake Linux 9.2 3.3.1-0.7mdk))

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

[Robert Love]

On Thu, 2003-07-31 at 16:18, J.A. Magallon wrote:

> Er, why 'she' ?
> In spanish we use root==admin as male, and root==tree or plant root,
> as female.

In English, the pronoun refers to the sex of the person. These is no
gender of words like in Spanish (i.e., a dog is a dog).

So, since administrators come in both sexes, I picked one.

	Robert Love

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

[Eric W. Biederman]

Robert Love <rml@tech9.net> writes:

> On Thu, 2003-07-31 at 16:18, J.A. Magallon wrote:
> 
> > Er, why 'she' ?
> > In spanish we use root==admin as male, and root==tree or plant root,
> > as female.
> 
> In English, the pronoun refers to the sex of the person. These is no
> gender of words like in Spanish (i.e., a dog is a dog).
> 
> So, since administrators come in both sexes, I picked one.

Except the convention in English is to use the male in that case.
Using the female pronoun tends to distract from the point.

There are plenty of plural forms that do not imply gender at all.
As in:

> Yah, I know. But this is a lot of code just to prevent root users from hanging
> themselves, and there are plenty of other ways they can do that.

Eric

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

[Jamie Lokier]

Eric W. Biederman wrote:
> > So, since administrators come in both sexes, I picked one.
> 
> Except the convention in English is to use the male in that case.
> Using the female pronoun tends to distract from the point.

That's right.  Many words in English come with a conventional gender,
based on a conventional gender role assignment:

	Doctor     -> male
	Carpenter  -> male
	Baker      -> male
	President  -> male
	Nurse      -> female
	Maid       -> female
	Cleaner    -> female
	Prostitute -> female

etc.  I picked some of the more emotive ones for the above list.

It has been shown that then such words are used in stories, without
any mention of one gender or the other, nearly everyone assumes that
the person so described has the conventional gender for the role.

This is not some flimsy, irrelevant observation.  The assumption is
strong enough that it is very effective at misleading about who is who
in a story.  (Unfortunately I don't have such a story to hand.)  And
it most certainly affects who ends up working in which field.

There are words which do not have such a strong gender role these
days, too.  Newsreader, for example, would have been strictly male at
one time but not any more.  Context matters.  Secretary, for example,
would depend on whether you mean a secretary in a small business, or a
secretary of state.  Administrator is another: System Administrator or
Network Administrator is quite male (oddly, - as all the sysadmins I
know are female), but Office Administration strikes me as assumedly
female.

As you can see from the latter two examples, a word _itself_ does not
imply a linguistic sex.  The role is what is significant.

Many people don't like the stereotyping, and the (significant) social
inequality which is reinforced by this, and choose to do their bit to
alter (possibly correct) the collective view of how things are, or
ought to be.

You're right that it distracts from the point, but then to _not_
distract from the point is to perpetuate social inequality, or
something like that...

> There are plenty of plural forms that do not imply gender at all.
> As in:
> 
> > Yah, I know. But this is a lot of code just to prevent root users from hanging
> > themselves, and there are plenty of other ways they can do that.

That is the most neutral.  Alas, plurals are often that bit more
cumbersome and dry.  Depends whether you're evoking a reference manual
or a personal story, I guess :)

-- Jamie

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

[Eric W. Biederman]

Jamie Lokier <jamie@shareable.org> writes:

> Eric W. Biederman wrote:
> > > So, since administrators come in both sexes, I picked one.
> > 
> > Except the convention in English is to use the male in that case.
> > Using the female pronoun tends to distract from the point.

> As you can see from the latter two examples, a word _itself_ does not
> imply a linguistic sex.  The role is what is significant.

To some extent.  To some extent before people started ``correcting''
the situation the masculine pronoun was on it's way to becoming
gender neutral in English. 

In a similar case sir has become vendor neutral in some contexts.
I need to review what has actually happened in the armed services
to see if a vendor neutral usage has caught on at large.  But it is
encouraging to see that things like this can happen.

> Many people don't like the stereotyping, and the (significant) social
> inequality which is reinforced by this, and choose to do their bit to
> alter (possibly correct) the collective view of how things are, or
> ought to be.

Same here.
 
> You're right that it distracts from the point, but then to _not_
> distract from the point is to perpetuate social inequality, or
> something like that...

It lowers my opinion of someone when they do not want to perpetuate
stereo types and yet cannot rephrase a sentence in a gender neutral
way, in English.

> > There are plenty of plural forms that do not imply gender at all.
> > As in:
> > 
> > > Yah, I know. But this is a lot of code just to prevent root users from
> hanging
> 
> > > themselves, and there are plenty of other ways they can do that.
> 
> That is the most neutral.  Alas, plurals are often that bit more
> cumbersome and dry.  Depends whether you're evoking a reference manual
> or a personal story, I guess :)

So brining this back on topic.   Given that plurals are gender neutral,
and that there are gender neutral words like person and people.  When doing
kernel documentation please do it in a gender neutral way.

Eric

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

[Jamie Lokier]

Eric W. Biederman wrote:
> So bringing this back on topic.  Given that plurals are gender
> neutral, and that there are gender neutral words like person and
> people.  When doing kernel documentation please do it in a gender
> neutral way.

Agreed.

-- Jamie

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

[Sean Estabrooks]

> Eric W. Biederman wrote:
> > So bringing this back on topic.  Given that plurals are gender
> > neutral, and that there are gender neutral words like person and
> > people.  When doing kernel documentation please do it in a gender
> > neutral way.
> 
> Agreed.
> 
Yes, we are the PC unix after all...

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

[Robert Love]

On Fri, 2003-08-01 at 04:09, Eric W. Biederman wrote:

> Except the convention in English is to use the male in that case.
> Using the female pronoun tends to distract from the point.

Ack, sorry. I will forward your grievance to my English teacher. He
(she?) obviously did a poor job. Or I just do not pay attention.

	Robert Love