[patch 0/9] Fix various __task_cred related invalid RCU assumptions

[patch 0/9] Fix various __task_cred related invalid RCU assumptions

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

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

Thanks,

	tglx

[patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Thomas Gleixner]

[-- Attachment #1: sys-add-missing-rcu-protection.patch --]
[-- Type: text/plain, Size: 1563 bytes --]

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 <tglx@linutronix.de>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: linux-security-module@vger.kernel.org
---
 kernel/sys.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-tip/kernel/sys.c
===================================================================
--- linux-2.6-tip.orig/kernel/sys.c
+++ linux-2.6-tip/kernel/sys.c
@@ -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;
 }

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Paul E. McKenney]

On Thu, Dec 10, 2009 at 12:52:51AM -0000, Thomas Gleixner wrote:
> 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.

And this is called out in Documentation/RCU/checklist.txt, item #2:

2.	Do the RCU read-side critical sections make proper use of
	rcu_read_lock() and friends?  These primitives are needed
	to prevent grace periods from ending prematurely, which
	could result in data being unceremoniously freed out from
	under your read-side code, which can greatly increase the
	actuarial risk of your kernel.

	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.

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

Good catch!

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: linux-security-module@vger.kernel.org
> ---
>  kernel/sys.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6-tip/kernel/sys.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/sys.c
> +++ linux-2.6-tip/kernel/sys.c
> @@ -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;
>  }
> 
> 

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Tetsuo Handa]

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

Does this mean that we need both rcu_read_lock() *and*
read_lock(&tasklist_lock) when calling find_task_by_vpid() because pid_task()
uses rcu_dereference() and find_pid_ns() uses hlist_for_each_entry_rcu ?

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 }

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Paul E. McKenney]

On Thu, Dec 10, 2009 at 12:52:51AM -0000, Thomas Gleixner wrote:
> 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.

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?

							Thanx, Paul

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: linux-security-module@vger.kernel.org
> ---
>  kernel/sys.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6-tip/kernel/sys.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/sys.c
> +++ linux-2.6-tip/kernel/sys.c
> @@ -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;
>  }
> 
> 

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Oleg Nesterov]

On 12/09, Paul E. McKenney wrote:
>
> On Thu, Dec 10, 2009 at 12:52:51AM -0000, Thomas Gleixner wrote:
> > 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.
>
> 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?

Yes, commit_creds() is called lockless.

Or I misunderstood the question/patch...

Oleg.

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Thomas Gleixner]

On Thu, 10 Dec 2009, Oleg Nesterov wrote:
> On 12/09, Paul E. McKenney wrote:
> >
> > On Thu, Dec 10, 2009 at 12:52:51AM -0000, Thomas Gleixner wrote:
> > > 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.
> >
> > 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?
> 
> 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.

Thanks,

	tglx

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

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

David

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Thomas Gleixner]

On Fri, 11 Dec 2009, David Howells wrote:

> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > > > 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_cred() is not required to take anything, but the reader side
needs to take rcu_read_lock() when accessing __task_cred() of another
task as that task could update its own creds right at that point.

The point is that read_lock(tasklist_lock) is not sufficient for the
reader side.

Thanks,

	tglx

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Oleg Nesterov]

On 12/10, Thomas Gleixner wrote:
>
> 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.
> ...
> --- linux-2.6-tip.orig/kernel/sys.c
> +++ linux-2.6-tip/kernel/sys.c
> @@ -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.

Oleg.

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[Thomas Gleixner]

On Thu, 10 Dec 2009, Oleg Nesterov wrote:
> On 12/10, Thomas Gleixner wrote:
> >
> > 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.
> > ...
> > --- linux-2.6-tip.orig/kernel/sys.c
> > +++ linux-2.6-tip/kernel/sys.c
> > @@ -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.

I guess we have whole bunch of auditing to do all over the place. And
we definitely need some debugging aid (preferrably a lockdep
extension) which allows us to detect such nasty details.

Thanks,

	tglx

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred()access

[Tetsuo Handa]

Oleg Nesterov wrote:
> On 12/10, Thomas Gleixner wrote:
> >
> > 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.
> > ...
> > --- linux-2.6-tip.orig/kernel/sys.c
> > +++ linux-2.6-tip/kernel/sys.c
> > @@ -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.

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 }

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred()access

[Thomas Gleixner]

On Fri, 11 Dec 2009, Tetsuo Handa wrote:
> > 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.
 
> 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 }
> 

Thanks,

	tglx

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred()access

[Tetsuo Handa]

Thomas Gleixner wrote:
> On Fri, 11 Dec 2009, Tetsuo Handa wrote:
> > > 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.

