Re: [PATCH] ptrace: Fix error handling in ptrace_hbp_get_initialised_bp

[Junlong li]

Thank you for your detailed explanation.
I misunderstood the meaning of the return value of the function
ptrace_hbp_get_event.


On Thu, Feb 13, 2025 at 8:17 PM Will Deacon <will@xxxxxxxxxx> wrote:
>
> On Wed, Feb 12, 2025 at 07:35:46PM +0800, Junlong li wrote:
> >    From b824aece318ed38666621610af7807e70831f964 Mon Sep 17 00:00:00 2001
> >    From: lijunlong <[1]lijunlong@xxxxxxxxxxxxx>
> >    Date: Wed, 12 Feb 2025 19:15:46 +0800
> >    Subject: [PATCH] ptrace: Fix error handling in
> >    ptrace_hbp_get_initialised_bp
> >
> >    The function ptrace_hbp_get_event() returns ERR_PTR(-EINVAL) on error,
> >    but ptrace_hbp_get_initialised_bp() was checking for NULL instead of
> >    using IS_ERR(). This could lead to incorrect error handling and
> >    potential issues when trying to create a new breakpoint event.
>
> Can you please give an example of how this goes wrong?
>
> >    Change the condition from:
> >        if (!bp)
> >    to:
> >        if (IS_ERR(bp))
> >
> >    This ensures proper error checking and maintains consistency with
> >    the error handling mechanism used by ptrace_hbp_get_event().
> >
> >    Signed-off-by: lijunlong [2]zhuizhuhaomeng@xxxxxxxxx
> >    ---
> >     arch/arm64/kernel/ptrace.c | 2 +-
> >     1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >    diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> >    index 0d022599eb61..3bf549b540b1 100644
> >    --- a/arch/arm64/kernel/ptrace.c
> >    +++ b/arch/arm64/kernel/ptrace.c
> >    @@ -414,7 +414,7 @@ static struct perf_event
> >    *ptrace_hbp_get_initialised_bp(unsigned int note_type,
> >     {
> >            struct perf_event *bp = ptrace_hbp_get_event(note_type, tsk, idx);
> >
> >    -       if (!bp)
> >    +       if (IS_ERR(bp))
> >                    bp = ptrace_hbp_create(note_type, tsk, idx);
>
> I think this change actually causes a problem.
>
> In the current code, ptrace_hbp_get_event() can return:
>
>  - An error if the note type is unknown or the index is out-of-bounds
>  - NULL if the relevant breakpoint has not yet been created
>  - The breakpoint pointer if it exists
>
> So, in the case of getting NULL back, we lazily create the breakpoint.
>
> Is it pretty? Hell no! But I'm not entirely sure it's broken, either.
>
> Will



-- 
-