linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ian Rogers <irogers@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>,
	linux-perf-users <linux-perf-users@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH] libperf evsel: Make use of FD robust.
Date: Thu, 19 Aug 2021 16:30:16 -0700	[thread overview]
Message-ID: <CAM9d7cgBayQFwqW-=3sMYUOTuCQcWYCVy+P9J0bWJOohAn5gAA@mail.gmail.com> (raw)
In-Reply-To: <YR6pYoyov8ZGbAvJ@kernel.org>

Hi Ian,

On Thu, Aug 19, 2021 at 11:56 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Aug 18, 2021 at 10:47:07PM -0700, Ian Rogers escreveu:
> > FD uses xyarray__entry that may return NULL if an index is out of
> > bounds. If NULL is returned then a segv happens as FD unconditionally
> > dereferences the pointer. This was happening in a case of with perf
> > iostat as shown below. The fix is to make FD an "int*" rather than an
> > int and handle the NULL case as either invalid input or a closed fd.
> >
> > $ sudo gdb --args perf stat --iostat  list
> > ...
> > Breakpoint 1, perf_evsel__alloc_fd (evsel=0x5555560951a0, ncpus=1, nthreads=1) at evsel.c:50
> > 50      {
> > (gdb) bt
> >  #0  perf_evsel__alloc_fd (evsel=0x5555560951a0, ncpus=1, nthreads=1) at evsel.c:50
> >  #1  0x000055555585c188 in evsel__open_cpu (evsel=0x5555560951a0, cpus=0x555556093410,
> >     threads=0x555556086fb0, start_cpu=0, end_cpu=1) at util/evsel.c:1792
> >  #2  0x000055555585cfb2 in evsel__open (evsel=0x5555560951a0, cpus=0x0, threads=0x555556086fb0)
> >     at util/evsel.c:2045
> >  #3  0x000055555585d0db in evsel__open_per_thread (evsel=0x5555560951a0, threads=0x555556086fb0)
> >     at util/evsel.c:2065
> >  #4  0x00005555558ece64 in create_perf_stat_counter (evsel=0x5555560951a0,
> >     config=0x555555c34700 <stat_config>, target=0x555555c2f1c0 <target>, cpu=0) at util/stat.c:590
> >  #5  0x000055555578e927 in __run_perf_stat (argc=1, argv=0x7fffffffe4a0, run_idx=0)
> >     at builtin-stat.c:833
> >  #6  0x000055555578f3c6 in run_perf_stat (argc=1, argv=0x7fffffffe4a0, run_idx=0)
> >     at builtin-stat.c:1048
> >  #7  0x0000555555792ee5 in cmd_stat (argc=1, argv=0x7fffffffe4a0) at builtin-stat.c:2534
> >  #8  0x0000555555835ed3 in run_builtin (p=0x555555c3f540 <commands+288>, argc=3,
> >     argv=0x7fffffffe4a0) at perf.c:313
> >  #9  0x0000555555836154 in handle_internal_command (argc=3, argv=0x7fffffffe4a0) at perf.c:365
> >  #10 0x000055555583629f in run_argv (argcp=0x7fffffffe2ec, argv=0x7fffffffe2e0) at perf.c:409
> >  #11 0x0000555555836692 in main (argc=3, argv=0x7fffffffe4a0) at perf.c:539

This callstack looks strange that 'perf iostat list' should not call
run_perf_stat() for the IOSTAT_LIST mode.

Hmm.. maybe it's because the --iostat option is declared
with OPT_CALLBACK_OPTARG which requires the option
to be specified like '--iostat=list' (not '--iostat list').

Anyway it should not crash..

Thanks,
Namhyung


> > ...
> > (gdb) c
> > Continuing.
> > Error:
> > The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (uncore_iio_0/event=0x83,umask=0x04,ch_mask=0xF,fc_mask=0x07/).
> > /bin/dmesg | grep -i perf may provide additional information.
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00005555559b03ea in perf_evsel__close_fd_cpu (evsel=0x5555560951a0, cpu=1) at evsel.c:166
> > 166                     if (FD(evsel, cpu, thread) >= 0)
>
> Humm
>
> static void perf_evsel__close_fd_cpu(struct perf_evsel *evsel, int cpu)
> {
>         int thread;
>
>         for (thread = 0; thread < xyarray__max_y(evsel->fd); ++thread) {
>                 if (FD(evsel, cpu, thread) >= 0)
>                         close(FD(evsel, cpu, thread));
>                 FD(evsel, cpu, thread) = -1;
>         }
> }
>
> void perf_evsel__close_fd(struct perf_evsel *evsel)
> {
>         int cpu;
>
>         for (cpu = 0; cpu < xyarray__max_x(evsel->fd); cpu++)
>                 perf_evsel__close_fd_cpu(evsel, cpu);
> }
>
> Isn't bounds checking being performed by the callers?
>
> - Arnaldo
>

  reply	other threads:[~2021-08-19 23:30 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-19  5:47 [PATCH] libperf evsel: Make use of FD robust Ian Rogers
2021-08-19 18:56 ` Arnaldo Carvalho de Melo
2021-08-19 23:30   ` Namhyung Kim [this message]
2021-08-20  0:13     ` Ian Rogers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM9d7cgBayQFwqW-=3sMYUOTuCQcWYCVy+P9J0bWJOohAn5gAA@mail.gmail.com' \
    --to=namhyung@kernel.org \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=eranian@google.com \
    --cc=irogers@google.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).