So, rcu_read_lock() is mandatory.
But I couldn't understand why tasklist_lock being held is not mandatory.

Caller of these functions will use "struct task_struct" and expect that values
and pointers retrieved by dereferencing returned pointer are valid.

If "struct task_struct" was removed from the list due to missing tasklist_lock
between returning from find_task_by_pid_ns() and dereferencing, I worry that
values and pointers retrieved by dereferencing are invalid.

rcu_read_lock() being held will prevent the returned "struct task_struct" from
being kfree()d, but I think rcu_read_lock() being held does not prevent values
and pointers of "struct task_struct" from being invalidated.



Anyway, I browsed 2.6.32 for find_task_by_vpid() / find_task_by_pid_ns() users
and found below users which lacks read_lock(&tasklist_lock) or rcu_read_lock().

Users missing read_lock(&tasklist_lock) when calling find_task_by_vpid():

  get_net_ns_by_pid() in net/core/net_namespace.c
  seq_print_userip_objs() in kernel/trace/trace_output.c
  fill_pid() in kernel/taskstats.c
  fill_tgid() in kernel/taskstats.c
  futex_find_get_task() in kernel/futex.c
  SYSCALL_DEFINE3(get_robust_list) in kernel/futex.c
  compat_sys_get_robust_list() in kernel/futex_compat.c
  ptrace_get_task_struct() in kernel/ptrace.c
  do_send_specific() in kernel/signal.c
  find_get_context() in kernel/perf_event.c
  posix_cpu_clock_get() in kernel/posix-cpu-timers.c
  good_sigevent() in kernel/posix-timers.c
  attach_task_by_pid() in kernel/cgroup.c
  SYSCALL_DEFINE1(getpgid) in kernel/sys.c
  SYSCALL_DEFINE1(getsid) in kernel/sys.c
  do_sched_setscheduler() in kernel/sched.c

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 read_lock(&tasklist_lock) when calling find_task_by_pid_ns():

  rest_init() in init/main.c
  proc_pid_lookup() in fs/proc/base.c
  proc_task_lookup() in fs/proc/base.c
  first_tid() in fs/proc/base.c
  getthread() in kernel/kgdb.c
  mconsole_stack() in arch/um/drivers/mconsole_kern.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

Regarding users which lack rcu_read_lock(), users call
read_lock(&tasklist_lock) in order to access "struct task_struct" returned
by find_task_by_pid_ns() but they do not want to be bothered by internal pid
structure. Thus, the fix would be to add rcu_read_lock() and rcu_read_unlock()
inside find_task_by_pid_ns().

But how to check users which lack read_lock(&tasklist_lock) but expecting that
it is safe to access "struct task_struct" returned by find_task_by_pid_ns() ?

[PATCH] Update comment on find_task_by_pid_ns

[Tetsuo Handa]

Thomas Gleixner wrote:
> On Fri, 11 Dec 2009, Tetsuo Handa wrote:
> > > 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.
>  
> > 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 }
> > 
> 
> Thanks,
> 
> 	tglx

