Re: [PATCH] kprobes: consistent rcu api usage for kretprobe holder

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 23 Nov 2023 22:26:59 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Wed, 22 Nov 2023 05:20:58 -0800
> JP Kobryn <inwardvessel@gmail.com> wrote:
> 
> > It seems that the pointer-to-kretprobe "rp" within the kretprobe_holder is
> > RCU-managed, based on the (non-rethook) implementation of get_kretprobe().
> > The thought behind this patch is to make use of the RCU API where possible
> > when accessing this pointer so that the needed barriers are always in place
> > and to self-document the code.
> > 
> > The __rcu annotation to "rp" allows for sparse RCU checking. Plain writes
> > done to the "rp" pointer are changed to make use of the RCU macro for
> > assignment. For the single read, the implementation of get_kretprobe()
> > is simplified by making use of an RCU macro which accomplishes the same,
> > but note that the log warning text will be more generic.
> > 
> > I did find that there is a difference in assembly generated between the
> > usage of the RCU macros vs without. For example, on arm64, when using
> > rcu_assign_pointer(), the corresponding store instruction is a
> > store-release (STLR) which has an implicit barrier. When normal assignment
> > is done, a regular store (STR) is found. In the macro case, this seems to
> > be a result of rcu_assign_pointer() using smp_store_release() when the
> > value to write is not NULL.
> 
> Good catch! I think rethook also needs this barrier to access its handler
> field. (unlikely to the kretprobe_holder, rethook->data field is not used
> for checking unregistered)
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

BTW, let me pick this in my tree (probes/fixes).

Thanks,

> 
> Thank you,
> 
> > 
> > Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> > ---
> >  include/linux/kprobes.h | 7 ++-----
> >  kernel/kprobes.c        | 4 ++--
> >  2 files changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> > index ab1da3142b06..64672bace560 100644
> > --- a/include/linux/kprobes.h
> > +++ b/include/linux/kprobes.h
> > @@ -139,7 +139,7 @@ static inline bool kprobe_ftrace(struct kprobe *p)
> >   *
> >   */
> >  struct kretprobe_holder {
> > -	struct kretprobe	*rp;
> > +	struct kretprobe __rcu *rp;
> >  	struct objpool_head	pool;
> >  };
> >  
> > @@ -245,10 +245,7 @@ unsigned long kretprobe_trampoline_handler(struct pt_regs *regs,
> >  
> >  static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
> >  {
> > -	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > -		"Kretprobe is accessed from instance under preemptive context");
> > -
> > -	return READ_ONCE(ri->rph->rp);
> > +	return rcu_dereference_check(ri->rph->rp, rcu_read_lock_any_held());
> >  }
> >  
> >  static nokprobe_inline unsigned long get_kretprobe_retaddr(struct kretprobe_instance *ri)
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 075a632e6c7c..d5a0ee40bf66 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2252,7 +2252,7 @@ int register_kretprobe(struct kretprobe *rp)
> >  		rp->rph = NULL;
> >  		return -ENOMEM;
> >  	}
> > -	rp->rph->rp = rp;
> > +	rcu_assign_pointer(rp->rph->rp, rp);
> >  	rp->nmissed = 0;
> >  	/* Establish function entry probe point */
> >  	ret = register_kprobe(&rp->kp);
> > @@ -2300,7 +2300,7 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
> >  #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> >  		rethook_free(rps[i]->rh);
> >  #else
> > -		rps[i]->rph->rp = NULL;
> > +		rcu_assign_pointer(rps[i]->rph->rp, NULL);
> >  #endif
> >  	}
> >  	mutex_unlock(&kprobe_mutex);
> > -- 
> > 2.42.0
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>