On Wed, 22 Nov 2023 05:20:58 -0800
JP Kobryn <inwardvessel@xxxxxxxxx> 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@xxxxxxxxxx>
Thank you,
>
> Signed-off-by: JP Kobryn <inwardvessel@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
