Fix various __task_cred related invalid RCU assumptions

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.

There is another instance of __task_cred() in sys_setpriority() itself which is equally unprotected.

Wrap the whole code section into a rcu read side critical section to fix this quick and dirty.

Will be revisited in course of the read_lock(&tasklist_lock) -> rcu crusade.

Signed-off-by Thomas Gleixner <[email protected]>

Cc David Howells <[email protected]>

Cc James Morris <[email protected]>

Cc [email protected]

---
 kernel/sys.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-tip/kernel/sys.c
===================================================================

kernel/sys.c +2 -0

@@ -163,6 +163,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	if (niceval > 19)
 		niceval = 19;
 
+	rcu_read_lock();
 	read_lock(&tasklist_lock);
 	switch (which) {
 		case PRIO_PROCESS:

@@ -200,6 +201,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	}
 out_unlock:
 	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 out:
 	return error;
 }

As a rough rule of thumb, any dereference of an RCU-protected pointer must be covered by rcu_read_lock() or rcu_read_lock_bh() or by the appropriate update-side lock.

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.

There is another instance of __task_cred() in sys_setpriority() itself which is equally unprotected.

Wrap the whole code section into a rcu read side critical section to fix this quick and dirty.

Will be revisited in course of the read_lock(&tasklist_lock) -> rcu crusade.

Signed-off-by Thomas Gleixner <[email protected]>

Cc David Howells <[email protected]>

Cc James Morris <[email protected]>

Cc [email protected]

---
 kernel/sys.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-tip/kernel/sys.c
===================================================================

kernel/sys.c +2 -0

@@ -163,6 +163,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	if (niceval > 19)
 		niceval = 19;
 
+	rcu_read_lock();
 	read_lock(&tasklist_lock);
 	switch (which) {
 		case PRIO_PROCESS:

@@ -200,6 +201,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	}
 out_unlock:
 	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 out:
 	return error;
 }

Quoted 12 lines · Thomas Gleixner

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.

Quoted 8 lines

There is another instance of __task_cred() in sys_setpriority() itself which is equally unprotected.

Wrap the whole code section into a rcu read side critical section to fix this quick and dirty.

Will be revisited in course of the read_lock(&tasklist_lock) -> rcu crusade.

OK, I will bite... Don't the corresponding updates write-hold tasklist_lock? If so, then the fact that the code below is read-holding tasklist_lock would prevent any of the data from changing, which would remove the need to do the rcu_read_lock().

Or are there updates that are carried out without write-holding tasklist_lock that I am missing?

Quoted 28 lines · Paul E. McKenney

Quoted 12 lines · Thomas Gleixner

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.

Quoted 8 lines

There is another instance of __task_cred() in sys_setpriority() itself which is equally unprotected.

Wrap the whole code section into a rcu read side critical section to fix this quick and dirty.

Will be revisited in course of the read_lock(&tasklist_lock) -> rcu crusade.

OK, I will bite... Don't the corresponding updates write-hold tasklist_lock? If so, then the fact that the code below is read-holding tasklist_lock would prevent any of the data from changing, which would remove the need to do the rcu_read_lock().

Quoted 2 lines

Or are there updates that are carried out without write-holding tasklist_lock that I am missing?

Yes, commit_creds() is called lockless.

Quoted 4 lines

Quoted 2 lines

Or are there updates that are carried out without write-holding tasklist_lock that I am missing?

Yes, commit_creds() is called lockless.

Right, and that's what the problem is. commit_creds(), which rcu frees the old creds, does not take tasklist lock write lock.

Quoted 7 lines · Thomas Gleixner

Quoted 4 lines

Quoted 2 lines

Or are there updates that are carried out without write-holding tasklist_lock that I am missing?

Yes, commit_creds() is called lockless.

Right, and that's what the problem is. commit_creds(), which rcu frees the old creds, does not take tasklist lock write lock.

commit_creds() does not need to hold a write lock, because it is implicitly write-locked by only being permitted to run in the thread to which it is committing.

I don't think commit_creds() needs to take the RCU read lock as no-one else can alter/delete the creds it is dealing with.

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.
...

Quoted 16 lines · Thomas Gleixner

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.
...