So, we need to update the comment on these functions?
----------------------------------------
[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.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/pid.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next.orig/kernel/pid.c
+++ linux-next/kernel/pid.c
@@ -376,7 +376,7 @@ struct task_struct *pid_task(struct pid 
 EXPORT_SYMBOL(pid_task);
 
 /*
- * Must be called under rcu_read_lock() or with tasklist_lock read-held.
+ * Must be called under rcu_read_lock().
  */
 struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
 {

Re: [PATCH] Update comment on find_task_by_pid_ns

[Oleg Nesterov]

On 02/08, Tetsuo Handa wrote:
>
> [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.

In any case, I think the patch is nice.

Oleg.

Re: [PATCH] Update comment on find_task_by_pid_ns

[Thomas Gleixner]

On Mon, 8 Feb 2010, Oleg Nesterov wrote:

> On 02/08, Tetsuo Handa wrote:
> >
> > [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.

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

	tglx

Re: [PATCH] Update comment on find_task_by_pid_ns

[Oleg Nesterov]

On 02/08, Thomas Gleixner wrote:
>
> On Mon, 8 Feb 2010, Oleg Nesterov wrote:
>
> > 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.
>
> We try to get rid of the read_lock sites of tasklist_lock, so please
> let's not think about adding more :)

Yes, yes, I agree. I didn't mean this patch makes sense.

Oleg.

Re: [PATCH] Update comment on find_task_by_pid_ns

[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():

  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()?
--------------------
[PATCH] Update comment on find_task_by_pid_ns

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

Protecting copy_process()->free_pid(any_pid) with tasklist_lock would make it
possible to call find_task_by_pid_ns() under tasklist safely, but we don't do
so because we are trying to get rid of the read_lock sites of tasklist_lock.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/pid.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next.orig/kernel/pid.c
+++ linux-next/kernel/pid.c
@@ -376,7 +376,7 @@ struct task_struct *pid_task(struct pid 
 EXPORT_SYMBOL(pid_task);
 
 /*
- * Must be called under rcu_read_lock() or with tasklist_lock read-held.
+ * Must be called under rcu_read_lock().
  */
 struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
 {

Re: [PATCH] Update comment on find_task_by_pid_ns

[Andrew Morton]

On Tue, 9 Feb 2010 06:42:45 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> 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

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

>   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()?

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?

Re: [PATCH] Update comment on find_task_by_pid_ns

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@linux-foundation.org):
> On Tue, 9 Feb 2010 06:42:45 +0900
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> > 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
> 
> 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()?)

> >   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()?
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Update comment on find_task_by_pid_ns

[Andrew Morton]

On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <serue@us.ibm.com> wrote:

> Quoting Andrew Morton (akpm@linux-foundation.org):
> > On Tue, 9 Feb 2010 06:42:45 +0900
> > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > 
> > > 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
> > 
> > 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.

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

Re: [PATCH] Update comment on find_task_by_pid_ns

[Thomas Gleixner]

On Wed, 10 Feb 2010, Andrew Morton wrote:
> On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > Quoting Andrew Morton (akpm@linux-foundation.org):
> > > On Tue, 9 Feb 2010 06:42:45 +0900
> > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > 
> > > > 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
> > > 
> > > 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.
> 
> > 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.

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

Thanks,

	tglx

Re: [PATCH] Update comment on find_task_by_pid_ns

[Serge E. Hallyn]

Quoting Thomas Gleixner (tglx@linutronix.de):
> On Wed, 10 Feb 2010, Andrew Morton wrote:
> > On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> > 
> > > Quoting Andrew Morton (akpm@linux-foundation.org):
> > > > On Tue, 9 Feb 2010 06:42:45 +0900
> > > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > > 
> > > > > 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
> > > > 
> > > > 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.
> > 
> > > 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.
> 
> 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?

thanks,
-serge

Re: [PATCH] Update comment on find_task_by_pid_ns

[Paul E. McKenney]

On Wed, Feb 10, 2010 at 02:18:34PM -0600, Serge E. Hallyn wrote:
> Quoting Thomas Gleixner (tglx@linutronix.de):
> > On Wed, 10 Feb 2010, Andrew Morton wrote:
> > > On Wed, 10 Feb 2010 10:30:33 -0600 "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> > > 
> > > > Quoting Andrew Morton (akpm@linux-foundation.org):
> > > > > On Tue, 9 Feb 2010 06:42:45 +0900
> > > > > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > > > 
> > > > > > 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
> > > > > 
> > > > > 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.
> > > 
> > > > 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.
> > 
> > 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?

You are correct, with CONFIG_PREEMPT_RCU, acquiring a lock does -not-
imply an RCU read-side critical section.  And acquiring a lock does
-not- imply any sort of RCU read-side critical section in -rt kernels
in any case.

							Thanx, Paul

Re: [PATCH] Update comment on find_task_by_pid_ns

[Tetsuo Handa]

Andrew Morton wrote:
> > 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.

Code for "struct task_struct" is too complicated for me to understand,
but my understanding is that

(1) tasklist_lock is acquired for writing.

(2) "struct task_struct" (to exit()) is removed from task's list.

(3) tasklist_lock is released.

(4) Wait for RCU grace period.

(5) kfree() members of "struct task_struct".

(6) kfree() "struct task_struct" itself.

If above sequence is correct, I think

	rcu_read_lock();
	task = find_task_by_pid_ns();
	if (task)
		do_something(task);
	rcu_read_unlock();

do_something() can safely access all members of task without
read_lock(&tasklist_lock), except task->prev (I don't know the exact member)
and task->usage, because do_something() finishes its work before (5).
I think we need to call find_task_by_pid_ns() with both
read_lock(&tasklist_lock) and rcu_read_lock()

	read_lock(&tasklist_lock);
	rcu_read_lock();
	task = find_task_by_pid_ns();
	if (task)
		atomido_something(task);
	rcu_read_unlock();
	read_unlock(&tasklist_lock);

only when do_something() wants to access task->prev or task->usage .

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

[PATCH] sys: Fix missing rcu protection for sys_getpriority.

[Tetsuo Handa]

Oleg Nesterov wrote:
> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/sys.c |    2 ++
 1 file changed, 2 insertions(+)

--- linux-2.6.33-rc7.orig/kernel/sys.c
+++ linux-2.6.33-rc7/kernel/sys.c
@@ -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;
 }

Re: [PATCH] sys: Fix missing rcu protection for sys_getpriority.

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@I-love.SAKURA.ne.jp):
> Oleg Nesterov wrote:
> > 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 <penguin-kernel@I-love.SAKURA.ne.jp>

Would be needed indeed but I don't have a copy of linux-next handy -
if this isn't changed there yet then

Acked-by: Serge Hallyn <serue@us.ibm.com>

thanks,
-serge

> ---
>  kernel/sys.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- linux-2.6.33-rc7.orig/kernel/sys.c
> +++ linux-2.6.33-rc7/kernel/sys.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tip:core/urgent] sys: Fix missing rcu protection for __task_cred() access

[tip-bot for Thomas Gleixner]

Commit-ID:  d4581a239a40319205762b76c01eb6363f277efa
Gitweb:     http://git.kernel.org/tip/d4581a239a40319205762b76c01eb6363f277efa
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 10 Dec 2009 00:52:51 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Dec 2009 23:04:11 +0100

sys: Fix missing rcu protection for __task_cred() access

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.

Oleg noted further:

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.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091210004703.029784964@linutronix.de>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

---
 kernel/sys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 9968c5f..bc1dc61 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -163,6 +163,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
 	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, int, who, int, niceval)
 	}
 out_unlock:
 	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 out:
 	return error;
 }

[patch 2/9] fs: Add missing rcu protection for __task_cred() in sys_ioprio_get

[Thomas Gleixner]

[-- Attachment #1: fs-fix-missing-rcu-protection-of-__task_cred.patch --]
[-- Type: text/plain, Size: 1112 bytes --]

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 <tglx@linutronix.de>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: linux-security-module@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ioprio.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-tip/fs/ioprio.c
===================================================================
--- linux-2.6-tip.orig/fs/ioprio.c
+++ linux-2.6-tip/fs/ioprio.c
@@ -230,6 +230,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, 
 			if (!user)
 				break;
 
+			rcu_read_lock();
 			do_each_thread(g, p) {
 				if (__task_cred(p)->uid != user->uid)
 					continue;
@@ -241,6 +242,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, 
 				else
 					ret = ioprio_best(ret, tmpio);
 			} while_each_thread(g, p);
+			rcu_read_unlock();
 
 			if (who)
 				free_uid(user);

[patch 3/9] proc: Add missing rcu protection for __task_cred() in task_sig()

[Thomas Gleixner]

[-- Attachment #1: proc-fix-missing-rcu-protection-of-__task_cred.patch --]
[-- Type: text/plain, Size: 1074 bytes --]

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 <tglx@linutronix.de>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: linux-security-module@vger.kernel.org
Cc: Al Viro <viro@zeniv.linux.org.uk>

---
 fs/proc/array.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-tip/fs/proc/array.c
===================================================================
--- linux-2.6-tip.orig/fs/proc/array.c
+++ linux-2.6-tip/fs/proc/array.c
@@ -265,7 +265,9 @@ static inline void task_sig(struct seq_f
 		blocked = p->blocked;
 		collect_sigign_sigcatch(p, &ignored, &caught);
 		num_threads = atomic_read(&p->signal->count);
+		rcu_read_lock();
 		qsize = atomic_read(&__task_cred(p)->user->sigpending);
+		rcu_read_unlock();
 		qlim = p->signal->rlim[RLIMIT_SIGPENDING].rlim_cur;
 		unlock_task_sighand(p, &flags);
 	}

[patch 4/9] oom: Add missing rcu protection of __task_cred() in dump_tasks

[Thomas Gleixner]

[-- Attachment #1: oom-fix-missing-rcu-protection-of-__task_cred.patch --]
[-- Type: text/plain, Size: 995 bytes --]

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 <tglx@linutronix.de>
Cc: linux-mm@kvack.org
---
 mm/oom_kill.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6-tip/mm/oom_kill.c
===================================================================
--- linux-2.6-tip.orig/mm/oom_kill.c
+++ linux-2.6-tip/mm/oom_kill.c
@@ -329,10 +329,13 @@ static void dump_tasks(const struct mem_
 			task_unlock(p);
 			continue;
 		}
+		/* 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();
 		task_unlock(p);
 	} while_each_thread(g, p);
 }

Re: [patch 4/9] oom: Add missing rcu protection of __task_cred() in dump_tasks

[KOSAKI Motohiro]

> 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 <tglx@linutronix.de>
> Cc: linux-mm@kvack.org
> ---
>  mm/oom_kill.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6-tip/mm/oom_kill.c
> ===================================================================
> --- linux-2.6-tip.orig/mm/oom_kill.c
> +++ linux-2.6-tip/mm/oom_kill.c
> @@ -329,10 +329,13 @@ static void dump_tasks(const struct mem_
>  			task_unlock(p);
>  			continue;
>  		}
> +		/* 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();
>  		task_unlock(p);
>  	} while_each_thread(g, p);
>  }

Looks straight forward and correct to me.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

[patch 5/9] security: Use get_task_cred() in keyctl_session_to_parent()

[Thomas Gleixner]

[-- Attachment #1: security-fix-missing-rcu-protection-of-__task_cred.patch --]
[-- Type: text/plain, Size: 1766 bytes --]

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 <tglx@linutronix.de>
Cc: James Morris <jmorris@namei.org>
Cc: linux-security-module@vger.kernel.org
---
 security/keys/keyctl.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6-tip/security/keys/keyctl.c
===================================================================
--- linux-2.6-tip.orig/security/keys/keyctl.c
+++ linux-2.6-tip/security/keys/keyctl.c
@@ -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:

Re: [patch 5/9] security: Use get_task_cred() in keyctl_session_to_parent()

[Paul E. McKenney]

On Thu, Dec 10, 2009 at 12:53:12AM -0000, Thomas Gleixner wrote:
> 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.

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> from RCU perspective.

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: James Morris <jmorris@namei.org>
> Cc: linux-security-module@vger.kernel.org
> ---
>  security/keys/keyctl.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6-tip/security/keys/keyctl.c
> ===================================================================
> --- linux-2.6-tip.orig/security/keys/keyctl.c
> +++ linux-2.6-tip/security/keys/keyctl.c
> @@ -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:
> 
> 

[patch 6/9] signal: Fix racy access to __task_cred in kill_pid_info_as_uid()

[Thomas Gleixner]

[-- Attachment #1: signal-fix-racy-access-to-task-cred.patch --]
[-- Type: text/plain, Size: 1588 bytes --]

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 <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/kernel/signal.c
===================================================================
--- linux-2.6-tip.orig/kernel/signal.c
+++ linux-2.6-tip/kernel/signal.c
@@ -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);

Re: [patch 6/9] signal: Fix racy access to __task_cred in kill_pid_info_as_uid()

[Oleg Nesterov]

On 12/10, Thomas Gleixner wrote:
>
> 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 <tglx@linutronix.de>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c |   17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Acked-by: Oleg Nesterov <oleg@redhat.com>

> Index: linux-2.6-tip/kernel/signal.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/signal.c
> +++ linux-2.6-tip/kernel/signal.c
> @@ -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);
> 
> 

[tip:core/urgent] signal: Fix racy access to __task_cred in kill_pid_info_as_uid()

[tip-bot for Thomas Gleixner]

Commit-ID:  14d8c9f3c09e7fd7b9af80904289fe204f5b93c6
Gitweb:     http://git.kernel.org/tip/14d8c9f3c09e7fd7b9af80904289fe204f5b93c6
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 10 Dec 2009 00:53:17 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Dec 2009 23:04:11 +0100

signal: Fix racy access to __task_cred in 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 <tglx@linutronix.de>
LKML-Reference: <20091210004703.232302055@linutronix.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 6b982f2..7331656 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1175,11 +1175,12 @@ int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid,
 	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 siginfo *info, struct pid *pid,
 	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);

[patch 7/9] signals: Fix more rcu assumptions

[Thomas Gleixner]

[-- Attachment #1: signal-fix-more-rcu-assumptions.patch --]
[-- Type: text/plain, Size: 1830 bytes --]

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.

This needs to be revisited to remove the remaining users of
read_lock(&tasklist_lock) but that's outside the scope of this patch.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Index: linux-2.6-tip/kernel/signal.c
===================================================================
--- linux-2.6-tip.orig/kernel/signal.c
+++ linux-2.6-tip/kernel/signal.c
@@ -220,8 +220,7 @@ __sigqueue_alloc(int sig, struct task_st
 	/*
 	 * We won't get problems with the target's UID changing under us
 	 * because changing it requires RCU be used, and if t != current, the
-	 * caller must be holding the RCU readlock (by way of a spinlock) and
-	 * we use RCU protection here
+	 * caller must be holding the RCU readlock.
 	 */
 	user = get_uid(__task_cred(t)->user);
 	atomic_inc(&user->sigpending);
@@ -946,7 +945,7 @@ out_set:
 static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			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) &&
@@ -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();
+
+	return ret;
 }
 
 static void print_fatal_signal(struct pt_regs *regs, int signr)

Re: [patch 7/9] signals: Fix more rcu assumptions

[Oleg Nesterov]

On 12/10, Thomas Gleixner wrote:
>
> 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,
>  			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) &&
> @@ -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.

Oleg.	

Re: [patch 7/9] signals: Fix more rcu assumptions

[Thomas Gleixner]

On Thu, 10 Dec 2009, Oleg Nesterov wrote:

> On 12/10, Thomas Gleixner wrote:
> >
> > 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,
> >  			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) &&
> > @@ -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.

