[PATCH] sched: mark parent and real_parent as __rcu

[PATCH] sched: mark parent and real_parent as __rcu

[Kees Cook]

The parent and real_parent pointers should be considered __rcu, since
they should be held under either tasklist_lock or rcu_read_lock.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/sched.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1c4f3e9..b05e8af 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1315,8 +1315,8 @@ struct task_struct {
 	 * older sibling, respectively.  (p->father can be replaced with 
 	 * p->real_parent->pid)
 	 */
-	struct task_struct *real_parent; /* real parent process */
-	struct task_struct *parent; /* recipient of SIGCHLD, wait4() reports */
+	struct task_struct __rcu *real_parent; /* real parent process */
+	struct task_struct __rcu *parent; /* recipient of SIGCHLD, wait4() reports */
 	/*
 	 * children/sibling forms the list of my natural children
 	 */
-- 
1.7.0.4

Re: [PATCH] sched: mark parent and real_parent as __rcu

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> The parent and real_parent pointers should be considered __rcu, since
> they should be held under either tasklist_lock or rcu_read_lock.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/sched.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Did you get some warning that alerted you to this problem? If 
yes then please include it in the changelog, so that people 
hitting the message can easily find it.

Same consideration applies the second patch as well.

Thanks,

	Ingo

Re: [PATCH] sched: mark parent and real_parent as __rcu

[Kees Cook]

On Wed, Dec 14, 2011 at 11:08 PM, Ingo Molnar <mingo@elte.hu> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>
>> The parent and real_parent pointers should be considered __rcu, since
>> they should be held under either tasklist_lock or rcu_read_lock.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/linux/sched.h |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> Did you get some warning that alerted you to this problem? If
> yes then please include it in the changelog, so that people
> hitting the message can easily find it.
>
> Same consideration applies the second patch as well.

No, this was via code inspection related to reviews of the Yama LSM's
use of these structures and comparing the RCU and tasklist_lock
behavior/requirements seen in the rest of the kernel. Most attempts at
using sparse after this change were not very successful due to the
high volume of unrelated noise currently seen with "make C=2", but I
was able to confirm that the apparmor and tomoyo patches were flagged
by sparse, at least. When this __rcu marking is in place, if there is
a place that might need rcu_dereference, it would show up as "warning:
dereference of noderef expression".

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] sched: mark parent and real_parent as __rcu

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> On Wed, Dec 14, 2011 at 11:08 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> The parent and real_parent pointers should be considered __rcu, since
> >> they should be held under either tasklist_lock or rcu_read_lock.
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >>  include/linux/sched.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > Did you get some warning that alerted you to this problem? If
> > yes then please include it in the changelog, so that people
> > hitting the message can easily find it.
> >
> > Same consideration applies the second patch as well.
> 
> No, this was via code inspection related to reviews of the 
> Yama LSM's use of these structures and comparing the RCU and 
> tasklist_lock behavior/requirements seen in the rest of the 
> kernel. Most attempts at using sparse after this change were 
> not very successful due to the high volume of unrelated noise 
> currently seen with "make C=2", but I was able to confirm that 
> the apparmor and tomoyo patches were flagged by sparse, at 
> least. When this __rcu marking is in place, if there is a 
> place that might need rcu_dereference, it would show up as 
> "warning: dereference of noderef expression".

ok - i'll apply the first patch, please send the second one 
against -tip, it does not apply anymore. (scheduler got moved to 
kernel/sched/)

Thanks,

	Ingo

[tip:sched/core] sched: Mark parent and real_parent as __rcu

[tip-bot for Kees Cook]

Commit-ID:  abd63bc3a0f65ae9d85bc3b1bb067d3e3c2b2cc2
Gitweb:     http://git.kernel.org/tip/abd63bc3a0f65ae9d85bc3b1bb067d3e3c2b2cc2
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Wed, 14 Dec 2011 14:39:26 -0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 15 Dec 2011 08:21:59 +0100

sched: Mark parent and real_parent as __rcu

The parent and real_parent pointers should be considered __rcu,
since they should be held under either tasklist_lock or
rcu_read_lock.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Link: http://lkml.kernel.org/r/20111214223925.GA27578@www.outflux.net
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/sched.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cc8c620..5ef0901 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1330,8 +1330,8 @@ struct task_struct {
 	 * older sibling, respectively.  (p->father can be replaced with 
 	 * p->real_parent->pid)
 	 */
-	struct task_struct *real_parent; /* real parent process */
-	struct task_struct *parent; /* recipient of SIGCHLD, wait4() reports */
+	struct task_struct __rcu *real_parent; /* real parent process */
+	struct task_struct __rcu *parent; /* recipient of SIGCHLD, wait4() reports */
 	/*
 	 * children/sibling forms the list of my natural children
 	 */