kernel/sys.c SYSCALL_DEFINE3(setpriority, int, which, · line 167

Quoted 5 lines

kernel/sys.c +1 -0

@@ -163,6 +163,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	if (niceval > 19)
 		niceval = 19;
 
+	rcu_read_lock();
 	read_lock(&tasklist_lock);

Off-topic, but can't resist...

This also fixes another bug here. find_task_by_vpid() is not safe without rcu_read_lock(). I do not mean it is not safe to use the result, just find_pid_ns() by itself is not safe.

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

Quoted 16 lines · Thomas Gleixner

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.
...

kernel/sys.c SYSCALL_DEFINE3(setpriority, int, which, · line 167

Quoted 5 lines

kernel/sys.c +1 -0

@@ -163,6 +163,7 @@ SYSCALL_DEFINE3(setpriority, int, which,
 	if (niceval > 19)
 		niceval = 19;
 
+	rcu_read_lock();
 	read_lock(&tasklist_lock);

Off-topic, but can't resist...

This also fixes another bug here. find_task_by_vpid() is not safe without rcu_read_lock(). I do not mean it is not safe to use the result, just find_pid_ns() by itself is not safe.

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

Quoted 4 lines

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

So, we need to change below comment from "or" to "and" ?

378 /*
379 * Must be called under rcu_read_lock() or with tasklist_lock read-held. 380 */
381 struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)

382 {
383 return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
384 }
385
386 struct task_struct *find_task_by_vpid(pid_t vnr) 387 {
388 return find_task_by_pid_ns(vnr, current->nsproxy->pid_ns);
389 }

Quoted 5 lines · Tetsuo Handa

Quoted 4 lines

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

Quoted 1 line

So, we need to change below comment from "or" to "and" ?

No, both functions must be called with rcu_read_lock()

tasklist_lock read-held is not protecting the rcu lists and does not protect against a concurrent update. It merily protects against tasks going away or being added while we look up the lists.

Quoted 6 lines · Tetsuo Handa

Quoted 4 lines

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

So, we need to change below comment from "or" to "and" ?

No, both functions must be called with rcu_read_lock()

tasklist_lock read-held is not protecting the rcu lists and does not protect against a concurrent update. It merily protects against tasks going away or being added while we look up the lists.

Quoted 4 lines

378 /*
379 * Must be called under rcu_read_lock() or with tasklist_lock read-held. 380 */
381 struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)

Quoted 9 lines

382 {
383 return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
384 }
385
386 struct task_struct *find_task_by_vpid(pid_t vnr) 387 {
388 return find_task_by_pid_ns(vnr, current->nsproxy->pid_ns);
389 }

Thanks,

tglx

[PATCH] Update comment on find_task_by_pid_ns

Caller of find_task_by_vpid() and find_task_by_pid_ns() needs to call rcu_read_lock() rather than read_lock(&tasklist_lock) because find_pid_ns() uses RCU primitives but spinlock does not prevent RCU callback if preemptive RCU ( CONFIG_TREE_PREEMPT_RCU ) is enabled.

Quoted 7 lines · Tetsuo Handa

[PATCH] Update comment on find_task_by_pid_ns

Caller of find_task_by_vpid() and find_task_by_pid_ns() needs to call rcu_read_lock() rather than read_lock(&tasklist_lock) because find_pid_ns() uses RCU primitives but spinlock does not prevent RCU callback if preemptive RCU ( CONFIG_TREE_PREEMPT_RCU ) is enabled.

I agree with the patch, but the changelog looks a bit confusing to me. Perhaps this is just me, though.

tasklist does protect the task and its pid, it can't go away. The problem is that find_pid_ns() itself is unsafe without rcu lock, it can race with copy_process()->free_pid(any_pid).

IOW, if we change copy_process()

	--- kernel/fork.c
	+++ kernel/fork.c
	@@ -1304,8 +1304,11 @@ static struct task_struct *copy_process(
		return p;
	 
	 bad_fork_free_pid:
	-	if (pid != &init_struct_pid)
	+	if (pid != &init_struct_pid) {
	+		read_lock(&tasklist_lock);
			free_pid(pid);
	+		read_unlock(&tasklist_lock);
	+	}
	 bad_fork_cleanup_io:
		if (p->io_context)
			exit_io_context(p);

then find_task_by_pid_ns/etc could be used under tasklist safely even with PREEMPT_RCU.

Quoted 18 lines · Oleg Nesterov

IOW, if we change copy_process()

	--- kernel/fork.c
	+++ kernel/fork.c
	@@ -1304,8 +1304,11 @@ static struct task_struct *copy_process(
		return p;

	 bad_fork_free_pid:
	-	if (pid != &init_struct_pid)
	+	if (pid != &init_struct_pid) {
	+		read_lock(&tasklist_lock);
			free_pid(pid);
	+		read_unlock(&tasklist_lock);
	+	}
	 bad_fork_cleanup_io:
		if (p->io_context)
			exit_io_context(p);

Quoted 2 lines

then find_task_by_pid_ns/etc could be used under tasklist safely even with PREEMPT_RCU.

We try to get rid of the read_lock sites of tasklist_lock, so please let's not think about adding more :)

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Users missing rcu_read_lock() when calling find_task_by_vpid():

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

audit_prepare_user_tty() in kernel/audit.c audit_receive_msg() in kernel/audit.c check_clock() in kernel/posix-cpu-timers.c posix_cpu_timer_create() in kernel/posix-cpu-timers.c SYSCALL_DEFINE3(setpriority) in kernel/sys.c SYSCALL_DEFINE2(getpriority) in kernel/sys.c SYSCALL_DEFINE2(setpgid) in kernel/sys.c SYSCALL_DEFINE1(sched_getscheduler) in kernel/sched.c SYSCALL_DEFINE2(sched_getparam) in kernel/sched.c sched_setaffinity() in kernel/sched.c sched_getaffinity() in kernel/sched.c SYSCALL_DEFINE2(sched_rr_get_interval) in kernel/sched.c tomoyo_is_select_one() in security/tomoyo/common.c tomoyo_read_pid() in security/tomoyo/common.c SYSCALL_DEFINE6(move_pages) in mm/migrate.c SYSCALL_DEFINE4(migrate_pages) in mm/mempolicy.c find_process_by_pid() in arch/mips/kernel/mips-mt-fpaff.c pfm_get_task() in arch/ia64/kernel/perfmon.c cxn_pin_by_pid() in arch/frv/mm/mmu-context.c

Users missing rcu_read_lock() when calling find_task_by_pid_ns():

rest_init() in init/main.c
getthread() in kernel/kgdb.c mconsole_stack() in arch/um/drivers/mconsole_kern.c

What should we do? Adding rcu_read_lock()/rcu_read_unlock() to each callers? Or adding rcu_read_lock()/rcu_read_unlock() inside find_task_by_pid_ns()?

Quoted 4 lines · On Tue, 9 Feb 2010 06:42:45 +0900 Tetsuo Handa…

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Quoted 5 lines

Users missing rcu_read_lock() when calling find_task_by_vpid():

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

Actually, cap_get_target_pid() uses rcu_read_lock() and doesn't take tasklist_lock.

Quoted 22 lines

audit_prepare_user_tty() in kernel/audit.c audit_receive_msg() in kernel/audit.c check_clock() in kernel/posix-cpu-timers.c posix_cpu_timer_create() in kernel/posix-cpu-timers.c SYSCALL_DEFINE3(setpriority) in kernel/sys.c SYSCALL_DEFINE2(getpriority) in kernel/sys.c SYSCALL_DEFINE2(setpgid) in kernel/sys.c SYSCALL_DEFINE1(sched_getscheduler) in kernel/sched.c SYSCALL_DEFINE2(sched_getparam) in kernel/sched.c sched_setaffinity() in kernel/sched.c sched_getaffinity() in kernel/sched.c SYSCALL_DEFINE2(sched_rr_get_interval) in kernel/sched.c tomoyo_is_select_one() in security/tomoyo/common.c tomoyo_read_pid() in security/tomoyo/common.c SYSCALL_DEFINE6(move_pages) in mm/migrate.c SYSCALL_DEFINE4(migrate_pages) in mm/mempolicy.c find_process_by_pid() in arch/mips/kernel/mips-mt-fpaff.c pfm_get_task() in arch/ia64/kernel/perfmon.c cxn_pin_by_pid() in arch/frv/mm/mmu-context.c

Users missing rcu_read_lock() when calling find_task_by_pid_ns():

Quoted 7 lines

rest_init() in init/main.c
getthread() in kernel/kgdb.c mconsole_stack() in arch/um/drivers/mconsole_kern.c

What should we do? Adding rcu_read_lock()/rcu_read_unlock() to each callers? Or adding rcu_read_lock()/rcu_read_unlock() inside find_task_by_pid_ns()?

Putting rcu_read_lock() in the callee isn't a complete solution. Because the function would still be returning a task_struct* without any locking held and without taking a reference against it. So that pointer is useless to the caller!

We could add a new function which looks up the task and then takes a reference on it, insde suitable locks. The caller would then use the task_struct and then remember to call put_task_struct() to unpin it. This prevents the task_struct from getting freed while it's being manipulated, but it doesn't prevent fields within it from being altered
- that's up to the caller to sort out.

One fix is to go through all those callsites and add the rcu_read_lock. That kinda sucks. Perhaps writing the new function which returns a pinned task_struct is better?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Tue, 9 Feb 2010 06:42:45 +0900 Tetsuo Handa…

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Users missing rcu_read_lock() when calling find_task_by_vpid():

Quoted 6 lines

Quoted 3 lines

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

Actually, cap_get_target_pid() uses rcu_read_lock() and doesn't take tasklist_lock.

Hmm - is that in -mm? In my copy here it takes read_lock(&tasklist_lock)

And I'll admit I'm a bit confused as to the current state of things:
do I understand correctly that we now need to take both the tasklist_lock and rcu_read_lock? (Presumably only for read_lock()?)

Quoted 10 lines · On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <[email protected]> wrote:

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Tue, 9 Feb 2010 06:42:45 +0900 Tetsuo Handa…

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Users missing rcu_read_lock() when calling find_task_by_vpid():

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

Actually, cap_get_target_pid() uses rcu_read_lock() and doesn't take tasklist_lock.

Hmm - is that in -mm? In my copy here it takes read_lock(&tasklist_lock)

yup. It got changed in linux-next.

Quoted 3 lines

And I'll admit I'm a bit confused as to the current state of things:
do I understand correctly that we now need to take both the tasklist_lock and rcu_read_lock? (Presumably only for read_lock()?)

Beats me. We need to protect both the pid->task_struct lookup data structures (during the lookup) and protect the resulting task_struct while the caller is playing with it. It's unclear whether rcu_read_lock() suffices for both purposes.

Quoted 27 lines · Andrew Morton

Quoted 10 lines · On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <[email protected]> wrote:

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Tue, 9 Feb 2010 06:42:45 +0900 Tetsuo Handa…

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Users missing rcu_read_lock() when calling find_task_by_vpid():

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

Actually, cap_get_target_pid() uses rcu_read_lock() and doesn't take tasklist_lock.

Hmm - is that in -mm? In my copy here it takes read_lock(&tasklist_lock)

yup. It got changed in linux-next.

Quoted 3 lines

And I'll admit I'm a bit confused as to the current state of things:
do I understand correctly that we now need to take both the tasklist_lock and rcu_read_lock? (Presumably only for read_lock()?)

Quoted 4 lines

Beats me. We need to protect both the pid->task_struct lookup data structures (during the lookup) and protect the resulting task_struct while the caller is playing with it. It's unclear whether rcu_read_lock() suffices for both purposes.

The rcu_read_lock section is sufficient. task_struct can not go away before the rcu_read_unlock()

Quoted 33 lines · Thomas Gleixner

Quoted 31 lines · Andrew Morton

Quoted 10 lines · On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <[email protected]> wrote:

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Tue, 9 Feb 2010 06:42:45 +0900 Tetsuo Handa…

OK. I updated description.

As of 2.6.32 , below users are missing rcu_read_lock().

Users missing rcu_read_lock() when calling find_task_by_vpid():

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
cap_get_target_pid() in kernel/capability.c

Actually, cap_get_target_pid() uses rcu_read_lock() and doesn't take tasklist_lock.

Hmm - is that in -mm? In my copy here it takes read_lock(&tasklist_lock)

yup. It got changed in linux-next.

Quoted 3 lines

And I'll admit I'm a bit confused as to the current state of things:
do I understand correctly that we now need to take both the tasklist_lock and rcu_read_lock? (Presumably only for read_lock()?)

Beats me. We need to protect both the pid->task_struct lookup data structures (during the lookup) and protect the resulting task_struct while the caller is playing with it. It's unclear whether rcu_read_lock() suffices for both purposes.

Quoted 2 lines

The rcu_read_lock section is sufficient. task_struct can not go away before the rcu_read_unlock()

But, more generally, it used to be the case that a spinlock (or rwlock) would imply an rcu read cycle, but now it no longer does, do I understand that right?

Quoted 3 lines

What should we do? Adding rcu_read_lock()/rcu_read_unlock() to each callers? Or adding rcu_read_lock()/rcu_read_unlock() inside find_task_by_pid_ns()?

Putting rcu_read_lock() in the callee isn't a complete solution. Because the function would still be returning a task_struct* without any locking held and without taking a reference against it. So that pointer is useless to the caller!

We could add a new function which looks up the task and then takes a reference on it, insde suitable locks. The caller would then use the task_struct and then remember to call put_task_struct() to unpin it. This prevents the task_struct from getting freed while it's being manipulated, but it doesn't prevent fields within it from being altered
- that's up to the caller to sort out.

One fix is to go through all those callsites and add the rcu_read_lock. That kinda sucks. Perhaps writing the new function which returns a pinned task_struct is better?

This also fixes another bug here. find_task_by_vpid() is not safe without rcu_read_lock(). I do not mean it is not safe to use the result, just find_pid_ns() by itself is not safe.

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

Quoted 8 lines · Oleg Nesterov

This also fixes another bug here. find_task_by_vpid() is not safe without rcu_read_lock(). I do not mean it is not safe to use the result, just find_pid_ns() by itself is not safe.

Usually tasklist gives enough protection, but if copy_process() fails it calls free_pid() lockless and does call_rcu(delayed_put_pid(). This means, without rcu lock find_pid_ns() can't scan the hash table safely.

This bug for sys_setpriority() was fixed, but not fixed for sys_getpriority(). Why not to add it as well?
--------------------
[PATCH] sys: Fix missing rcu protection for sys_setpriority.

find_task_by_vpid() is not safe without rcu_read_lock(). 2.6.33-rc7 got RCU protection for sys_setpriority() but missed it for sys_getpriority().

Signed-off-by Tetsuo Handa <[email protected]>

---
 kernel/sys.c |    2 ++
 1 file changed, 2 insertions(+)

kernel/sys.c +2 -1

@@ -222,6 +222,7 @@ SYSCALL_DEFINE2(getpriority, int, which,
 	if (which > PRIO_USER || which < PRIO_PROCESS)
 		return -EINVAL;
 
+	rcu_read_lock();
 	read_lock(&tasklist_lock);
 	switch (which) {
 		case PRIO_PROCESS:

@@ -267,6 +268,7 @@ SYSCALL_DEFINE2(getpriority, int, which,
 	}
 out_unlock:
 	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 
 	return retval;
 }
--

To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

commit c69e8d9 (CRED: Use RCU to access another task's creds and to release a task's own creds) added non rcu_read_lock() protected access to task creds of the target task in set_prio_one().

The comment above the function says:
* - the caller must hold the RCU read lock

The calling code in sys_setpriority does read_lock(&tasklist_lock) but not rcu_read_lock(). This works only when CONFIG_TREE_PREEMPT_RCU=n. With CONFIG_TREE_PREEMPT_RCU=y the rcu_callbacks can run in the tick interrupt when they see no read side critical section.

There is another instance of __task_cred() in sys_setpriority() itself which is equally unprotected.

Wrap the whole code section into a rcu read side critical section to fix this quick and dirty.

Will be revisited in course of the read_lock(&tasklist_lock) -> rcu crusade.

Signed-off-by Thomas Gleixner <[email protected]>

Cc James Morris <[email protected]>

Cc [email protected]

sys_ioprio_get() accesses __task_cred() without being in a RCU read side critical section. tasklist_lock is not protecting that when CONFIG_TREE_PREEMPT_RCU=y.

Add a rcu_read_lock/unlock() section around the code which accesses __task_cred().

Signed-off-by Thomas Gleixner <[email protected]>

task_sig() accesses __task_cred() without being in a RCU read side critical section. tasklist_lock is not protecting that when CONFIG_TREE_PREEMPT_RCU=y.

Add a rcu_read_lock/unlock() section around the code which accesses __task_cred().

Signed-off-by Thomas Gleixner <[email protected]>

dump_tasks accesses __task_cred() without being in a RCU read side critical section. tasklist_lock is not protecting that when CONFIG_TREE_PREEMPT_RCU=y.

Add a rcu_read_lock/unlock() section around the code which accesses __task_cred().

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

---
 mm/oom_kill.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6-tip/mm/oom_kill.c
===================================================================

mm/oom_kill.c +0 -0

@@ -329,10 +329,13 @@ static void dump_tasks(const struct mem_
 			task_unlock(p);
 			continue;
 		}

mm/oom_kill.c static void dump_tasks(const struct mem_ · line 338

Quoted 7 lines · Thomas Gleixner

mm/oom_kill.c +3 -0

+		/* Protect __task_cred() access */
+		rcu_read_lock();
 		printk(KERN_INFO "[%5d] %5d %5d %8lu %8lu %3d     %3d %s\n",
 		       p->pid, __task_cred(p)->uid, p->tgid, mm->total_vm,
 		       get_mm_rss(mm), (int)task_cpu(p), p->signal->oom_adj,
 		       p->comm);
+		rcu_read_unlock();

No. If there's only one access to __task_cred() like this, use task_cred_xxx() or one of its wrappers instead:

(quoted diff) +1 -1

-		       p->pid, __task_cred(p)->uid, p->tgid, mm->total_vm,
+		       p->pid, task_uid(p), p->tgid, mm->total_vm,

that limits the size of the critical section.

Strictly this is not necessary today, but according to Paul McKenney it's not guaranteed that irq disabled regions will prevent RCU progress. That's a property of the current implementation. In preempt-rt this assumption is not true anymore.

Use get_task_cred() to make this future proof.

Signed-off-by Thomas Gleixner <[email protected]>

Cc James Morris <[email protected]>

Cc [email protected]

---
 security/keys/keyctl.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6-tip/security/keys/keyctl.c
===================================================================

security/keys/keyctl.c +5 -2

@@ -1237,7 +1237,7 @@ long keyctl_get_security(key_serial_t ke
 long keyctl_session_to_parent(void)
 {
 	struct task_struct *me, *parent;
-	const struct cred *mycred, *pcred;
+	const struct cred *mycred, *pcred = NULL;
 	struct cred *cred, *oldcred;
 	key_ref_t keyring_r;
 	int ret;

@@ -1274,7 +1274,7 @@ long keyctl_session_to_parent(void)
 	/* the parent and the child must have different session keyrings or
 	 * there's no point */
 	mycred = current_cred();
-	pcred = __task_cred(parent);
+	pcred = get_task_cred(parent);
 	if (mycred == pcred ||
 	    mycred->tgcred->session_keyring == pcred->tgcred->session_keyring)
 		goto already_same;

@@ -1312,6 +1312,7 @@ long keyctl_session_to_parent(void)
 	set_ti_thread_flag(task_thread_info(parent), TIF_NOTIFY_RESUME);
 
 	write_unlock_irq(&tasklist_lock);
+	put_cred(pcred);
 	if (oldcred)
 		put_cred(oldcred);
 	return 0;

@@ -1321,6 +1322,8 @@ already_same:
 not_permitted:
 	write_unlock_irq(&tasklist_lock);
 	put_cred(cred);
+	if (pcred)
+		put_cred(pcred);
 	return ret;
 
 error_keyring:

Strictly this is not necessary today, but according to Paul McKenney it's not guaranteed that irq disabled regions will prevent RCU progress. That's a property of the current implementation. In preempt-rt this assumption is not true anymore.

Use get_task_cred() to make this future proof.

Signed-off-by Thomas Gleixner <[email protected]>

kill_pid_info_as_uid() accesses __task_cred() without being in a RCU read side critical section. tasklist_lock is not protecting that when CONFIG_TREE_PREEMPT_RCU=y.

Convert the whole tasklist_lock section to rcu and use lock_task_sighand to prevent the exit race.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Oleg Nesterov <[email protected]>

---
 kernel/signal.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/kernel/signal.c
===================================================================

kernel/signal.c +10 -7

@@ -1175,11 +1175,12 @@ int kill_pid_info_as_uid(int sig, struct
 	int ret = -EINVAL;
 	struct task_struct *p;
 	const struct cred *pcred;
+	unsigned long flags;
 
 	if (!valid_signal(sig))
 		return ret;
 
-	read_lock(&tasklist_lock);
+	rcu_read_lock();
 	p = pid_task(pid, PIDTYPE_PID);
 	if (!p) {
 		ret = -ESRCH;

@@ -1196,14 +1197,16 @@ int kill_pid_info_as_uid(int sig, struct
 	ret = security_task_kill(p, info, sig, secid);
 	if (ret)
 		goto out_unlock;
-	if (sig && p->sighand) {
-		unsigned long flags;
-		spin_lock_irqsave(&p->sighand->siglock, flags);
-		ret = __send_signal(sig, info, p, 1, 0);
-		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+
+	if (sig) {
+		if (lock_task_sighand(p, &flags)) {
+			ret = __send_signal(sig, info, p, 1, 0);
+			unlock_task_sighand(p, &flags);
+		} else
+			ret = -ESRCH;
 	}
 out_unlock:
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(kill_pid_info_as_uid);

kill_pid_info_as_uid() accesses __task_cred() without being in a RCU read side critical section. tasklist_lock is not protecting that when CONFIG_TREE_PREEMPT_RCU=y.

Convert the whole tasklist_lock section to rcu and use lock_task_sighand to prevent the exit race.

Signed-off-by Thomas Gleixner <[email protected]>

1) Remove the misleading comment in __sigqueue_alloc() which claims that holding a spinlock is equivalent to rcu_read_lock().

2) Wrap the __send_signal() call in send_signal() into a rcu read side critical section to guarantee that the __sigqueue_alloc() requirement is met in any case.
...
static int send_signal(int sig, struct siginfo *info, struct task_struct *t,

kernel/signal.c +1 -1

 			int group)
 {
-	int from_ancestor_ns = 0;
+	int ret, from_ancestor_ns = 0;
 
 #ifdef CONFIG_PID_NS
 	if (!is_si_special(info) && SI_FROMUSER(info) &&

Quoted 17 lines · Thomas Gleixner

1) Remove the misleading comment in __sigqueue_alloc() which claims that holding a spinlock is equivalent to rcu_read_lock().

2) Wrap the __send_signal() call in send_signal() into a rcu read side critical section to guarantee that the __sigqueue_alloc() requirement is met in any case.
...
static int send_signal(int sig, struct siginfo *info, struct task_struct *t,

kernel/signal.c +1 -1

 			int group)
 {
-	int from_ancestor_ns = 0;
+	int ret, from_ancestor_ns = 0;
 
 #ifdef CONFIG_PID_NS
 	if (!is_si_special(info) && SI_FROMUSER(info) &&

kernel/signal.c static int send_signal(int sig, struct s · line 958

Quoted 7 lines

kernel/signal.c +3 -1

@@ -954,7 +953,11 @@ static int send_signal(int sig, struct s
 		from_ancestor_ns = 1;
 #endif
 
-	return __send_signal(sig, info, t, group, from_ancestor_ns);
+	rcu_read_lock();
+	ret = __send_signal(sig, info, t, group, from_ancestor_ns);
+	rcu_read_unlock();

But, without a comment it is very unobvious why do we need rcu_read_lock().

Perhaps it is better to modify __sigqueue_alloc() instead? It can take rcu_lock() around cred->user itself.

Quoted 2 lines

Perhaps it is better to modify __sigqueue_alloc() instead? It can take rcu_lock() around cred->user itself.

Indeed. Was too tired to see the obvious :)

2) remove the stale signal code snippet

Index: linux-2.6-tip/Documentation/kmemcheck.txt
===================================================================

Documentation/kmemcheck.txt +0 -0

@@ -429,8 +429,7 @@ Let's take a look at it:

 193         /*

 194          * We won't get problems with the target's UID changing under us 195          * because changing it requires RCU be used, and if t != current, the -196          * caller must be holding the RCU readlock (by way of a spinlock) and -197          * we use RCU protection here
+196          * caller must be holding the RCU readlocke
 198          */
 199         user = get_uid(__task_cred(t)->user);
 200         atomic_inc(&user->sigpending);

-197 * we use RCU protection here
+196 * caller must be holding the RCU readlocke

Quoted 2 lines · Thomas Gleixner

-197 * we use RCU protection here
+196 * caller must be holding the RCU readlocke

You mean "readlock" I suspect.

Quoted 2 lines · David Howells

Thomas Gleixner <[email protected]> wrote:

Quoted 4 lines

Quoted 2 lines

-197 * we use RCU protection here
+196 * caller must be holding the RCU readlocke

You mean "readlock" I suspect.

Or maybe he's talking about ye olde readlocke, used widely for OS research throughout the middle ages. You still find that spelling in some really old CS literature.

Quoted 11 lines · Linus Torvalds

Quoted 6 lines · David Howells

Quoted 2 lines · Thomas Gleixner

-197 * we use RCU protection here
+196 * caller must be holding the RCU readlocke

You mean "readlock" I suspect.

Quoted 3 lines

Or maybe he's talking about ye olde readlocke, used widely for OS research throughout the middle ages. You still find that spelling in some really old CS literature.

Interestingly enough, they also tended to split it into two words and capitalize it, as can be seen by searching for "Read Locke" at

http://faculty.uml.edu/enelson/modern07.htm

Index: linux-2.6-tip/Documentation/credentials.txt
===================================================================

Quoted 8 lines · On Thursday 10 December 2009 00:53:26 Thomas Gleixner wrote:

Index: linux-2.6-tip/Documentation/credentials.txt
===================================================================

Documentation/credentials.txt +0 -0

@@ -408,9 +408,6 @@ This should be used inside the RCU read 
...
        }

Documentation/credentials.txt This should be used inside the RCU read · line 413

Quoted 6 lines

Documentation/credentials.txt +0 -3

-A function need not get RCU read lock to use __task_cred() if it is holding a
-spinlock at the time as this implicitly holds the RCU read lock.
-
 Should it be necessary to hold another task's credentials for a long period of
 time, and possibly to sleep whilst doing so, then the caller should get a
 reference on them using:

How about changing the documentation to explain why you can't just use a spinlock or local_irq_disable instead of rcu_read_lock? You explained it well in your [patch 0/9], but that part had not occurred to me yet and having it in the kernel sources might prevent more people from getting it wrong in the future.

1) held spinlocks are not equivalent to rcu_read_lock

2) access to current_cred() is safe as only current can modify its own credentials.

Signed-off-by Thomas Gleixner <[email protected]>

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

That caused me to audit all the __task_cred usage sites except in kernel/exit.c.

Most of the usage sites are correct, but some of them trip over invalid assumptions about the protection which is given by RCU.

- spinlocked/preempt_disabled regions are equivalent to rcu_read_lock():

That's wrong. RCU does not guarantee that.

It has been that way due to implementation details and it still is valid for CONFIG_TREE_PREEMPT_RCU=n, but there is no guarantee that this will be the case forever.

- interrupt disabled regions are equivalent to rcu_read_lock():

Wrong again. RCU does not guarantee that.

It's true for current mainline, but again this is an implementation detail and there is no guarantee by the RCU semantics.

Indeed we want to get rid of that to avoid scalability issues on large systems and preempt-rt got already rid of it to a certain extent.

I'm sure we are lucky that CONFIG_TREE_PREEMPT_RCU=y is not yet wide spread and the code pathes are esoteric enough not to trigger that subtle races (some of them might just error out silently).

Nevertheless we need to fix all invalid assumptions about RCU protection.

The following patch series fixes all yet known affected __task_cred() sites, but there is more auditing of all other rcu users necessary.

Quoted 12 lines · Thomas Gleixner

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

That caused me to audit all the __task_cred usage sites except in kernel/exit.c.

Most of the usage sites are correct, but some of them trip over invalid assumptions about the protection which is given by RCU.

- spinlocked/preempt_disabled regions are equivalent to rcu_read_lock():

Quoted 5 lines

That's wrong. RCU does not guarantee that.

It has been that way due to implementation details and it still is valid for CONFIG_TREE_PREEMPT_RCU=n, but there is no guarantee that this will be the case forever.

To back this up, item #2 from Documentation/RCU/checklist.txt says:

Quoted 19 lines · Paul E. McKenney

Quoted 17 lines · Thomas Gleixner

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

That caused me to audit all the __task_cred usage sites except in kernel/exit.c.

Most of the usage sites are correct, but some of them trip over invalid assumptions about the protection which is given by RCU.

- spinlocked/preempt_disabled regions are equivalent to rcu_read_lock():

That's wrong. RCU does not guarantee that.

It has been that way due to implementation details and it still is valid for CONFIG_TREE_PREEMPT_RCU=n, but there is no guarantee that this will be the case forever.

Quoted 1 line

To back this up, item #2 from Documentation/RCU/checklist.txt says:

Hmm. This seems to be a difference that the tree-RCU things introduced, no? I wonder if we have other areas where we just knew that a spinlock would make an rcu read-lock unnecessary (which used to be true..)

Quoted 22 lines · Linus Torvalds

Quoted 19 lines · Paul E. McKenney

Quoted 17 lines · Thomas Gleixner

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

That caused me to audit all the __task_cred usage sites except in kernel/exit.c.

Most of the usage sites are correct, but some of them trip over invalid assumptions about the protection which is given by RCU.

- spinlocked/preempt_disabled regions are equivalent to rcu_read_lock():

That's wrong. RCU does not guarantee that.

It has been that way due to implementation details and it still is valid for CONFIG_TREE_PREEMPT_RCU=n, but there is no guarantee that this will be the case forever.

Quoted 5 lines

Quoted 1 line

To back this up, item #2 from Documentation/RCU/checklist.txt says:

Hmm. This seems to be a difference that the tree-RCU things introduced, no? I wonder if we have other areas where we just knew that a spinlock would make an rcu read-lock unnecessary (which used to be true..)

That failed being true when we merged PREEMPT_RCU,.. which was a long time ago.

Ah -- I have a related lockdep question. Is there a primitive that says whether or not the current task holds at least one lock of any type? If so, I would like to make rcu_dereference() do at least a little crude checking for this problem.

Quoted 4 lines · Paul E. McKenney

Ah -- I have a related lockdep question. Is there a primitive that says whether or not the current task holds at least one lock of any type? If so, I would like to make rcu_dereference() do at least a little crude checking for this problem.

Hmm, no, but that's not hard to do, however I actually implemented something like that for RCU a long while ago and that gives a metric TON of false positives due to things like the radix tree which are RCU-safe but are not required to be used with RCU.

Quoted 11 lines · Peter Zijlstra

Quoted 4 lines · Paul E. McKenney

Ah -- I have a related lockdep question. Is there a primitive that says whether or not the current task holds at least one lock of any type? If so, I would like to make rcu_dereference() do at least a little crude checking for this problem.

Hmm, no, but that's not hard to do, however I actually implemented something like that for RCU a long while ago and that gives a metric TON of false positives due to things like the radix tree which are RCU-safe but are not required to be used with RCU.

Understood -- my current guess is that there needs to be a way to tag a variant of the rcu_dereference() API with the conditions that must be met, for example, either in an rcu-sched read-side critical section or holding a specific type of lock.

This does make it a little harder to retroactively add checking to existing calls to rcu_dereference(), but should allow a good balance between false positives and false negatives going forward.

Seem reasonable, or am I still missing something?

Quoted 18 lines · Paul E. McKenney

Quoted 11 lines · Peter Zijlstra

Quoted 4 lines · Paul E. McKenney

Ah -- I have a related lockdep question. Is there a primitive that says whether or not the current task holds at least one lock of any type? If so, I would like to make rcu_dereference() do at least a little crude checking for this problem.

Hmm, no, but that's not hard to do, however I actually implemented something like that for RCU a long while ago and that gives a metric TON of false positives due to things like the radix tree which are RCU-safe but are not required to be used with RCU.

Understood -- my current guess is that there needs to be a way to tag a variant of the rcu_dereference() API with the conditions that must be met, for example, either in an rcu-sched read-side critical section or holding a specific type of lock.

Quoted 5 lines

This does make it a little harder to retroactively add checking to existing calls to rcu_dereference(), but should allow a good balance between false positives and false negatives going forward.

Seem reasonable, or am I still missing something?

The only concern is drowning in rcu_dereference() annotations. But I guess that is unavoidable.

I think you can use lock_is_held(&rcu_lock_map), except you need to deal with the !debug_locks case, because lockdep stops once debug_locks becomes false, which means lock_is_held() will return rubbish.

Quoted 3 lines

I think you can use lock_is_held(&rcu_lock_map), except you need to deal with the !debug_locks case, because lockdep stops once debug_locks becomes false, which means lock_is_held() will return rubbish.

OK, so I need to do something like the following, then?

debug_locks ? lock_is_held(&rcu_lock_map) : 1

Quoted 6 lines · Paul E. McKenney

Quoted 3 lines

I think you can use lock_is_held(&rcu_lock_map), except you need to deal with the !debug_locks case, because lockdep stops once debug_locks becomes false, which means lock_is_held() will return rubbish.

OK, so I need to do something like the following, then?

Quoted 1 line

debug_locks ? lock_is_held(&rcu_lock_map) : 1

Depending on what the safe return value is, yeah. If you want an assert like function, false is usually the safe one.

#define lockdep_assert_held(l) WARN_ON(debug_locks && !lockdep_is_held(l))

is the one in-tree user of this.

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

Quoted 3 lines · Thomas Gleixner

While auditing the read_lock(&tasklist_lock) sites for a possible conversion to rcu-read_lock() I stumbled over an unprotected user of __task_cred in kernel/sys.c

I'm sure last time I looked, spinlock primitives implied RCU read locks. Maybe I was mistaken or maybe it's changed. Whatever, good catch, Thomas!