Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220602073259.25669-1-nashuiliang@gmail.com> <20220604000312.69f42c92e932152da256c2bb@kernel.org>
In-Reply-To: <20220604000312.69f42c92e932152da256c2bb@kernel.org>
From:   chuang <nashuiliang@gmail.com>
Date:   Sat, 4 Jun 2022 10:16:52 +0800
Message-ID: <CACueBy42VUp=4-nMRfAERmLxEwXkc0tGqWQk935oqLeJ51OU5A@mail.gmail.com>
Subject: Re: [PATCH] kprobes: Rollback kprobe flags on failed arm_kprobe
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     Jingren Zhou <zhoujingren@didiglobal.com>,
        "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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

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

Have a wonderful day!