Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620854 > unrolled thread

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

Started by"Du, Changbin" <changbin.du@intel.com>
First post2017-04-11 05:10 +0200
Last post2017-04-11 12:30 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-11 05:10 +0200
    Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-11 09:40 +0200
      Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-11 10:30 +0200
        Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-11 12:20 +0200
          Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-11 12:40 +0200
            Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-12 04:00 +0200
        Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-11 12:30 +0200

#1620854 — Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

From"Du, Changbin" <changbin.du@intel.com>
Date2017-04-11 05:10 +0200
SubjectRe: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
Message-ID<tuUlQ-1p3-7@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

On Mon, Apr 10, 2017 at 01:33:25PM +0200, Jiri Olsa wrote:
> On Mon, Apr 10, 2017 at 06:21:12PM +0800, Du, Changbin wrote:
> > On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> > > On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > SNIP
> > > 
> > > > > ---
> > > > >  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> > > > >  1 file changed, 15 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > > index 5d632dc..f94b301 100644
> > > > > --- a/tools/perf/ui/hist.c
> > > > > +++ b/tools/perf/ui/hist.c
> > > > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > > > >  
> > > > >  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > > >  {
> > > > > -	struct perf_hpp_fmt *fmt, *tmp;
> > > > > +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > > > >  
> > > > >  	/* reset output fields */
> > > > > -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > > -		list_del_init(&fmt->list);
> > > > > -		list_del_init(&fmt->sort_list);
> > > > > -		fmt_free(fmt);
> > > > > +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > > > +		list_del_init(&field_fmt->list);
> > > > > +		/* reset sort keys */
> > > > > +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > > > > +			if (field_fmt == sort_fmt) {
> > > > > +				list_del_init(&field_fmt->sort_list);
> > > > > +				break;
> > > > > +			}
> > > > > +		}
> > > 
> > > I agree with Namhyung in here.. seems like the only thing you
> > > added is to check if the field_fmt was also linked in as a sort
> > > entry before you call list_del_init on it
> > >
> > This is correct.
> > 
> > > which I think should be also done with list_empty function, but
> > > more importantly I dont see a reason for that.. list_del_init
> > > call should be fine on empty list
> > > 
> > You didn't catch the problem here. The problem is double free a fmt.
> > For exampe, fmt A is linked to both list. Then it will be first free
> > by the first iteration over list, then it will be freed again at the
> > second iteration over sort_list. This must cause application crash.
> 
> the original code takes it out of both lists,
> so the next itaration won't go over that entry
>
oh, my bad, my desc is wrong. I replayed the crash. The problem is
list_del_init a unlinked entry.

perf: Segmentation fault
-------- backtrace --------
./perf[0x57394b]
/lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0]
./perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7]
./perf(hists__sort_by_fields+0x3d7)[0x509777]
./perf[0x5704c1]
./perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5]
./perf(cmd_report+0x1a9b)[0x43b4fb]
./perf[0x494731]
./perf(main+0x704)[0x426304]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830]
./perf(_start+0x29)[0x4263f9]
[0x0]

(gdb) print fmt.list
$4 = {next = 0x100, prev = 0x200}    // LIST_POISON
(gdb) print fmt.sort_list
$5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}

In this case, the fmt is linked in sort_list, but not in list. So crash
at the list_del_init(&fmt->list) of second loop.

Another potential case is the fmt is linked in list, but not in sort_list.

Oh, my brain was broken. correct patch but wrong commit message. :(
Will drop this one and submit a new one.

> jirka

-- 
Thanks,
Changbin Du

[toc] | [next] | [standalone]

#1620981

FromJiri Olsa <jolsa@redhat.com>
Date2017-04-11 09:40 +0200
Message-ID<tuYz7-3XH-3@gated-at.bofh.it>
In reply to#1620854
On Tue, Apr 11, 2017 at 11:06:14AM +0800, Du, Changbin wrote:

SNIP

> > the original code takes it out of both lists,
> > so the next itaration won't go over that entry
> >
> oh, my bad, my desc is wrong. I replayed the crash. The problem is
> list_del_init a unlinked entry.
> 
> perf: Segmentation fault
> -------- backtrace --------
> ./perf[0x57394b]
> /lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0]
> ./perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7]
> ./perf(hists__sort_by_fields+0x3d7)[0x509777]
> ./perf[0x5704c1]
> ./perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5]
> ./perf(cmd_report+0x1a9b)[0x43b4fb]
> ./perf[0x494731]
> ./perf(main+0x704)[0x426304]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830]
> ./perf(_start+0x29)[0x4263f9]
> [0x0]
> 
> (gdb) print fmt.list
> $4 = {next = 0x100, prev = 0x200}    // LIST_POISON
> (gdb) print fmt.sort_list
> $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> 
> In this case, the fmt is linked in sort_list, but not in list. So crash
> at the list_del_init(&fmt->list) of second loop.

so the only place I can see the POISON could get there
is in perf_hpp__column_unregister.. can't we just get
rid of it like below

jirka


