Re: [PATCH] kprobes: Rollback kprobe flags on failed arm_kprobe

[chuang]

Thanks for your quick reply.
I'm very, very sorry for sending multiple emails. I am submitting a
patch for the first time.

On Fri, Jun 3, 2022 at 11:03 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> This should go to stable, so add below tag. (No need to CC to stable)
>
> Fixes: 12310e343755 ("kprobes: Propagate error from arm_kprobe_ftrace()")
> Cc: stable@xxxxxxxxxxxxxxx

Thanks for your kind reminder.

>
> And could you also update this patch as below?
>
> > ---
> >  kernel/kprobes.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index f214f8c088ed..96c75e23113c 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2422,8 +2422,10 @@ int enable_kprobe(struct kprobe *kp)
> >       if (!kprobes_all_disarmed && kprobe_disabled(p)) {
> >               p->flags &= ~KPROBE_FLAG_DISABLED;
> >               ret = arm_kprobe(p);
> > -             if (ret)
> > +             if (ret) {
> >                       p->flags |= KPROBE_FLAG_DISABLED;
>
> Here, can you add a check?
>
>         if (p != kp)
>
> > +                     kp->flags |= KPROBE_FLAG_DISABLED;

Well, I also thought about it. This already covers conditions `p ==
kp` and `p != kp`:
---
                       kp->flags |= KPROBE_FLAG_DISABLED;
---

When p is equal to kp, `kp->flags` is assigned to KPROBE_FLAG_DISABLED twice.
However, if you add a check that p is not equal to kp, this makes the
code read more clearly. Anything is OK, what is your suggestion?

>
> Thus is is clear that this is corresponding to
> ---
>         if (p != kp)
>                 kp->flags &= ~KPROBE_FLAG_DISABLED;
> ---
>
> Thank you,
>
> > +             }
> >       }
> >  out:
> >       mutex_unlock(&kprobe_mutex);
> > --
> > 2.34.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Have a wonderful day!