| 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: <1216139149.12595.224.camel@twins>
Date: Tue, 15 Jul 2008 18:25:49 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc: akpm@...ux-foundation.org, Ingo Molnar <mingo@...e.hu>,
linux-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...hat.com>,
"Frank Ch. Eigler" <fche@...hat.com>,
Hideo AOKI <haoki@...hat.com>,
Takashi Nishiie <t-nishiie@...css.fujitsu.com>,
Steven Rostedt <rostedt@...dmis.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>,
Paul E McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: [patch 01/15] Kernel Tracepoints
On Tue, 2008-07-15 at 12:08 -0400, Mathieu Desnoyers wrote:
> * Peter Zijlstra (peterz@...radead.org) wrote:
> > On Tue, 2008-07-15 at 11:22 -0400, Mathieu Desnoyers wrote:
> > > * Peter Zijlstra (peterz@...radead.org) wrote:
> > > >
> > > > I'm confused by the barrier games here.
> > > >
> > > > Why not:
> > > >
> > > > void **it_func;
> > > >
> > > > preempt_disable();
> > > > it_func = rcu_dereference((tp)->funcs);
> > > > if (it_func) {
> > > > for (; *it_func; it_func++)
> > > > ((void(*)(proto))(*it_func))(args);
> > > > }
> > > > preempt_enable();
> > > >
> > > > That is, why can we skip the barrier when !it_func? is that because at
> > > > that time we don't actually dereference it_func and therefore cannot
> > > > observe stale data?
> > > >
> > >
> > > Exactly. I used the implementation of rcu_assign_pointer as a hint that
> > > we did not need barriers when setting the pointer to NULL, and thus we
> > > should not need the read barrier when reading the NULL pointer, because
> > > it references no data.
> > >
> > > #define rcu_assign_pointer(p, v) \
> > > ({ \
> > > if (!__builtin_constant_p(v) || \
> > > ((v) != NULL)) \
> > > smp_wmb(); \
> > > (p) = (v); \
> > > })
> >
> > Yeah, I saw that,.. made me wonder. It basically assumes that when we
> > write:
> >
> > rcu_assign_pointer(foo, NULL);
> >
> > foo will not be used as an index or offset.
> >
> > I guess Paul has thought it through and verified all in-kernel use
> > cases, but it still makes me feel unconfortable.
> >
> > > #define rcu_dereference(p) ({ \
> > > typeof(p) _________p1 = ACCESS_ONCE(p); \
> > > smp_read_barrier_depends(); \
> > > (_________p1); \
> > > })
> > >
> > > But I think you are right, since we are already in unlikely code, using
> > > rcu_dereference as you do is better than my use of read barrier depends.
> > > It should not change anything in the assembly result except on alpha,
> > > where the read_barrier_depends() is not a nop.
> > >
> > > I wonder if there would be a way to add this kind of NULL pointer case
> > > check without overhead in rcu_dereference() on alpha. I guess not, since
> > > the pointer is almost never known at compile-time. And I guess Paul must
> > > already have thought about it. The only case where we could add this
> > > test is when we know that we have a if (ptr != NULL) test following the
> > > rcu_dereference(); we could then assume the compiler will merge the two
> > > branches since they depend on the same condition.
> >
> > I remember seeing a thread about all this special casing NULL, but have
> > never been able to find it again - my google skillz always fail me.
> >
> > Basically it doesn't work if you use the variable as an index/offset,
> > because in that case 0 is a valid offset and you still generate a data
> > dependency.
> >
> > IIRC the conclusion was that the gains were too small to spend more time
> > on it, although I would like to hear about the special case in
> > rcu_assign_pointer.
> >
> > /me goes use git blame....
> >
>
> Actually, we could probably do the following, which also adds an extra
> coherency check about non-NULL pointer assumptions :
>
> #ifdef CONFIG_RCU_DEBUG /* this would be new */
> #define DEBUG_RCU_BUG_ON(x) BUG_ON(x)
> #else
> #define DEBUG_RCU_BUG_ON(x)
> #endif
>
> #define rcu_dereference(p) ({ \
> typeof(p) _________p1 = ACCESS_ONCE(p); \
> if (p != NULL) \
> smp_read_barrier_depends(); \
> (_________p1); \
> })
>
> #define rcu_dereference_non_null(p) ({ \
> typeof(p) _________p1 = ACCESS_ONCE(p); \
> DEBUG_RCU_BUG_ON(p == NULL); \
> smp_read_barrier_depends(); \
> (_________p1); \
> })
>
> The use-case where rcu_dereference() would be used is when it is
> followed by a null pointer check (grepping through the sources shows me
> this is a very very common case). In rare cases, it is assumed that the
> pointer is never NULL and it is used just after the rcu_dereference. It
> those cases, the extra test could be saved on alpha by using
> rcu_dereference_non_null(p), which would check the the pointer is indeed
> never NULL under some debug kernel configuration.
>
> Does it make sense ?
This would break the case where the dereferenced variable is used as an
index/offset where 0 is a valid value and still generates data
dependencies.
So if with your new version we do:
i = rcu_dereference(foo);
j = table[i];
which translates into:
i = ACCESS_ONCE(foo);
if (i)
smp_read_barrier_depends();
j = table[i];
which when i == 0, would fail to do the barrier and can thus cause j to
be a wrong value.
Sadly I'll have to defer to Paul to explain exactly how that can happen
- I always get my head in a horrible twist with this case.
--
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