---
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dca672a..7577effbf746 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -529,7 +529,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)

[toc] | [prev] | [next] | [standalone]

#1621022

From"Du, Changbin" <changbin.du@intel.com>
Date2017-04-11 10:30 +0200
Message-ID<tuZlw-4vi-7@gated-at.bofh.it>
In reply to#1620981

[Multipart message — attachments visible in raw view] — view raw

> > (gdb) print fmt.sort_list
> > $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> > 
> > In this case, the fmt is linked in sort_list, but not in list. So crash
> > at the list_del_init(&fmt->list) of second loop.
> 
> so the only place I can see the POISON could get there
> is in perf_hpp__column_unregister.. can't we just get
> rid of it like below
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..7577effbf746 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -529,7 +529,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);
>  }
>  
yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
No rule rather than create one.

But anyway, both are ok for me. What's your options?

>  void perf_hpp__cancel_cumulate(void)

-- 
Thanks,
Changbin Du

[toc] | [prev] | [next] | [standalone]

#1621118

From"Du, Changbin" <changbin.du@intel.com>
Date2017-04-11 12:20 +0200
Message-ID<tv13Y-5FJ-5@gated-at.bofh.it>
In reply to#1621022

[Multipart message — attachments visible in raw view] — view raw

> > >  
> > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > No rule rather than create one.
> > 
> > But anyway, both are ok for me. What's your options?
> 
> hum, also I dont think we need to touch that bit at all
> if we are going to remove it right away.. how about the
> change below?
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..0ee7db43dd7d 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -613,15 +613,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);
What if the fmt is not linked to sort_list? I see it is possible (please
checking perf_hpp__setup_output_field()). I am not sure if we really has
sunch case currently, just concern :)

>  	}
>  
>  	/* 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);
>  	}
>  }

-- 
Thanks,
Changbin Du

[toc] | [prev] | [next] | [standalone]

#1621131

FromJiri Olsa <jolsa@redhat.com>
Date2017-04-11 12:40 +0200
Message-ID<tv1nk-5Mx-15@gated-at.bofh.it>
In reply to#1621118
On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > >  
> > > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > > No rule rather than create one.
> > > 
> > > But anyway, both are ok for me. What's your options?
> > 
> > hum, also I dont think we need to touch that bit at all
> > if we are going to remove it right away.. how about the
> > change below?
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 5d632dca672a..0ee7db43dd7d 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -613,15 +613,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);
> What if the fmt is not linked to sort_list? I see it is possible (please
> checking perf_hpp__setup_output_field()). I am not sure if we really has
> sunch case currently, just concern :)

if it's not linked to sort_list, then sort_list is initialized
and list_del should do no harm

jirka

[toc] | [prev] | [next] | [standalone]

#1621743

From"Du, Changbin" <changbin.du@intel.com>
Date2017-04-12 04:00 +0200
Message-ID<tvfJD-6AM-1@gated-at.bofh.it>
In reply to#1621131

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 11, 2017 at 12:32:49PM +0200, Jiri Olsa wrote:
> On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > > >  
> > > > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > > > No rule rather than create one.
> > > > 
> > > > But anyway, both are ok for me. What's your options?
> > > 
> > > hum, also I dont think we need to touch that bit at all
> > > if we are going to remove it right away.. how about the
> > > change below?
> > > 
> > > jirka
> > > 
> > > 
> > > ---
> > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > index 5d632dca672a..0ee7db43dd7d 100644
> > > --- a/tools/perf/ui/hist.c
> > > +++ b/tools/perf/ui/hist.c
> > > @@ -613,15 +613,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);
> > What if the fmt is not linked to sort_list? I see it is possible (please
> > checking perf_hpp__setup_output_field()). I am not sure if we really has
> > sunch case currently, just concern :)
> 
> if it's not linked to sort_list, then sort_list is initialized
> and list_del should do no harm
> 
ok, then it's fine if you insist.

> jirka

-- 
Thanks,
Changbin Du

[toc] | [prev] | [next] | [standalone]

#1621123

FromJiri Olsa <jolsa@redhat.com>
Date2017-04-11 12:30 +0200
Message-ID<tv13Y-5FJ-7@gated-at.bofh.it>
In reply to#1621022
On Tue, Apr 11, 2017 at 04:25:50PM +0800, Du, Changbin wrote:
> > > (gdb) print fmt.sort_list
> > > $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> > > 
> > > In this case, the fmt is linked in sort_list, but not in list. So crash
> > > at the list_del_init(&fmt->list) of second loop.
> > 
> > so the only place I can see the POISON could get there
> > is in perf_hpp__column_unregister.. can't we just get
> > rid of it like below
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 5d632dca672a..7577effbf746 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -529,7 +529,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);
> >  }
> >  
> yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> No rule rather than create one.
> 
> But anyway, both are ok for me. What's your options?

hum, also I dont think we need to touch that bit at all
if we are going to remove it right away.. how about the
change below?

jirka


---
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dca672a..0ee7db43dd7d 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -613,15 +613,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);
 	}
 }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web