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

Groups > linux.kernel > #1616095 > unrolled thread

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

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 Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-04 17:20 +0200
    Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field Namhyung Kim <namhyung@kernel.org> - 2017-04-04 17:40 +0200
      Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-04-04 18:00 +0200
        Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-05 04:50 +0200
          Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-09 19:10 +0200
            Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-10 04:20 +0200
    Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-10 10:50 +0200
      Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field "Du, Changbin" <changbin.du@intel.com> - 2017-04-10 12:30 +0200
        Re: [PATCH v2] perf: fix double free at function  perf_hpp__reset_output_field Jiri Olsa <jolsa@redhat.com> - 2017-04-10 13: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 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

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

Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> From: Changbin Du <changbin.du@intel.com>
> 
> Some perf_hpp_fmt both registered at field and sort list. For such
> instance, we only can free it when removed from the both lists. This
> function currently only used by self-test code, but still should fix
> it.

Looks sane, applying,

Jiri, Namhyung, please holler (or ack) if needed,

- Arnaldo
 
> Signed-off-by: Changbin Du <changbin.du@intel.com>
> ---
> v2: removed redundant Signed-off.
> 
> ---
>  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;
> +			}
> +		}
> +		fmt_free(field_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);
> -		fmt_free(fmt);
> +	/* reset remaining sort keys */
> +	perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
> +		list_del_init(&sort_fmt->sort_list);
> +		fmt_free(sort_fmt);
>  	}
>  }
>  
> -- 
> 2.7.4

[toc] | [next] | [standalone]

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

Hi Arnaldo,

On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
>> From: Changbin Du <changbin.du@intel.com>
>>
>> Some perf_hpp_fmt both registered at field and sort list. For such
>> instance, we only can free it when removed from the both lists. This
>> function currently only used by self-test code, but still should fix
>> it.
>
> Looks sane, applying,
>
> Jiri, Namhyung, please holler (or ack) if needed,

Did you actually see the double free problem?  AFAICS the old code
removed a fmt from both list before free it.  In the first loop, fmt that
was linked to both output list and sort list will be remove.  And the
second loop frees fmt that was linked only to the sort list (IOW, it
frees fmt that was not freed in the first loop).

Thanks,
Namhyung


>
> - Arnaldo
>
>> Signed-off-by: Changbin Du <changbin.du@intel.com>
>> ---
>> v2: removed redundant Signed-off.
>>
>> ---
>>  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;
>> +                     }
>> +             }
>> +             fmt_free(field_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);
>> -             fmt_free(fmt);
>> +     /* reset remaining sort keys */
>> +     perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
>> +             list_del_init(&sort_fmt->sort_list);
>> +             fmt_free(sort_fmt);
>>       }
>>  }
>>
>> --
>> 2.7.4

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

#1616154

Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> >> From: Changbin Du <changbin.du@intel.com>
> >>
> >> Some perf_hpp_fmt both registered at field and sort list. For such
> >> instance, we only can free it when removed from the both lists. This
> >> function currently only used by self-test code, but still should fix
> >> it.
> >
> > Looks sane, applying,
> >
> > Jiri, Namhyung, please holler (or ack) if needed,
> 
> Did you actually see the double free problem?  AFAICS the old code

I assumed that he had seen it, in some self-test code, Changbin, can you
please show command output or further describe when this patch would be
necessary?

- Arnaldo

> removed a fmt from both list before free it.  In the first loop, fmt that
> was linked to both output list and sort list will be remove.  And the
> second loop frees fmt that was linked only to the sort list (IOW, it
> frees fmt that was not freed in the first loop).
> 
> Thanks,
> Namhyung
> 
> 
> >
> > - Arnaldo
> >
> >> Signed-off-by: Changbin Du <changbin.du@intel.com>
> >> ---
> >> v2: removed redundant Signed-off.
> >>
> >> ---
> >>  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;
> >> +                     }
> >> +             }
> >> +             fmt_free(field_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);
> >> -             fmt_free(fmt);
> >> +     /* reset remaining sort keys */
> >> +     perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
> >> +             list_del_init(&sort_fmt->sort_list);
> >> +             fmt_free(sort_fmt);
> >>       }
> >>  }
> >>
> >> --
> >> 2.7.4

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

#1616536

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

