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.
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.
Fixes: 12310e343755 ("kprobes: Propagate error from arm_kprobe_ftrace()")
Signed-off-by: Chuang W <[email protected]>
Cc: <[email protected]>
---
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
Hi Chuang,
On Tue, 14 Jun 2022 17:06:33 +0800
Chuang W <[email protected]> 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?
>
> 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,
>
> Fixes: 12310e343755 ("kprobes: Propagate error from arm_kprobe_ftrace()")
> Signed-off-by: Chuang W <[email protected]>
> Cc: <[email protected]>
> ---
> 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) <[email protected]>
On Wed, Jun 15, 2022 at 8:34 AM Masami Hiramatsu <[email protected]> wrote:
>
> Hi Chuang,
>
> On Tue, 14 Jun 2022 17:06:33 +0800
> Chuang W <[email protected]> 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/[email protected]/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 <[email protected]>
> > Cc: <[email protected]>
> > ---
> > 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) <[email protected]>
On Wed, 15 Jun 2022 11:09:16 +0800
chuang <[email protected]> wrote:
> On Wed, Jun 15, 2022 at 8:34 AM Masami Hiramatsu <[email protected]> wrote:
> >
> > Hi Chuang,
> >
> > On Tue, 14 Jun 2022 17:06:33 +0800
> > Chuang W <[email protected]> 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/[email protected]/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
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 <[email protected]>
> > > Cc: <[email protected]>
> > > ---
> > > 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) <[email protected]>
--
Masami Hiramatsu (Google) <[email protected]>