Re: [PATCH] tracing: fprobe: fix suspicious rcu usage in fprobe_entry

[Herbert Xu <herbert@gondor.apana.org.au>]

Paul E. McKenney <paulmck@kernel.org> wrote:
>
> Yes, preempt_disable() does indeed start an RCU read-side critical section,
> just as surely as rcu_read_lock() does.
> 
> However, this is a lockdep check inside of __rhashtable_lookup():
> 
>        rht_dereference_rcu(ht->tbl, ht)
> 
> Which is defined as:
> 
>        rcu_dereference_check(p, lockdep_rht_mutex_is_held(ht));
> 
> This is explicitly telling lockdep that rcu_read_lock() is OK and
> holding ht->mutex is OK, but nothing else is.

I think that's a deficiency in rcu_dereference_check.

Yes I could certainly add a preemption check to rht_dereference_rcu,
but that makes zero sense because this is an implementation detail
of RCU and there is no reason why this logic should be added to
rhashtable.

rhashtable never relies on the fact that turning off preemption
creates is safe for RCU reads, so it makes no sense to add this
logic to rht_dereference_rcu since RCU could conceivably (even
if it is unlikely) be changed on day so that turning off preemption
is no longer safe for RCU reads.

My preference would be to add the preemption test to
rcu_dereference_check, or if that is not possible for some reason,
create a new RCU helper that includes the preemption test.

Of course just adding RCU read locks as this patch does is also
fine.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt