linux-kernel - Re: [PATCH 00/10] __rcu annotations, first draft

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100225083751.GA18272@elte.hu>
Date:	Thu, 25 Feb 2010 09:37:51 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	paulmck@...ux.vnet.ibm.com,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	linux-kernel@...r.kernel.org, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	josh@...htriplett.org, dvhltc@...ibm.com, niv@...ibm.com,
	tglx@...utronix.de, peterz@...radead.org, rostedt@...dmis.org,
	Valdis.Kletnieks@...edu, dhowells@...hat.com
Subject: Re: [PATCH 00/10] __rcu annotations, first draft


* Arnd Bergmann <arnd@...db.de> wrote:

> On Tuesday 23 February 2010, Paul E. McKenney wrote:
> > > I've just started an experimental implementation and got stuck at list rcu.
> > > The two to deal with it that I can see are
> > > - ignore list-rcu for now, and make all include/linux/rculist.h __force the
> > >   problem to be ignored.
> > > - introduce a new struct rcu_list_head that needs to be used for list rcu.
> > >
> > > A nicer option might be if sparse would let you write
> > > 'struct list_head __rcu head' and interpret that as having the pointers
> > > inside it annotated as __rcu.
> >
> > Only the "next" pointer, not the "prev" pointer, but yes.
> >
> > Perhaps it would be best to see if the sparse guys are willing to take
> > this on?
> 
> I've implemented the second option now, and this seems to work alright.
> In order to test this out, I've annotated all rcu users in the kernel
> directory. One observation is that many members of task_struct are
> using RCU inconsistently. I'm not sure whether these should be considered
> bugs or false positives though. If we get a lot of false positives or
> people are generally unhappy about the annotations, they are not worth
> it, but if we find a few actual bugs, I guess it would be good to
> have this.
> 
> Examples of pointers that I did not annotate because most of the
> places accessing them do not use RCU are task->sighand, task->nsproxy,
> and task->real_parent.
> 
> Sorry for hijacking the discussion on your patch, but I assume that
> the same people interested in the lockdep diagnostics are also
> interested in the static annotations for RCU.
> 
> 	Arnd
> 
> Arnd Bergmann (10):
>   rcu: define __rcu address space modifier for sparse
>   rcu: annotated list rcu code
>   cgroups: __rcu annotations
>   credentials: rcu annotation
>   perf_event: __rcu annotations
>   audit: __rcu annotations
>   module: __rcu annotations
>   pid: __rcu annotations
>   notifiers: __rcu annotations
>   scheduler: __rcu annotations
> 
>  include/linux/cgroup.h     |    6 +-
>  include/linux/compiler.h   |    2 +
>  include/linux/cred.h       |    4 +-
>  include/linux/fdtable.h    |    2 +-
>  include/linux/init_task.h  |    6 +-
>  include/linux/module.h     |    4 +-
>  include/linux/notifier.h   |   10 ++--
>  include/linux/perf_event.h |   10 ++--
>  include/linux/pid.h        |    9 ++-
>  include/linux/rculist.h    |  152 ++++++++++++++++++++++++++++++++------------
>  include/linux/rcupdate.h   |   46 ++++++++++++--
>  include/linux/sched.h      |   14 ++--
>  kernel/audit.c             |    4 +-
>  kernel/audit.h             |    6 +-
>  kernel/audit_tree.c        |   14 ++--
>  kernel/auditfilter.c       |   28 ++++----
>  kernel/auditsc.c           |    8 +-
>  kernel/cgroup.c            |   63 ++++++++++---------
>  kernel/cpuset.c            |    2 +-
>  kernel/cred.c              |   50 ++++++++-------
>  kernel/exit.c              |   12 ++--
>  kernel/fork.c              |    6 +-
>  kernel/module.c            |   20 ++++--
>  kernel/notifier.c          |   12 ++--
>  kernel/perf_event.c        |   52 ++++++++--------
>  kernel/pid.c               |    8 +-
>  kernel/sched.c             |   31 +++++----
>  27 files changed, 352 insertions(+), 229 deletions(-)

This looks very clean IMO.

By far the biggest remaining design aspects of RCU is that it's somewhat on 
the opaque side of APIs: it's not immediately obvious when a data structure 
(and subsequently, code using that date structure) is affected by RCU 
semantics.

This is good in terms of API utility: you _can_ use it with very little 
resistence on the way - you can combine RCU and non-RCU code with few 
syntactic constraints imposed.

That kind of flexibility inevitably has its downsides: with a quick guess i'd 
say more than half of the non-trivial RCU related usage bugs in various 
subsystem stemmed out of using non-RCU facilities on RCU data structures.

So the annotation both 'look' clean [which alone is a plus as people write new 
code] and they also might result in bugfixes ...

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/