Re: [PATCH] perf trace: Fix IS_ERR() vs NULL check bug

[Ian Rogers]

On Thu, Feb 26, 2026 at 7:50 AM Howard Chu <howardchu95@xxxxxxxxx> wrote:
>
> Hi wangguangju,
>
> On Thu, Feb 26, 2026 at 4:23 AM <wangguangju@xxxxxxxx> wrote:
> >
> > From: wangguangju <wangguangju@xxxxxxxx>
> >
> > The alloc_syscall_stats() function always returns an error pointer
> > (ERR_PTR) on failure.
> >
> > So replace NULL check with IS_ERR() check after calling
> > delete_syscall_stats() function.
>
> Thanks, syscall_stats is never NULL after alloc_syscall_stats(), and
> it seems like perf now frees the hashmap using the error pointer,
> pretty dangerous.
>
> >
> > Signed-off-by: wangguangju <wangguangju@xxxxxxxx>
>
> Reviewed-by: Howard Chu <howardchu95@xxxxxxxxx>

Acked-by: Ian Rogers <irogers@xxxxxxxxxx>

We've been burnt by IS_ERR many times, I think in user land we should
declare it an anti-pattern and switch to using NULL and errno.
Alternatively we could wrap functions that return an error pointer by
returning some kind of struct, so the error state is checked. I
reached out to other kernel developers; they agree but the problem in
the kernel is too large to fix. In perf 90% of the problems originate
from hashmap. Alternatively, a sparse/Coccinelle build bot or
build-test that can flag the issue would be good.

Thanks,
Ian

> Thanks,
> Howard
>
> > ---
> >  tools/perf/builtin-trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 311d9da9896a..295b272c6c29 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1573,7 +1573,7 @@ static void delete_syscall_stats(struct hashmap *syscall_stats)
> >         struct hashmap_entry *pos;
> >         size_t bkt;
> >
> > -       if (syscall_stats == NULL)
> > +       if (IS_ERR(syscall_stats))
> >                 return;
> >
> >         hashmap__for_each_entry(syscall_stats, pos, bkt)
> > --
> > 2.43.0
> >
> >
> >