On Thu, 23 Nov 2023 22:26:59 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> 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>
BTW, let me pick this in my tree (probes/fixes).
Thanks,
>
> 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>
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
