stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: chuang <nashuiliang@gmail.com>
Cc: stable@vger.kernel.org,
	"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	Ingo Molnar <mingo@kernel.org>, Jessica Yu <jeyu@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kprobes: Rollback post_handler on failed arm_kprobe()
Date: Sun, 19 Jun 2022 01:04:40 +0900	[thread overview]
Message-ID: <20220619010440.57fe2f296b555df666108559@kernel.org> (raw)
In-Reply-To: <CACueBy7Q6TenVFGau7Y+8nuo9ZLqruC1Pijw1YuMgyOUhjULMA@mail.gmail.com>

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.org/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 <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>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

      reply	other threads:[~2022-06-18 16:04 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-14  9:06 [PATCH] kprobes: Rollback post_handler on failed arm_kprobe() Chuang W
2022-06-15  0:34 ` Masami Hiramatsu
2022-06-15  3:09   ` chuang
2022-06-18 16:04     ` Masami Hiramatsu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220619010440.57fe2f296b555df666108559@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=anil.s.keshavamurthy@intel.com \
    --cc=davem@davemloft.net \
    --cc=jeyu@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=nashuiliang@gmail.com \
    --cc=naveen.n.rao@linux.ibm.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).