Indeed. Was too tired to see the obvious :)

Thanks,

	tglx

Re: [patch 7/9] signals: Fix more rcu assumptions

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> > 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 :)

Ah, but...  If __sigqueue_alloc() is called from sigqueue_alloc(), then you
don't need the RCU read lock as the target task is current.

So perhaps the callsite for __sigqueue_alloc() in __send_signal()?  That at
least puts the rcu_read_lock() call in proximity to the function that actually
needs it.

David

[tip:core/urgent] signals: Fix more rcu assumptions

[tip-bot for Thomas Gleixner]

Commit-ID:  7cf7db8df0b78076eafa4ead47559344ca7b7a43
Gitweb:     http://git.kernel.org/tip/7cf7db8df0b78076eafa4ead47559344ca7b7a43
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 10 Dec 2009 00:53:21 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 10 Dec 2009 23:04:11 +0100

signals: Fix more rcu assumptions

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

2) Add a rcu_read_lock/unlock around the __task_cred() access
   in __sigqueue_alloc()

This needs to be revisited to remove the remaining users of
read_lock(&tasklist_lock) but that's outside the scope of this patch.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091210004703.269843657@linutronix.de>
---
 kernel/signal.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7331656..f67545f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -218,13 +218,13 @@ __sigqueue_alloc(int sig, struct task_struct *t, gfp_t flags, int override_rlimi
 	struct user_struct *user;
 
 	/*
-	 * We won't get problems with the target's UID changing under us
-	 * because changing it requires RCU be used, and if t != current, the
-	 * caller must be holding the RCU readlock (by way of a spinlock) and
-	 * we use RCU protection here
+	 * Protect access to @t credentials. This can go away when all
+	 * callers hold rcu read lock.
 	 */
+	rcu_read_lock();
 	user = get_uid(__task_cred(t)->user);
 	atomic_inc(&user->sigpending);
+	rcu_read_unlock();
 
 	if (override_rlimit ||
 	    atomic_read(&user->sigpending) <=

[patch 8/9] Documentation: Fix invalid rcu assumptions

[Thomas Gleixner]

[-- Attachment #1: documentation-fix-rcu-assumptions.patch --]
[-- Type: text/plain, Size: 1758 bytes --]

1) spinlock held is not equivalent to rcu_read_lock()

2) remove the stale signal code snippet

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Vegard Nossum <vegardno@ifi.uio.no>
Cc: James Morris <jmorris@namei.org>
---
 Documentation/credentials.txt |    3 ---
 Documentation/kmemcheck.txt   |    3 +--
 2 files changed, 1 insertion(+), 5 deletions(-)

Index: linux-2.6-tip/Documentation/credentials.txt
===================================================================
--- linux-2.6-tip.orig/Documentation/credentials.txt
+++ linux-2.6-tip/Documentation/credentials.txt
@@ -408,9 +408,6 @@ This should be used inside the RCU read 
 		...
 	}
 
