Re: [PATCH -tip] kprobes: Lock rcu_read_lock() while searching kprobe

[Paul E. McKenney]

On Tue, Dec 03, 2019 at 08:13:29AM +0100, Ingo Molnar wrote:
> 
> * Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> 
> > On Mon, Dec 02, 2019 at 04:32:13PM +0900, Masami Hiramatsu wrote:
> > > Anders reported that the lockdep warns that suspicious
> > > RCU list usage in register_kprobe() (detected by
> > > CONFIG_PROVE_RCU_LIST.) This is because get_kprobe()
> > > access kprobe_table[] by hlist_for_each_entry_rcu()
> > > without rcu_read_lock.
> > > 
> > > If we call get_kprobe() from the breakpoint handler context,
> > > it is run with preempt disabled, so this is not a problem.
> > > But in other cases, instead of rcu_read_lock(), we locks
> > > kprobe_mutex so that the kprobe_table[] is not updated.
> > > So, current code is safe, but still not good from the view
> > > point of RCU.
> > > 
> > > Let's lock the rcu_read_lock() around get_kprobe() and
> > > ensure kprobe_mutex is locked at those points.
> > > 
> > > Note that we can safely unlock rcu_read_lock() soon after
> > > accessing the list, because we are sure the found kprobe has
> > > never gone before unlocking kprobe_mutex. Unless locking
> > > kprobe_mutex, caller must hold rcu_read_lock() until it
> > > finished operations on that kprobe.
> > > 
> > > Reported-by: Anders Roxell <anders.roxell@xxxxxxxxxx>
> > > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > 
> > Instead of this, can you not just pass the lockdep_is_held() expression as
> > the last argument to list_for_each_entry_rcu() to silence the warning? Then
> > it will be a simpler patch.
> 
> Come on, we do not silence warnings!
> 
> If it's safely inside the lock then why not change it from 
> hlist_for_each_entry_rcu() to hlist_for_each_entry()?
> 
> I do think that 'lockdep flag' inside hlist_for_each_entry_rcu():
> 
> /**
>  * hlist_for_each_entry_rcu - iterate over rcu list of given type
>  * @pos:        the type * to use as a loop cursor.
>  * @head:       the head for your list.
>  * @member:     the name of the hlist_node within the struct.
>  * @cond:       optional lockdep expression if called from non-RCU protection.
>  *
>  * This list-traversal primitive may safely run concurrently with
>  * the _rcu list-mutation primitives such as hlist_add_head_rcu()
>  * as long as the traversal is guarded by rcu_read_lock().
>  */
> #define hlist_for_each_entry_rcu(pos, head, member, cond...)            \
> 
> is actively harmful. Why is it there?

For cases where common code might be invoked both from the reader
(with RCU protection) and from the updater (protected by some
lock).  This common code can then use the optional argument to
hlist_for_each_entry_rcu() to truthfully tell lockdep that it might be
called with either form of protection in place.

This also combines with the __rcu tag used to mark RCU-protected
pointers, in which case sparse complains when a non-RCU API is applied
to these pointers, to get back to your earlier question about use of
hlist_for_each_entry_rcu() within the update-side lock.

But what are you seeing as actively harmful about all of this?
What should we be doing instead?

If you are suggesting that KCSAN might replace sparse's use of __rcu,
I have been attempting to think along those lines, but thus far without
any success.  In contrast, I am starting to think that lockdep has made
the sparse __acquires() and __releases() tags obsolete.

Or am I missing your point?

							Thanx, Paul