Re: [PATCH -tip V2 0/2] kprobes: Fix RCU warning and cleanup

[Paul E. McKenney]

On Mon, Jan 13, 2020 at 10:09:53PM +0900, Masami Hiramatsu wrote:
> Hi Joel,
> 
> On Mon, 13 Jan 2020 12:16:40 +0900
> Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> > > > > 
> > > > > Hi Masami,
> > > > > 
> > > > > I believe I had commented before that I don't agree with this patch:
> > > > > https://lore.kernel.org/lkml/157535318870.16485.6366477974356032624.stgit@devnote2/
> > > > > 
> > > > > The rationale you used is to replace RCU-api with non-RCU api just to avoid
> > > > > warnings. I think a better approach is to use RCU api and pass the optional
> > > > > expression to silence the false-positive warnings by informing the RCU API
> > > > > about the fact that locks are held (similar to what we do for
> > > > > rcu_dereference_protected()). The RCU API will do additional checking
> > > > > (such as making sure preemption is disabled for safe RCU usage etc) as well.
> > > > 
> > > > Yes, that is what I did in [1/2] for get_kprobe().
> > > > Let me clarify the RCU list usage in [2/2].
> > > > 
> > > > With the careful check, other list traversals never be done in non-sleepable
> > > > context, those are always runs with kprobe_mutex held.
> > > > If I correctly understand the Documentation/RCU/listRCU.rst, we should/can use
> > > > non-RCU api for those cases, or do I miss something?
> > > 
> > > Yes, that is fine. However personally I prefer not to mix usage of
> > > list_for_each_entry_rcu() and list_for_each_entry() on the same pointer
> > > (kprobe_table). I think it is more confusing and error prone. Just use
> > > list_for_each_entry_rcu() everywhere and pass the appropriate lockdep
> > > expression, instead of calling lockdep_assert_held() independently. Is this
> > > not doable?
> > 
> > Hmm, but isn't it more confusing that user just take a mutex but
> > no rcu_read_lock() with list_for_each_entry_rcu()? In that case,
> > sometimes it might sleep inside list_for_each_entry_rcu(), I thought
> > that might be more confusing mind model for users...

The correct answer will be different in different situations.
For example, code that might be called either with the mutex held or
within an RCU read-side critical section will definitely need the _rcu()
and the lockdep_is_held().  Code that looks OK to call from within
RCU readers, but must not be (e.g., because it sleeps), will just as
definitely need to avoid _rcu().  (If the lack of _rcu() proves confusing,
maybe list_for_each_entry() needs to grow an optional lockdep expression?)

I am therefore personally OK with either approach, though in confusing
cases a comment might help.

> I meant, do we always need to do something like below?
> 
> {
> 	mutex_lock(&lock);
> 	list_for_each_entry_rcu(list, ..., lockdep_is_held(&lock)) {
> 		...
> 	}
> 	mutex_unlock(&lock);
> }
> 
> BTW, I found another problem on this policy, since we don't have
> list_for_each_*_safe() equivalents for RCU, we can not do a safe
> loop on it. Should we call a find function for each time?

Good point.

RCU readers don't need _safe() because RCU grace periods provide this
for free within RCU read-side critical sections.

So agreed, if you need _safe() on the update side, you would need to
call list_for_each_entry_safe().  If this proves confusing due to RCU
readers, maybe it should grow a lockdep expression?  In the meantime,
lockdep_assert_held() could be used if needed to let people know that
this should not be used in an RCU reader.

Does that work, or am I missing part of the problem?

							Thanx, Paul