-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:
Index: linux-2.6-tip/Documentation/kmemcheck.txt
===================================================================
--- linux-2.6-tip.orig/Documentation/kmemcheck.txt
+++ linux-2.6-tip/Documentation/kmemcheck.txt
@@ -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);

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Vegard Nossum]

[trimmed Cc]

>2) remove the stale signal code snippet
...
2009/12/10 Thomas Gleixner <tglx@linutronix.de>:
> Index: linux-2.6-tip/Documentation/kmemcheck.txt
> ===================================================================
> --- linux-2.6-tip.orig/Documentation/kmemcheck.txt
> +++ linux-2.6-tip/Documentation/kmemcheck.txt
> @@ -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);

I am not sure that I really agree with this change. This is not a code
example for the sake of showing how to do a particular thing, it's an
example of real code from the tree.

I don't remember if the document is referring to a particular git
version of the code, but I think it might not, in which case it
doesn't REALLY matter even on the microscopic level.

But I won't make a big fuss about it :-)


Vegard

PS: Upon closer inspection, I noticed that one line (line 197) goes
completely missing, there seems to be a typo there too, "readlocke".
Still it's not a huge deal, I admit.

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Arnd Bergmann]

On Thursday 10 December 2009 00:53:26 Thomas Gleixner wrote:
> Index: linux-2.6-tip/Documentation/credentials.txt
> ===================================================================
> --- linux-2.6-tip.orig/Documentation/credentials.txt
> +++ linux-2.6-tip/Documentation/credentials.txt
> @@ -408,9 +408,6 @@ This should be used inside the RCU read 
>                 ...
>         }
>  
> -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.

	Arnd

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Paul E. McKenney]

