On Wed, Jun 15, 2022 at 8:34 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > Hi Chuang, > > On Tue, 14 Jun 2022 17:06:33 +0800 > Chuang W <nashuiliang@xxxxxxxxx> wrote: > > > In a scenario where livepatch and aggrprobe coexist, if arm_kprobe() > > returns an error, ap.post_handler, while has been modified to > > p.post_handler, is not rolled back. > > Would you mean 'coexist' on the same function? Yes, It's the same function. > > > > > When ap.post_handler is not NULL (not rolled back), the caller (e.g. > > register_kprobe/enable_kprobe) of arm_kprobe_ftrace() will always fail. > > It seems this explanation and the actual code does not > match. Can you tell me what actually you observed? > > Thank you, > I briefly describe the steps involved, a patch (kprobes: Rollback kprobe flags on failed arm_kprobe, https://lore.kernel.org/all/20220612213156.1323776351ee1be3cabc7fcc@xxxxxxxxxx/T/) must be added, otherwise it will panic: 1) add a livepatch $ insmod livepatch-XXX.ko 2) add a kprobe using tracefs API $ echo 'p:mykprobe XXX' > /sys/kernel/debug/tracing/kprobe_events At this time, XXX is a simple kprobe, kprobe->post_handler = NULL. 3) add a second kprobe using raw kprobe API (i.e. register_kprobe), the new kprobe->post_handler != NULL $ insmod kprobe_XXX.ko $ insmod: ERROR: could not insert module kprobe_XXX.ko: Device or resource busy This will fail (as expected). However, XXX is modified to an aggrprobe. agKprobe->post_handler = aggr_post_handler, it's not rolled back on failed arm_kprobe(). 4) add a third kprobe using bpftrace/bcc tool $ bpftrace -e 'kprobe:XXX {printf("%s", kstack());}' Attaching 1 probe... perf_event_open(/sys/kernel/debug/tracing/events/kprobes/p_XXX_0_1_bcc_440/id): Device or resource busy Error attaching probe: 'kprobe:blkcg_destroy_blkgs' $ bpftrace -e 'kprobe:XXX {printf("%s", kstack());}' Attaching 1 probe... perf_event_open(/sys/kernel/debug/tracing/events/kprobes/p_XXX_0_1_bcc_440/id): Device or resource busy Error attaching probe: 'kprobe:blkcg_destroy_blkgs' This will always fail (not as expected). > > > > Fixes: 12310e343755 ("kprobes: Propagate error from arm_kprobe_ftrace()") > > Signed-off-by: Chuang W <nashuiliang@xxxxxxxxx> > > Cc: <stable@xxxxxxxxxxxxxxx> > > --- > > kernel/kprobes.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index f214f8c088ed..0610b02a3a05 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -1300,6 +1300,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p) > > { > > int ret = 0; > > struct kprobe *ap = orig_p; > > + kprobe_post_handler_t old_post_handler = NULL; > > > > cpus_read_lock(); > > > > @@ -1351,6 +1352,9 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p) > > > > /* Copy the insn slot of 'p' to 'ap'. */ > > copy_kprobe(ap, p); > > + > > + /* save the old post_handler */ > > + old_post_handler = ap->post_handler; > > ret = add_new_kprobe(ap, p); > > > > out: > > @@ -1365,6 +1369,7 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p) > > ret = arm_kprobe(ap); > > if (ret) { > > ap->flags |= KPROBE_FLAG_DISABLED; > > + ap->post_handler = old_post_handler; > > list_del_rcu(&p->list); > > synchronize_rcu(); > > } > > -- > > 2.34.1 > > > > > -- > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
