protect migration/%d etc from sched_setaffinity

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

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

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

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

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

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 <[email protected]>      \                 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))

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

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:

Eric

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

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

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.

Same here.

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.

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

Agreed.

-- Jamie

Yes, we are the PC unix after all...

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

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'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

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

You are so damn cunning, Andrew.

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

Robert Love

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