On Fri, Dec 11, 2009 at 09:28:25PM +0000, Arnd Bergmann wrote:
> On Thursday 10 December 2009 00:53:26 Thomas Gleixner wrote:
> > Index: linux-2.6-tip/Documentation/credentials.txt
> > ===================================================================
> > --- linux-2.6-tip.orig/Documentation/credentials.txt
> > +++ linux-2.6-tip/Documentation/credentials.txt
> > @@ -408,9 +408,6 @@ This should be used inside the RCU read 
> >                 ...
> >         }
> >  
> > -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.

That does make a lot of sense...  Will add that in.

							Thanx, Paul

[patch 9/9] security: Fix invalid rcu assumptions in comments

[Thomas Gleixner]

[-- Attachment #1: security-fix-rcu-assumptions.patch --]
[-- Type: text/plain, Size: 1595 bytes --]

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 <tglx@linutronix.de>
Cc: James Morris <jmorris@namei.org>
Cc: linux-security-module@vger.kernel.org

---
 security/keys/permission.c |    3 +--
 security/keys/proc.c       |    2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

Index: linux-2.6-tip/security/keys/permission.c
===================================================================
--- linux-2.6-tip.orig/security/keys/permission.c
+++ linux-2.6-tip/security/keys/permission.c
@@ -23,8 +23,7 @@
  * Check to see whether permission is granted to use a key in the desired way,
  * but permit the security modules to override.
  *
- * The caller must hold either a ref on cred or must hold the RCU readlock or a
- * spinlock.
+ * The caller must hold either a ref on cred or must hold the RCU readlock.
  */
 int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 			key_perm_t perm)
