Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753119AbdDKDJT (ORCPT ); Mon, 10 Apr 2017 23:09:19 -0400 Received: from mga11.intel.com ([192.55.52.93]:51688 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752047AbdDKDJS (ORCPT ); Mon, 10 Apr 2017 23:09:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,184,1488873600"; d="asc'?scan'208";a="1133767182" Date: Tue, 11 Apr 2017 11:06:14 +0800 From: "Du, Changbin" To: Jiri Olsa Cc: "Du, Changbin" , Arnaldo Carvalho de Melo , Namhyung Kim , Jiri Olsa , peterz@infradead.org, mingo@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field Message-ID: <20170411030614.GA9155@intel.com> References: <20170315021631.31980-1-changbin.du@intel.com> <20170327062255.27309-1-changbin.du@intel.com> <20170404151940.GD12903@kernel.org> <20170410083950.GD25354@krava> <20170410102111.GA6437@intel.com> <20170410113325.GE25354@krava> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GvXjxJ+pjyke8COw" Content-Disposition: inline In-Reply-To: <20170410113325.GE25354@krava> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4203 Lines: 117 --GvXjxJ+pjyke8COw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wr= ote: > > >=20 > > > SNIP > > >=20 > > > > > --- > > > > > tools/perf/ui/hist.c | 25 +++++++++++++++---------- > > > > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > >=20 > > > > > 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 *f= mt) > > > > > =20 > > > > > 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; > > > > > =20 > > > > > /* 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 =3D=3D sort_fmt) { > > > > > + list_del_init(&field_fmt->sort_list); > > > > > + break; > > > > > + } > > > > > + } > > >=20 > > > 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. > >=20 > > > 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 > > >=20 > > 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. >=20 > 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 -------- =2E/perf[0x57394b] /lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0] =2E/perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7] =2E/perf(hists__sort_by_fields+0x3d7)[0x509777] =2E/perf[0x5704c1] =2E/perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5] =2E/perf(cmd_report+0x1a9b)[0x43b4fb] =2E/perf[0x494731] =2E/perf(main+0x704)[0x426304] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830] =2E/perf(_start+0x29)[0x4263f9] [0x0] (gdb) print fmt.list $4 =3D {next =3D 0x100, prev =3D 0x200} // LIST_POISON (gdb) print fmt.sort_list $5 =3D {next =3D 0x9727d0 , prev =3D 0x9727d0 } 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 --=20 Thanks, Changbin Du --GvXjxJ+pjyke8COw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJY7EgmAAoJEAanuZwLnPNUVGwH/i9QtIp6BBmqEzlLROdgIbqM +uKwdawOmZbNpT6wAFbg5ij2VE3TqcZKM52XAj37VTiOM/+p5MJNKUM/t/dJIiYn 23QgXpe3B+ityQn6b2NDjmVwBpQ4KVRr4Gc6Y6u094KIpmFNkeaN9vIm/ISeFsbo VPV5W3F6xnVkOEZf2m/PwaEbHS1Hg7IlccWMXagZIBfxivlyN5Lfo5Y+EVK70Tp0 H8b4DGWDqUg4++T1B/g2gblII+VyyEdTFSn9l/qgdevCLaeFC5SFRcOkFmyTp50V /JuEzml8XpELNEpo5Vim4d5Copct+ub3qlzBT4Ud10QQnLOYoFtDW0G2nExFf3E= =WulS -----END PGP SIGNATURE----- --GvXjxJ+pjyke8COw--