* [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging
@ 2019-06-17 9:11 Leo Yan
2019-06-17 9:11 ` [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info() Leo Yan
2019-06-17 15:24 ` [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Arnaldo Carvalho de Melo
0 siblings, 2 replies; 6+ messages in thread
From: Leo Yan @ 2019-06-17 9:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, linux-kernel, netdev,
bpf
Cc: Leo Yan
In the function trace__syscall_info(), it explicitly checks verbose
level and print out log with fprintf(). Actually, we can use
pr_debug() to do the same thing for debug logging.
This patch uses pr_debug() instead of fprintf() for debug logging; it
includes a minor fixing for 'space before tab in indent', which
dismisses git warning when apply it.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/builtin-trace.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index bd1f00e7a2eb..5cd74651db4c 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1760,12 +1760,11 @@ static struct syscall *trace__syscall_info(struct trace *trace,
* grep "NR -1 " /t/trace_pipe
*
* After generating some load on the machine.
- */
- if (verbose > 1) {
- static u64 n;
- fprintf(trace->output, "Invalid syscall %d id, skipping (%s, %" PRIu64 ") ...\n",
- id, perf_evsel__name(evsel), ++n);
- }
+ */
+ static u64 n;
+
+ pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
+ id, perf_evsel__name(evsel), ++n);
return NULL;
}
@@ -1779,12 +1778,10 @@ static struct syscall *trace__syscall_info(struct trace *trace,
return &trace->syscalls.table[id];
out_cant_read:
- if (verbose > 0) {
- fprintf(trace->output, "Problems reading syscall %d", id);
- if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
- fprintf(trace->output, "(%s)", trace->syscalls.table[id].name);
- fputs(" information\n", trace->output);
- }
+ pr_debug("Problems reading syscall %d", id);
+ if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
+ pr_debug("(%s)", trace->syscalls.table[id].name);
+ pr_debug(" information\n");
return NULL;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info()
2019-06-17 9:11 [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Leo Yan
@ 2019-06-17 9:11 ` Leo Yan
2019-06-17 17:32 ` Arnaldo Carvalho de Melo
2019-06-17 15:24 ` [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Arnaldo Carvalho de Melo
1 sibling, 1 reply; 6+ messages in thread
From: Leo Yan @ 2019-06-17 9:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, linux-kernel, netdev,
bpf
Cc: Leo Yan
trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
retrieve system calls information, it always passes NULL for 'evsel'
argument; when id is an invalid value then the logging will try to
output event name, this triggers NULL pointer dereference.
This patch directly uses string "unknown" for event name when 'evsel'
is NULL pointer.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
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 5cd74651db4c..49dfb2fd393b 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
static u64 n;
pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
- id, perf_evsel__name(evsel), ++n);
+ id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
return NULL;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging
2019-06-17 9:11 [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Leo Yan
2019-06-17 9:11 ` [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info() Leo Yan
@ 2019-06-17 15:24 ` Arnaldo Carvalho de Melo
2019-06-18 6:24 ` Leo Yan
1 sibling, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-06-17 15:24 UTC (permalink / raw)
To: Leo Yan
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
Em Mon, Jun 17, 2019 at 05:11:39PM +0800, Leo Yan escreveu:
> In the function trace__syscall_info(), it explicitly checks verbose
> level and print out log with fprintf(). Actually, we can use
> pr_debug() to do the same thing for debug logging.
>
> This patch uses pr_debug() instead of fprintf() for debug logging; it
> includes a minor fixing for 'space before tab in indent', which
> dismisses git warning when apply it.
But those are not fprintf(stdout,), they explicitely redirect to the
output file that the user may have specified using 'perf trace --output
filename.trace' :-)
- Arnaldo
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-trace.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index bd1f00e7a2eb..5cd74651db4c 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1760,12 +1760,11 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> * grep "NR -1 " /t/trace_pipe
> *
> * After generating some load on the machine.
> - */
> - if (verbose > 1) {
> - static u64 n;
> - fprintf(trace->output, "Invalid syscall %d id, skipping (%s, %" PRIu64 ") ...\n",
> - id, perf_evsel__name(evsel), ++n);
> - }
> + */
> + static u64 n;
> +
> + pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> + id, perf_evsel__name(evsel), ++n);
> return NULL;
> }
>
> @@ -1779,12 +1778,10 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> return &trace->syscalls.table[id];
>
> out_cant_read:
> - if (verbose > 0) {
> - fprintf(trace->output, "Problems reading syscall %d", id);
> - if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> - fprintf(trace->output, "(%s)", trace->syscalls.table[id].name);
> - fputs(" information\n", trace->output);
> - }
> + pr_debug("Problems reading syscall %d", id);
> + if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> + pr_debug("(%s)", trace->syscalls.table[id].name);
> + pr_debug(" information\n");
> return NULL;
> }
>
> --
> 2.17.1
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info()
2019-06-17 9:11 ` [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info() Leo Yan
@ 2019-06-17 17:32 ` Arnaldo Carvalho de Melo
2019-06-18 6:39 ` Leo Yan
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-06-17 17:32 UTC (permalink / raw)
To: Leo Yan
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
Em Mon, Jun 17, 2019 at 05:11:40PM +0800, Leo Yan escreveu:
> trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
> retrieve system calls information, it always passes NULL for 'evsel'
> argument; when id is an invalid value then the logging will try to
> output event name, this triggers NULL pointer dereference.
>
> This patch directly uses string "unknown" for event name when 'evsel'
> is NULL pointer.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> 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 5cd74651db4c..49dfb2fd393b 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> static u64 n;
>
> pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> - id, perf_evsel__name(evsel), ++n);
> + id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
> return NULL;
What do you think of this instead?
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 68beef8f47ff..1d6af95b9207 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -590,6 +590,9 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
{
char bf[128];
+ if (!evsel)
+ goto out_unknown;
+
if (evsel->name)
return evsel->name;
@@ -629,7 +632,10 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
evsel->name = strdup(bf);
- return evsel->name ?: "unknown";
+ if (evsel->name)
+ return evsel->name;
+out_unknown:
+ return "unknown";
}
const char *perf_evsel__group_name(struct perf_evsel *evsel)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging
2019-06-17 15:24 ` [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Arnaldo Carvalho de Melo
@ 2019-06-18 6:24 ` Leo Yan
0 siblings, 0 replies; 6+ messages in thread
From: Leo Yan @ 2019-06-18 6:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
On Mon, Jun 17, 2019 at 12:24:12PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 17, 2019 at 05:11:39PM +0800, Leo Yan escreveu:
> > In the function trace__syscall_info(), it explicitly checks verbose
> > level and print out log with fprintf(). Actually, we can use
> > pr_debug() to do the same thing for debug logging.
> >
> > This patch uses pr_debug() instead of fprintf() for debug logging; it
> > includes a minor fixing for 'space before tab in indent', which
> > dismisses git warning when apply it.
>
> But those are not fprintf(stdout,), they explicitely redirect to the
> output file that the user may have specified using 'perf trace --output
> filename.trace' :-)
Thanks for pointing out, sorry for noise. Please drop this patch.
Thanks,
Leo Yan
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/builtin-trace.c | 21 +++++++++------------
> > 1 file changed, 9 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index bd1f00e7a2eb..5cd74651db4c 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1760,12 +1760,11 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > * grep "NR -1 " /t/trace_pipe
> > *
> > * After generating some load on the machine.
> > - */
> > - if (verbose > 1) {
> > - static u64 n;
> > - fprintf(trace->output, "Invalid syscall %d id, skipping (%s, %" PRIu64 ") ...\n",
> > - id, perf_evsel__name(evsel), ++n);
> > - }
> > + */
> > + static u64 n;
> > +
> > + pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> > + id, perf_evsel__name(evsel), ++n);
> > return NULL;
> > }
> >
> > @@ -1779,12 +1778,10 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > return &trace->syscalls.table[id];
> >
> > out_cant_read:
> > - if (verbose > 0) {
> > - fprintf(trace->output, "Problems reading syscall %d", id);
> > - if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> > - fprintf(trace->output, "(%s)", trace->syscalls.table[id].name);
> > - fputs(" information\n", trace->output);
> > - }
> > + pr_debug("Problems reading syscall %d", id);
> > + if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> > + pr_debug("(%s)", trace->syscalls.table[id].name);
> > + pr_debug(" information\n");
> > return NULL;
> > }
> >
> > --
> > 2.17.1
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info()
2019-06-17 17:32 ` Arnaldo Carvalho de Melo
@ 2019-06-18 6:39 ` Leo Yan
0 siblings, 0 replies; 6+ messages in thread
From: Leo Yan @ 2019-06-18 6:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
On Mon, Jun 17, 2019 at 02:32:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 17, 2019 at 05:11:40PM +0800, Leo Yan escreveu:
> > trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
> > retrieve system calls information, it always passes NULL for 'evsel'
> > argument; when id is an invalid value then the logging will try to
> > output event name, this triggers NULL pointer dereference.
> >
> > This patch directly uses string "unknown" for event name when 'evsel'
> > is NULL pointer.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > 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 5cd74651db4c..49dfb2fd393b 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > static u64 n;
> >
> > pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> > - id, perf_evsel__name(evsel), ++n);
> > + id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
> > return NULL;
>
> What do you think of this instead?
Yes, I agree the below change is right thing to do. FWIW:
Reviewed-by: Leo Yan <leo.yan@linaro.org>
BTW, my patch followed the code in [1], after apply below your change,
could consider to simplify code in [1] for without checking 'evsel' is
NULL pointer anymore.
Thanks,
Leo
[1] https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/builtin-report.c?h=perf/core#n301
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 68beef8f47ff..1d6af95b9207 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -590,6 +590,9 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
> {
> char bf[128];
>
> + if (!evsel)
> + goto out_unknown;
> +
> if (evsel->name)
> return evsel->name;
>
> @@ -629,7 +632,10 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
>
> evsel->name = strdup(bf);
>
> - return evsel->name ?: "unknown";
> + if (evsel->name)
> + return evsel->name;
> +out_unknown:
> + return "unknown";
> }
>
> const char *perf_evsel__group_name(struct perf_evsel *evsel)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-18 6:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 9:11 [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Leo Yan
2019-06-17 9:11 ` [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info() Leo Yan
2019-06-17 17:32 ` Arnaldo Carvalho de Melo
2019-06-18 6:39 ` Leo Yan
2019-06-17 15:24 ` [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging Arnaldo Carvalho de Melo
2019-06-18 6:24 ` Leo Yan
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).