Index: linux-2.6-tip/security/keys/proc.c
===================================================================
--- linux-2.6-tip.orig/security/keys/proc.c
+++ linux-2.6-tip/security/keys/proc.c
@@ -194,8 +194,6 @@ static int proc_keys_show(struct seq_fil
 
 	/* check whether the current task is allowed to view the key (assuming
 	 * non-possession)
-	 * - the caller holds a spinlock, and thus the RCU read lock, making our
-	 *   access to __current_cred() safe
 	 */
 	rc = key_task_permission(make_key_ref(key, 0), current_cred(),
 				 KEY_VIEW);

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Thu, Dec 10, 2009 at 12:52:46AM -0000, Thomas Gleixner wrote:
> 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.

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

2.	Do the RCU read-side critical sections make proper use of
	rcu_read_lock() and friends?  These primitives are needed
	to prevent grace periods from ending prematurely, which
	could result in data being unceremoniously freed out from
	under your read-side code, which can greatly increase the
	actuarial risk of your kernel.

	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.

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

Same item #2 above covers this.

The only exception is when you use synchronize_sched(), as described
in the "Defer"/"Protect" list near line 323 of the 2.6.32 version of
Documentation/RCU/whatisRCU.txt:

	Defer			Protect

a.	synchronize_rcu()	rcu_read_lock() / rcu_read_unlock()
	call_rcu()

b.	call_rcu_bh()		rcu_read_lock_bh() / rcu_read_unlock_bh()

c.	synchronize_sched()	preempt_disable() / preempt_enable()
				local_irq_save() / local_irq_restore()
				hardirq enter / hardirq exit
				NMI enter / NMI exit

And yes, I need to update this based on the addition of
rcu_read_lock_sched() and friends.  I will be doing another
documentation update soon.

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

Agreed!!!

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

Thank you very much for putting this series together -- I will take
a quick look at them.

							Thanx, Paul

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Linus Torvalds]

On Wed, 9 Dec 2009, Paul E. McKenney wrote:

> On Thu, Dec 10, 2009 at 12:52:46AM -0000, Thomas Gleixner wrote:
> > 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.
> 
> 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..)

		Linus

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Peter Zijlstra]

On Wed, 2009-12-09 at 19:15 -0800, Linus Torvalds wrote:
> 
> On Wed, 9 Dec 2009, Paul E. McKenney wrote:
> 
> > On Thu, Dec 10, 2009 at 12:52:46AM -0000, Thomas Gleixner wrote:
> > > 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.
> > 
> > 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.

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Thu, Dec 10, 2009 at 06:13:51AM +0100, Peter Zijlstra wrote:
> On Wed, 2009-12-09 at 19:15 -0800, Linus Torvalds wrote:
> > 
> > On Wed, 9 Dec 2009, Paul E. McKenney wrote:
> > 
> > > On Thu, Dec 10, 2009 at 12:52:46AM -0000, Thomas Gleixner wrote:
> > > > 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.
> > > 
> > > 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.

							Thanx, Paul

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Peter Zijlstra]

