* [PATCH 1/2] perf tools: Fix crash in perf_hpp__reset_output_field
@ 2017-06-01 11:17 Jiri Olsa
2017-06-01 11:17 ` [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2017-06-01 11:17 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Ingo Molnar, Peter Zijlstra, Namhyung Kim, David Ahern, Du,
Changbin
Du Changbin reported crash [1] when calling perf_hpp__reset_output_field
after unregistering field via perf_hpp__column_unregister.
This ends up in calling following list_del* sequence on
the same format:
perf_hpp__column_unregister:
list_del(&format->list);
perf_hpp__reset_output_field:
list_del_init(&fmt->list);
where the later list_del_init might touch already
freed formats.
Fixing this by replacing list_del with list_del_init
in perf_hpp__column_unregister.
[1] http://marc.info/?l=linux-kernel&m=149059595826019&w=2
Reported-by: "Du, Changbin" <changbin.du@intel.com>
Link: http://lkml.kernel.org/n/tip-8umo89ntt3kawmfwsivav43t@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/ui/hist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 59addd52d9cd..feb2174ddd1f 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -530,7 +530,7 @@ void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
{
- list_del(&format->list);
+ list_del_init(&format->list);
}
void perf_hpp__cancel_cumulate(void)
--
2.9.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset
2017-06-01 11:17 [PATCH 1/2] perf tools: Fix crash in perf_hpp__reset_output_field Jiri Olsa
@ 2017-06-01 11:17 ` Jiri Olsa
2017-06-01 13:02 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2017-06-01 11:17 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Ingo Molnar, Peter Zijlstra, Namhyung Kim, David Ahern, Du,
Changbin
We only needs to remove the format from the currently
iterated list. The other removals/inits are superfluous
because we free the format in any case.
Link: http://lkml.kernel.org/n/tip-8umo89ntt3kawmfwsivav43t@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/ui/hist.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index feb2174ddd1f..a0fee2ac8599 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -614,15 +614,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
/* reset output fields */
perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
- list_del_init(&fmt->list);
- list_del_init(&fmt->sort_list);
+ list_del(&fmt->list);
+ /* Remove the fmt from next loop processing. */
+ list_del(&fmt->sort_list);
fmt_free(fmt);
}
/* reset sort keys */
perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
- list_del_init(&fmt->list);
- list_del_init(&fmt->sort_list);
+ list_del(&fmt->sort_list);
fmt_free(fmt);
}
}
--
2.9.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset
2017-06-01 11:17 ` [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset Jiri Olsa
@ 2017-06-01 13:02 ` Arnaldo Carvalho de Melo
2017-06-01 13:12 ` Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-06-01 13:02 UTC (permalink / raw)
To: Jiri Olsa
Cc: lkml, Ingo Molnar, Peter Zijlstra, Namhyung Kim, David Ahern, Du,
Changbin
Em Thu, Jun 01, 2017 at 01:17:44PM +0200, Jiri Olsa escreveu:
> We only needs to remove the format from the currently
> iterated list. The other removals/inits are superfluous
> because we free the format in any case.
>
> Link: http://lkml.kernel.org/n/tip-8umo89ntt3kawmfwsivav43t@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/ui/hist.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index feb2174ddd1f..a0fee2ac8599 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -614,15 +614,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
>
> /* reset output fields */
> perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> - list_del_init(&fmt->list);
> - list_del_init(&fmt->sort_list);
> + list_del(&fmt->list);
> + /* Remove the fmt from next loop processing. */
> + list_del(&fmt->sort_list);
Why not just add the comment and leave it as list_del_init(), then, in
fmt_free() -> fmt->free() -> hse_free() (for instance), have:
BUG_ON(!list_empty(&fmt->list));
BUG_ON(!list_empty(&fmt->sort_list));
The patch would be smaller and overall the code would be more robust?
> fmt_free(fmt);
> }
>
> /* reset sort keys */
> perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
> - list_del_init(&fmt->list);
> - list_del_init(&fmt->sort_list);
> + list_del(&fmt->sort_list);
Ditto, just ditch what you said you would do in the cset comment, i.e.
ditch the extra "list_del_init(&fmt->list);" call, no?
- Arnaldo
> fmt_free(fmt);
> }
> }
> --
> 2.9.4
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset
2017-06-01 13:02 ` Arnaldo Carvalho de Melo
@ 2017-06-01 13:12 ` Jiri Olsa
0 siblings, 0 replies; 4+ messages in thread
From: Jiri Olsa @ 2017-06-01 13:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, lkml, Ingo Molnar, Peter Zijlstra, Namhyung Kim,
David Ahern, Du, Changbin
On Thu, Jun 01, 2017 at 10:02:26AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jun 01, 2017 at 01:17:44PM +0200, Jiri Olsa escreveu:
> > We only needs to remove the format from the currently
> > iterated list. The other removals/inits are superfluous
> > because we free the format in any case.
> >
> > Link: http://lkml.kernel.org/n/tip-8umo89ntt3kawmfwsivav43t@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/perf/ui/hist.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index feb2174ddd1f..a0fee2ac8599 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -614,15 +614,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> >
> > /* reset output fields */
> > perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > - list_del_init(&fmt->list);
> > - list_del_init(&fmt->sort_list);
> > + list_del(&fmt->list);
> > + /* Remove the fmt from next loop processing. */
> > + list_del(&fmt->sort_list);
>
> Why not just add the comment and leave it as list_del_init(), then, in
> fmt_free() -> fmt->free() -> hse_free() (for instance), have:
>
> BUG_ON(!list_empty(&fmt->list));
> BUG_ON(!list_empty(&fmt->sort_list));
>
> The patch would be smaller and overall the code would be more robust?
ok, that could catch something.. will change
thanks,
jirka
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-01 13:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-01 11:17 [PATCH 1/2] perf tools: Fix crash in perf_hpp__reset_output_field Jiri Olsa
2017-06-01 11:17 ` [PATCH 2/2] perf tools: Remove extra list_del_init calls in list reset Jiri Olsa
2017-06-01 13:02 ` Arnaldo Carvalho de Melo
2017-06-01 13:12 ` Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.