| 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
| ||
|
Message-ID: <20081017150101.GE6706@linux.vnet.ibm.com>
Date: Fri, 17 Oct 2008 08:01:01 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Lai Jiangshan <laijs@...fujitsu.com>
Cc: Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rcupdate: fix 2 bugs of rcu_barrier*()
On Fri, Oct 17, 2008 at 01:47:42PM +0800, Lai Jiangshan wrote:
> Paul E. McKenney wrote:
> > On Thu, Oct 16, 2008 at 04:51:56PM +0800, Lai Jiangshan wrote:
> >> current rcu_barrier_bh() is like this:
> >>
> >> void rcu_barrier_bh(void)
> >> {
> >> BUG_ON(in_interrupt());
> >> /* Take cpucontrol mutex to protect against CPU hotplug */
> >> mutex_lock(&rcu_barrier_mutex);
> >> init_completion(&rcu_barrier_completion);
> >> atomic_set(&rcu_barrier_cpu_count, 0);
> >> /*
> >> * The queueing of callbacks in all CPUs must be atomic with
> >> * respect to RCU, otherwise one CPU may queue a callback,
> >> * wait for a grace period, decrement barrier count and call
> >> * complete(), while other CPUs have not yet queued anything.
> >> * So, we need to make sure that grace periods cannot complete
> >> * until all the callbacks are queued.
> >> */
> >> rcu_read_lock();
> >> on_each_cpu(rcu_barrier_func, (void *)RCU_BARRIER_BH, 1);
> >> rcu_read_unlock();
> >> wait_for_completion(&rcu_barrier_completion);
> >> mutex_unlock(&rcu_barrier_mutex);
> >> }
> >>
> >> this is bug, rcu_read_lock() cannot make sure that "grace periods for RCU_BH
> >> cannot complete until all the callbacks are queued".
> >> it only make sure that race periods for RCU cannot complete
> >> until all the callbacks are queued.
> >>
> >> so we must use rcu_read_lock_bh() for rcu_barrier_bh().
> >> like this:
> >>
> >> void rcu_barrier_bh(void)
> >> {
> >> ......
> >> rcu_read_lock_bh();
> >> on_each_cpu(rcu_barrier_func, (void *)RCU_BARRIER_BH, 1);
> >> rcu_read_unlock_bh();
> >> ......
> >> }
> >>
> >> and also rcu_barrier() rcu_barrier_sched() are implemented like this.
> >> it will bring a lot of duplicate code. My patch uses another way to
> >> fix this bug, please see the comment of my patch.
> >
> > Excellent catch!!! I had incorrectly convinced myself that because RCU
> > read-side implies an RCU_BH and RCU_SCHED that I could simply use an
> > RCU read-side critical section. Thank you for finding this!
> >
> > Just out of curiosity, did an actual oops/hang lead you to this bug, or
> > did you find it by inspection?
>
> by inspection. I was planning to put synchronize_rcu* back to
> kernel/rcupdate.c and I found the code and the comments are
> inconsistent suddenly when I was reviewing kernel/rcupdate.c.
Good eyes!!!
> >> Bug 2:
> >> on_each_cpu() do not imply wmb, so we need a explicit wmb.
> >> I became a paranoid too.
> >
> > Actually, there is a memory barrier in the list_add_tail_rcu() in the
> > implementation of smp_call_function(), and furthermore, the way that
> > atomic operations work on all architectures I am aware of removes the need
> > for the memory barrier. Nevertheless, I have absolutely no objection
> > to adding this memory barrier. This code path is used infrequently and
> > has high overhead anyway, so I agree that making it easy to understand
> > is the correct approach. If it were on the read side, I would argue. ;-)
>
> I will remove this wmb.
> Thank you a lot
Sounds good to me -- on_each_cpu() really needs to provide the barrier
internally anyway, otherwise it is too hard to use. So am OK with your
leaving the wmb out.
Thanx, Paul
> Lai.
>
> >
> > In any case, I must agree that you are doing a good job of learning to
> > be paranoid!
> >
> > The only change I suggest is to rewrite the comments as shown below.
> >
> > With that update, this change should be applied.
> >
> > Reviewed-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> >
> >> Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
> >> ---
> >> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> >> index 467d594..a667e21 100644
> >> --- a/kernel/rcupdate.c
> >> +++ b/kernel/rcupdate.c
> >> @@ -119,18 +119,23 @@ static void _rcu_barrier(enum rcu_barrier type)
> >> /* Take cpucontrol mutex to protect against CPU hotplug */
> >> mutex_lock(&rcu_barrier_mutex);
> >> init_completion(&rcu_barrier_completion);
> >> - atomic_set(&rcu_barrier_cpu_count, 0);
> >> /*
> >> - * The queueing of callbacks in all CPUs must be atomic with
> >> - * respect to RCU, otherwise one CPU may queue a callback,
> >> - * wait for a grace period, decrement barrier count and call
> >> - * complete(), while other CPUs have not yet queued anything.
> >> - * So, we need to make sure that grace periods cannot complete
> >> - * until all the callbacks are queued.
> >> + * init and set rcu_barrier_cpu_count to 1, otherwise(set it to 0)
> >> + * one CPU may queue a callback, wait for a grace period, decrement
> >> + * barrier count and call complete(), while other CPUs have not yet
> >> + * queued anything.
> >> + * So, we need to make sure that rcu_barrier_cpu_count cannot become
> >> + * 0 until all the callbacks are queued.
> >
> > * Initialize rcu_barrier_cpu_count to 1, then invoke
> > * rcu_barrier_func() on each CPU, so that each CPU also has
> > * incremented rcu_barrier_cpu_count. Only then is it safe to
> > * decrement rcu_barrier_cpu_count -- otherwise the first CPU
> > * might complete its grace period before all of the other CPUs
> > * did their increment, causing this function to return too
> > * early.
> >
> >> */
> >> - rcu_read_lock();
> >> + atomic_set(&rcu_barrier_cpu_count, 1);
> >> + /*
> >> + * rcu_barrier_cpu_count = 1 must be visible to cpus before
> >> + * them call rcu_barrier_func().
> >> + */
> >> + smp_wmb();
> >
> > smp_wmb(); /* atomic_set() must precede all rcu_barrier_func()s. */
> >
> >> on_each_cpu(rcu_barrier_func, (void *)type, 1);
> >> - rcu_read_unlock();
> >> + if (atomic_dec_and_test(&rcu_barrier_cpu_count))
> >> + complete(&rcu_barrier_completion);
> >> wait_for_completion(&rcu_barrier_completion);
> >> mutex_unlock(&rcu_barrier_mutex);
> >> }
> >>
> >>
> >>
> >
> >
> >
>
>
--
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/
Powered by blists - more mailing lists