On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > Hi Arnaldo,
> > 
> > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > >> From: Changbin Du <changbin.du@intel.com>
> > >>
> > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > >> instance, we only can free it when removed from the both lists. This
> > >> function currently only used by self-test code, but still should fix
> > >> it.
> > >
> > > Looks sane, applying,
> > >
> > > Jiri, Namhyung, please holler (or ack) if needed,
> > 
> > Did you actually see the double free problem?  AFAICS the old code
> 
> I assumed that he had seen it, in some self-test code, Changbin, can you
> please show command output or further describe when this patch would be
> necessary?
> 
Arnaldo, I did observe this issue but not in self-test code. The self-test code
uses that function but does not have a case that a fmt linked to two both list. 
I found this issue when I try to add 'dynamic sort' feature to perf, which
I use this function to reset out fields.

Anyway, it is clear that this is a real bug, a potential issue need to fix.

> - Arnaldo
> 
> > removed a fmt from both list before free it.  In the first loop, fmt that
> > was linked to both output list and sort list will be remove.  And the
> > second loop frees fmt that was linked only to the sort list (IOW, it
> > frees fmt that was not freed in the first loop).
> >
This is right. It is to handle the fmts that linked to both two lists.

> > Thanks,
> > Namhyung
> > 
> > 
> > >
> > > - Arnaldo
> > >

-- 
Thanks,
Changbin Du

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

#1619519

On Wed, Apr 05, 2017 at 10:44:22AM +0800, Du, Changbin wrote:
> On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > > Hi Arnaldo,
> > > 
> > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > > >> From: Changbin Du <changbin.du@intel.com>
> > > >>
> > > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > > >> instance, we only can free it when removed from the both lists. This
> > > >> function currently only used by self-test code, but still should fix
> > > >> it.
> > > >
> > > > Looks sane, applying,
> > > >
> > > > Jiri, Namhyung, please holler (or ack) if needed,
> > > 
> > > Did you actually see the double free problem?  AFAICS the old code
> > 
> > I assumed that he had seen it, in some self-test code, Changbin, can you
> > please show command output or further describe when this patch would be
> > necessary?
> > 
> Arnaldo, I did observe this issue but not in self-test code. The self-test code
> uses that function but does not have a case that a fmt linked to two both list. 
> I found this issue when I try to add 'dynamic sort' feature to perf, which
> I use this function to reset out fields.

could you post it with the rest of your patches? might be easier to review

thanks,
jirka

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

#1619594

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

On Sun, Apr 09, 2017 at 07:05:52PM +0200, Jiri Olsa wrote:
> On Wed, Apr 05, 2017 at 10:44:22AM +0800, Du, Changbin wrote:
> > On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > > > Hi Arnaldo,
> > > > 
> > > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > > > >> From: Changbin Du <changbin.du@intel.com>
> > > > >>
> > > > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > > > >> instance, we only can free it when removed from the both lists. This
> > > > >> function currently only used by self-test code, but still should fix
> > > > >> it.
> > > > >
> > > > > Looks sane, applying,
> > > > >
> > > > > Jiri, Namhyung, please holler (or ack) if needed,
> > > > 
> > > > Did you actually see the double free problem?  AFAICS the old code
> > > 
> > > I assumed that he had seen it, in some self-test code, Changbin, can you
> > > please show command output or further describe when this patch would be
> > > necessary?
> > > 
> > Arnaldo, I did observe this issue but not in self-test code. The self-test code
> > uses that function but does not have a case that a fmt linked to two both list. 
> > I found this issue when I try to add 'dynamic sort' feature to perf, which
> > I use this function to reset out fields.
> 
> could you post it with the rest of your patches? might be easier to review
>
jirka, I am sorry that the 'dynamic sort' is only half done. Now I am
very busy with work at hand. I will send the rest of patches when I
finish them. Could we check out this fix first?

> thanks,
> jirka

-- 
Thanks,
Changbin Du

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

#1619728

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

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

please describe the issue in more details, perhaps we'ew missing
something

jirka

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

#1619790

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

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.

> please describe the issue in more details, perhaps we'ew missing
> something
> 
> jirka

-- 
Thanks,
Changbin Du

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

#1619847

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

jirka

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

#1620854

[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] | [prev] | [next] | [standalone]

#1620981

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

[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

[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

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

[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

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