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

#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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web