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

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

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>

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>