| Message ID | 20231122132058.3359-1-inwardvessel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kprobes: consistent rcu api usage for kretprobe holder | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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>
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);
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. Signed-off-by: JP Kobryn <inwardvessel@gmail.com> --- include/linux/kprobes.h | 7 ++----- kernel/kprobes.c | 4 ++-- 2 files changed, 4 insertions(+), 7 deletions(-)