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

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

[Fushuai Wang]

The alloc_syscall_stats() function always returns an error pointer
(ERR_PTR) on failure. So replace NULL check with IS_ERR() check
after calling alloc_syscall_stats() function.

Fixes: fc00897c8a3f ("perf trace: Add --summary-mode option")
Signed-off-by: Fushuai Wang <wangfushuai@baidu.com>
---
 tools/perf/builtin-trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index fe737b3ac6e6..25c41b89f8ab 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4440,7 +4440,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 
 	if (trace->summary_mode == SUMMARY__BY_TOTAL && !trace->summary_bpf) {
 		trace->syscall_stats = alloc_syscall_stats();
-		if (trace->syscall_stats == NULL)
+		if (IS_ERR(trace->syscall_stats))
 			goto out_delete_evlist;
 	}
 
@@ -4748,7 +4748,7 @@ static int trace__replay(struct trace *trace)
 
 	if (trace->summary_mode == SUMMARY__BY_TOTAL) {
 		trace->syscall_stats = alloc_syscall_stats();
-		if (trace->syscall_stats == NULL)
+		if (IS_ERR(trace->syscall_stats))
 			goto out;
 	}
 
-- 
2.36.1

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

[Ian Rogers]

On Wed, Sep 17, 2025 at 2:54 AM Fushuai Wang <wangfushuai@baidu.com> wrote:
>
> The alloc_syscall_stats() function always returns an error pointer
> (ERR_PTR) on failure. So replace NULL check with IS_ERR() check
> after calling alloc_syscall_stats() function.
>
> Fixes: fc00897c8a3f ("perf trace: Add --summary-mode option")
> Signed-off-by: Fushuai Wang <wangfushuai@baidu.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-trace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index fe737b3ac6e6..25c41b89f8ab 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4440,7 +4440,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>
>         if (trace->summary_mode == SUMMARY__BY_TOTAL && !trace->summary_bpf) {
>                 trace->syscall_stats = alloc_syscall_stats();
> -               if (trace->syscall_stats == NULL)
> +               if (IS_ERR(trace->syscall_stats))
>                         goto out_delete_evlist;
>         }
>
> @@ -4748,7 +4748,7 @@ static int trace__replay(struct trace *trace)
>
>         if (trace->summary_mode == SUMMARY__BY_TOTAL) {
>                 trace->syscall_stats = alloc_syscall_stats();
> -               if (trace->syscall_stats == NULL)
> +               if (IS_ERR(trace->syscall_stats))
>                         goto out;
>         }
>
> --
> 2.36.1
>

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

[Arnaldo Carvalho de Melo]

On Wed, Sep 17, 2025 at 08:31:02AM -0700, Ian Rogers wrote:
> On Wed, Sep 17, 2025 at 2:54 AM Fushuai Wang <wangfushuai@baidu.com> wrote:
> >
> > The alloc_syscall_stats() function always returns an error pointer
> > (ERR_PTR) on failure. So replace NULL check with IS_ERR() check
> > after calling alloc_syscall_stats() function.
> >
> > Fixes: fc00897c8a3f ("perf trace: Add --summary-mode option")
> > Signed-off-by: Fushuai Wang <wangfushuai@baidu.com>
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks, applied to perf-tools-next,

- Arnaldo

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

[wangguangju]

From: wangguangju <wangguangju@hygon.cn>

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.

Signed-off-by: wangguangju <wangguangju@hygon.cn>
---
 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

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

[Howard Chu]

Hi wangguangju,

On Thu, Feb 26, 2026 at 4:23 AM <wangguangju@hygon.cn> wrote:
>
> From: wangguangju <wangguangju@hygon.cn>
>
> 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@hygon.cn>

Reviewed-by: Howard Chu <howardchu95@gmail.com>

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
>
>
>

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@gmail.com> wrote:
>
> Hi wangguangju,
>
> On Thu, Feb 26, 2026 at 4:23 AM <wangguangju@hygon.cn> wrote:
> >
> > From: wangguangju <wangguangju@hygon.cn>
> >
> > 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@hygon.cn>
>
> Reviewed-by: Howard Chu <howardchu95@gmail.com>

Acked-by: Ian Rogers <irogers@google.com>

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
> >
> >
> >

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

[Namhyung Kim]

On Thu, 26 Feb 2026 20:22:08 +0800, wangguangju@hygon.cn wrote:
> 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.
> 
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung

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

[Ian Rogers]

On Fri, Feb 27, 2026 at 1:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, 26 Feb 2026 20:22:08 +0800, wangguangju@hygon.cn wrote:
> > 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.
> >
> >
> > [...]
> Applied to perf-tools-next, thanks!

This broke the "perf trace summary" test for me (run `perf test -v`).
The failing command under gdb shows:
```
$ gdb --args perf trace -s -- true
...
(gdb) r
...
Program received signal SIGSEGV, Segmentation fault.
0x0000555555672210 in delete_syscall_stats (syscall_stats=0x0) at
builtin-trace.c:1579
1579            hashmap__for_each_entry(syscall_stats, pos, bkt)
(gdb) l
1574            size_t bkt;
1575
1576            if (IS_ERR(syscall_stats))
1577                    return;
1578
1579            hashmap__for_each_entry(syscall_stats, pos, bkt)
1580                    zfree(&pos->pvalue);
1581            hashmap__free(syscall_stats);
1582    }
```

As NULL is false for IS_ERR I think the correct change is:
```
if (syscall_stats == NULL || IS_ERR(syscall_stats))
```

We seem to be routinely breaking tests, which may indicate a lot of
noise in the test output. I see many failures in the type profiling
test due to the typedef vs struct issue.

Thanks,
Ian

> Best regards,
> Namhyung
>
>

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

[Howard Chu]

Hi Ian,

On Fri, Feb 27, 2026 at 10:40 PM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Feb 27, 2026 at 1:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, 26 Feb 2026 20:22:08 +0800, wangguangju@hygon.cn wrote:
> > > 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.
> > >
> > >
> > > [...]
> > Applied to perf-tools-next, thanks!
>
> This broke the "perf trace summary" test for me (run `perf test -v`).
> The failing command under gdb shows:
> ```
> $ gdb --args perf trace -s -- true
> ...
> (gdb) r
> ...
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000555555672210 in delete_syscall_stats (syscall_stats=0x0) at
> builtin-trace.c:1579
> 1579            hashmap__for_each_entry(syscall_stats, pos, bkt)
> (gdb) l
> 1574            size_t bkt;
> 1575
> 1576            if (IS_ERR(syscall_stats))
> 1577                    return;
> 1578
> 1579            hashmap__for_each_entry(syscall_stats, pos, bkt)
> 1580                    zfree(&pos->pvalue);
> 1581            hashmap__free(syscall_stats);
> 1582    }
> ```
>
> As NULL is false for IS_ERR I think the correct change is:
> ```
> if (syscall_stats == NULL || IS_ERR(syscall_stats))
> ```

I suspected the same thing but couldn't find a NULL assignment in the
code, so I rushed the review, I apologize. Yours looks good to me.

Thanks,
Howard

>
> We seem to be routinely breaking tests, which may indicate a lot of
> noise in the test output. I see many failures in the type profiling
> test due to the typedef vs struct issue.
>
> Thanks,
> Ian
>
> > Best regards,
> > Namhyung
> >
> >
>