On Wed, 2009-12-09 at 21:34 -0800, Paul E. McKenney wrote:

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

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Sun, Dec 13, 2009 at 07:56:17PM +0100, Peter Zijlstra wrote:
> On Wed, 2009-12-09 at 21:34 -0800, Paul E. McKenney wrote:
> 
> > 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?

							Thanx, Paul

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Peter Zijlstra]

On Sun, 2009-12-13 at 17:53 -0800, Paul E. McKenney wrote:
> On Sun, Dec 13, 2009 at 07:56:17PM +0100, Peter Zijlstra wrote:
> > On Wed, 2009-12-09 at 21:34 -0800, Paul E. McKenney wrote:
> > 
> > > 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?

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.

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Mon, Dec 14, 2009 at 11:17:39AM +0100, Peter Zijlstra wrote:
> On Sun, 2009-12-13 at 17:53 -0800, Paul E. McKenney wrote:
> > On Sun, Dec 13, 2009 at 07:56:17PM +0100, Peter Zijlstra wrote:
> > > On Wed, 2009-12-09 at 21:34 -0800, Paul E. McKenney wrote:
> > > 
> > > > 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?
> 
> The only concern is drowning in rcu_dereference() annotations. But I
> guess that is unavoidable.

So far, I haven't been able to think of anything better.  :-/

> 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

							Thanx, Paul

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Peter Zijlstra]

On Mon, 2009-12-14 at 06:16 -0800, Paul E. McKenney wrote:

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

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.

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Mon, Dec 14, 2009 at 03:30:39PM +0100, Peter Zijlstra wrote:
> On Mon, 2009-12-14 at 06:16 -0800, Paul E. McKenney wrote:
> 
> > > 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
> 
> 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.

Sounds good, thank you for the tip!!!

							Thanx, Paul

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

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

Thanks,
David

Re: [patch 0/9] Fix various __task_cred related invalid RCU assumptions

[Paul E. McKenney]

On Fri, Dec 11, 2009 at 01:39:06PM +0000, David Howells wrote:
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > 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!

It did indeed change with the split of the old synchronize_kernel()
primitive into the synchronize_rcu() and synchronize_sched() primitives
in 2.6.12 -- after this point, synchronize_rcu() was only guaranteed
to respect "real" rcu_read_lock()-base critical sections.  But actual
failures would not show up until PREEMPT_RCU was introduced into 2.6.25.

The -rt effort rooted out a bunch of these sorts of problems, but we
clearly missed a few.

							Thanx, Paul

Re: [patch 1/9] sys: Fix missing rcu protection for __task_cred() access

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>
> Cc: James Morris <jmorris@namei.org>
> Cc: linux-security-module@vger.kernel.org

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 2/9] fs: Add missing rcu protection for __task_cred() in sys_ioprio_get

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 3/9] proc: Add missing rcu protection for __task_cred() in task_sig()

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 4/9] oom: Add missing rcu protection of __task_cred() in dump_tasks

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> +		/* 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:

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

David

Re: [patch 4/9] oom: Add missing rcu protection of __task_cred() in dump_tasks

[Thomas Gleixner]

On Fri, 11 Dec 2009, David Howells wrote:

> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > +		/* 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:
> 
> -		       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.

Fair enough.

     tglx

Re: [patch 5/9] security: Use get_task_cred() in keyctl_session_to_parent()

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 6/9] signal: Fix racy access to __task_cred in kill_pid_info_as_uid()

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

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

You mean "readlock" I suspect.

Otherwise, feel free to add:

Acked-by: David Howells <dhowells@redhat.com>

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Linus Torvalds]

On Fri, 11 Dec 2009, David Howells wrote:

> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > -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.

			Linus

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Paul E. McKenney]

On Fri, Dec 11, 2009 at 08:07:45AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 11 Dec 2009, David Howells wrote:
> 
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > -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.

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

							Thanx, Paul

Re: [patch 8/9] Documentation: Fix invalid rcu assumptions

[Thomas Gleixner]

On Fri, 11 Dec 2009, Paul E. McKenney wrote:
> On Fri, Dec 11, 2009 at 08:07:45AM -0800, Linus Torvalds wrote:
> > 
> > 
> > On Fri, 11 Dec 2009, David Howells wrote:
> > 
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > 
> > > > -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.
> 
> 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

The good thing about "Read Locke" is: there is no "Write Locke".

Thanks,
    	 tglx

Re: [patch 9/9] security: Fix invalid rcu assumptions in comments

[David Howells]

Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>

Acked-by: David Howells <dhowells@redhat.com>