public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Andreea-Cristina Bernat <bernat.ada@gmail.com>
To: dhowells@redhat.com, shemming@brocade.com, linux-kernel@vger.kernel.org
Cc: paulmck@linux.vnet.ibm.com
Subject: [PATCH RFC] rcu: assoc_array: Add critical section to avoid a bug
Date: Tue, 26 Aug 2014 18:41:29 +0300	[thread overview]
Message-ID: <20140826154129.GA4481@ada> (raw)

* The function "assoc_array_apply_edit()" passes "edit->rcu" to "call_rcu()".
* "assoc_array_apply_edit()'s" callers do not call it under an RCU
read-side critical section.
* The function "assoc_array_gc()" could be preempted between the call to
"assoc_array_apply_edit()" call and the assignment
"edit->array->nr_leaves_on_tree = nr_leaves_on_tree;", so the grace
period could complete.
* The problem is that the assignment could access the freelist.
* Solution:
 Put the call to the "assoc_array_apply_edit()" function and the
 assignment between "rcu_read_lock()" and "rcu_read_unlock()". In this
 way, the callback function provided by the "call_rcu()" call will be
 invoked after all pre-existing critical sections end which means after
 the added critical section.

The first step to detect this was made with the following Coccinelle
semantic patch which looks for calls of functions which have in their bodies a
call to "call_rcu()" with a global first parameter:

@ locally @
identifier l;
type t;
position p;
@@

t l;
... when any
call_rcu@p((<+...l...+>), ...);

@ globally @
identifier fn;
identifier g != locally.l;
position p2 != locally.p;
@@

fn(...) {
... when any
call_rcu@p2((<+...g...+>), ...);
... when any
}

@ other_func @
identifier globally.fn, ff;
@@

ff(...) {
... when any
* fn(...)
... when any
}

The second step was looking at the uses of the functions found and see if the
parameter of interest is used after the call of the function.

Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
---
 lib/assoc_array.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index c0b1007..45d11c0 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -1734,8 +1734,10 @@ ascend_old_tree:
 
 gc_complete:
 	edit->set[0].to = new_root;
+	rcu_read_lock();
 	assoc_array_apply_edit(edit);
 	edit->array->nr_leaves_on_tree = nr_leaves_on_tree;
+	rcu_read_unlock();
 	return 0;
 
 enomem:
-- 
1.9.1


             reply	other threads:[~2014-08-26 15:41 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-26 15:41 Andreea-Cristina Bernat [this message]
2014-08-28 11:32 ` [PATCH RFC] rcu: assoc_array: Add critical section to avoid a bug David Howells
2014-08-29 17:57   ` Andreea Bernat
2014-08-29 22:49     ` David Howells
2014-08-29 23:08       ` Andreea Bernat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140826154129.GA4481@ada \
    --to=bernat.ada@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=shemming@brocade.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox