Re: [PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check optimizing logic

[Yang Jihong]

Hello Masami,

On 2023/2/15 23:08, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:29 +0800
> Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:
>
> > Since the following commit:
> >
> >    commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
> >
> > modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
> > may be in the optimizing or unoptimizing state when op.kp->flags
> > has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
> >
> > The __recover_optprobed_insn check logic is incorrect, a kprobe in the
> > unoptimizing state may be incorrectly determined as unoptimizing.
> > As a result, incorrect instructions are copied.
>
> Ah, good catch!
>
> > The optprobe_queued_unopt function needs to be exported for invoking in
> > arch directory.
> >
> > Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>
> Cc: stable@xxxxxxxxxxxxxxx

OK, will add in next version.

> > Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx>
> > ---
> >   arch/x86/kernel/kprobes/opt.c | 4 ++--
> >   include/linux/kprobes.h       | 1 +
> >   kernel/kprobes.c              | 2 +-
> >   3 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
> > index e57e07b0edb6..3718d6863555 100644
> > --- a/arch/x86/kernel/kprobes/opt.c
> > +++ b/arch/x86/kernel/kprobes/opt.c
> > @@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
> >   		/* This function only handles jump-optimized kprobe */
> >   		if (kp && kprobe_optimized(kp)) {
> >   			op = container_of(kp, struct optimized_kprobe, kp);
> > -			/* If op->list is not empty, op is under optimizing */
> > -			if (list_empty(&op->list))
> > +			/* If op is [un]optimized or under unoptimizing */
>
> Hmm, this is a bit confusing comment. If it is unoptimized, the kprobe_optimized() returns false.
> Thus the comment should be /* If op is optimized or under unoptimizing */.

OK, will fix in next version.

Thanks,
Yang.