From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 66CC4C433F5 for ; Fri, 11 Feb 2022 17:14:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242158AbiBKROp (ORCPT ); Fri, 11 Feb 2022 12:14:45 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:37508 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345360AbiBKROo (ORCPT ); Fri, 11 Feb 2022 12:14:44 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9FC5A2E8; Fri, 11 Feb 2022 09:14:42 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 51166B82B0E; Fri, 11 Feb 2022 17:14:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ABE74C340E9; Fri, 11 Feb 2022 17:14:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1644599680; bh=rpU93oW9eR2pFieHpUXayDQW9Hf20gDzvibQ0qElxUc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Rzq58ji20rPeZJfecIlKKJ8tp42cn2Ws+5AWURs9tIII010Ko3J2HX3TTmtuRjG4N ywXP7Q/cw0dqE+3/fhJIoTZ3/AM/A52LTfXVXat78d/8DoaHybuvMsWW8+uKwfr+pS yCdONpAw6VSswxauzGY3roWJe/fEt7wRh1vy5Y5NdaSwI94SzHBwvu0Iy4ruTzSRxR bPyFJSWJ7IGbAp7EsMuDAOPR3V4OCASsZjpYM5yEbwpknwZtXtwEFtv3Sh0+2s2fq/ QuoXnrMhqVUwpPeQrNJzvUWCgRZvhOt01+nxfjX8vVmjMb/B4qp3ttyL0e2KTHtU/Q c8hNkhtS/UhKw== Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id 274FF400FE; Fri, 11 Feb 2022 14:14:38 -0300 (-03) Date: Fri, 11 Feb 2022 14:14:38 -0300 From: Arnaldo Carvalho de Melo To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Thomas Gleixner , Darren Hart , Davidlohr Bueso , =?iso-8859-1?Q?Andr=E9?= Almeida , James Clark , John Garry , Riccardo Mancini , Yury Norov , Andy Shevchenko , Andrew Morton , Jin Yao , Adrian Hunter , Leo Yan , Andi Kleen , Thomas Richter , Kan Liang , Madhavan Srinivasan , Shunsuke Nakamura , Song Liu , Masami Hiramatsu , Steven Rostedt , Miaoqian Lin , Stephen Brennan , Kajol Jain , Alexey Bayduraev , German Gomez , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Dumazet , Dmitry Vyukov , Hao Luo , eranian@google.com Subject: Re: [PATCH v3 04/22] perf dso: Hold lock when accessing nsinfo Message-ID: References: <20220211103415.2737789-1-irogers@google.com> <20220211103415.2737789-5-irogers@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220211103415.2737789-5-irogers@google.com> X-Url: http://acmel.wordpress.com Precedence: bulk List-ID: X-Mailing-List: linux-perf-users@vger.kernel.org Em Fri, Feb 11, 2022 at 02:33:57AM -0800, Ian Rogers escreveu: > There may be threads racing to update dso->nsinfo: > https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/ > Holding the dso->lock avoids use-after-free, memory leaks and other > such bugs. Apply the fix in: > https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/ > of there being a missing nsinfo__put now that the accesses are data race > free. I think this is too source code polluting, see previous comment, that would cover this case as well, I think. - Arnaldo > Signed-off-by: Ian Rogers > --- > tools/perf/builtin-inject.c | 4 ++++ > tools/perf/util/dso.c | 5 ++++- > tools/perf/util/map.c | 3 +++ > tools/perf/util/probe-event.c | 2 ++ > tools/perf/util/symbol.c | 2 +- > 5 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c > index fbf43a454cba..bede332bf0e2 100644 > --- a/tools/perf/builtin-inject.c > +++ b/tools/perf/builtin-inject.c > @@ -363,8 +363,10 @@ static struct dso *findnew_dso(int pid, int tid, const char *filename, > } > > if (dso) { > + BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > nsinfo__put(dso->nsinfo); > dso->nsinfo = nsi; > + pthread_mutex_unlock(&dso->lock); > } else > nsinfo__put(nsi); > > @@ -547,7 +549,9 @@ static int dso__read_build_id(struct dso *dso) > if (dso->has_build_id) > return 0; > > + BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > nsinfo__mountns_enter(dso->nsinfo, &nsc); > + pthread_mutex_unlock(&dso->lock); > if (filename__read_build_id(dso->long_name, &dso->bid) > 0) > dso->has_build_id = true; > nsinfo__mountns_exit(&nsc); > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c > index 6beccffeef7b..b2f570adba35 100644 > --- a/tools/perf/util/dso.c > +++ b/tools/perf/util/dso.c > @@ -548,8 +548,11 @@ static int open_dso(struct dso *dso, struct machine *machine) > int fd; > struct nscookie nsc; > > - if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE) > + if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE) { > + BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > nsinfo__mountns_enter(dso->nsinfo, &nsc); > + pthread_mutex_unlock(&dso->lock); > + } > fd = __open_dso(dso, machine); > if (dso->binary_type != DSO_BINARY_TYPE__BUILD_ID_CACHE) > nsinfo__mountns_exit(&nsc); > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c > index 8af693d9678c..ae99b52502d5 100644 > --- a/tools/perf/util/map.c > +++ b/tools/perf/util/map.c > @@ -192,7 +192,10 @@ struct map *map__new(struct machine *machine, u64 start, u64 len, > if (!(prot & PROT_EXEC)) > dso__set_loaded(dso); > } > + BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > + nsinfo__put(dso->nsinfo); > dso->nsinfo = nsi; > + pthread_mutex_unlock(&dso->lock); > > if (build_id__is_defined(bid)) > dso__set_build_id(dso, bid); > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c > index a834918a0a0d..7444e689ece7 100644 > --- a/tools/perf/util/probe-event.c > +++ b/tools/perf/util/probe-event.c > @@ -180,8 +180,10 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user) > > map = dso__new_map(target); > if (map && map->dso) { > + BUG_ON(pthread_mutex_lock(&map->dso->lock) != 0); > nsinfo__put(map->dso->nsinfo); > map->dso->nsinfo = nsinfo__get(nsi); > + pthread_mutex_unlock(&map->dso->lock); > } > return map; > } else { > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > index 43f47532696f..a504346feb05 100644 > --- a/tools/perf/util/symbol.c > +++ b/tools/perf/util/symbol.c > @@ -1774,6 +1774,7 @@ int dso__load(struct dso *dso, struct map *map) > char newmapname[PATH_MAX]; > const char *map_path = dso->long_name; > > + BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > perfmap = strncmp(dso->name, "/tmp/perf-", 10) == 0; > if (perfmap) { > if (dso->nsinfo && (dso__find_perf_map(newmapname, > @@ -1783,7 +1784,6 @@ int dso__load(struct dso *dso, struct map *map) > } > > nsinfo__mountns_enter(dso->nsinfo, &nsc); > - BUG_ON(pthread_mutex_lock(&dso->lock) != 0); > > /* check again under the dso->lock */ > if (dso__loaded(dso)) { > -- > 2.35.1.265.g69c8d7142f-goog -- - Arnaldo