On Wed, 15 Jun 2022 11:09:16 +0800 chuang nashuiliang@gmail.com wrote:

On Wed, Jun 15, 2022 at 8:34 AM Masami Hiramatsu mhiramat@kernel.org wrote:

...

Hi Chuang,

On Tue, 14 Jun 2022 17:06:33 +0800 Chuang W nashuiliang@gmail.com 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@kernel.o...) 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

Ah, OK. In this case, "p->post_handler != NULL" indicates this kprobe will modify regs->ip. Thus the ftrace will conflict with livepatch. In this case, if ap->post_handler is not rolled back, the ap will never be enabled.

Can you update the patch description something like below?

----- In a scenario where livepatch and a aggrprobe coexist on the same function entry, and if the aggrprobe has a post_handler, the arm_kprobe() always fail because both of livepatch and the aggrprobe with post_handler will use FTRACE_OPS_FL_IPMODIFY. This flag is not allowed to be used by the different ftrace user on the same function entry. Since the register_aggr_kprobe() doesn't roll back the post_handler when the arm_kprobe() is failed, this aggrprobe will not be available from now on even if all kprobes on the aggrprobe don't have the post_handler.

Fix to roll back the post_handler of the aggrprobe for this case. With this fix, if the kprobe which has the post_handler is removed from the aggrprobe (since arm_kprobe() failed), it will be available again. -----

This will explains the technical background, what will happen with current code, how it is fixed and what is the corrected behavior.

Thank you,

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@gmail.com Cc: stable@vger.kernel.org

---

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@kernel.org