Message-Id: <20220128142348.17d51894dbdb35c9a9449567@kernel.org>
Date:   Fri, 28 Jan 2022 14:23:48 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Darren Hart <dvhart@...radead.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        André Almeida <andrealmeid@...labora.com>,
        James Clark <james.clark@....com>,
        John Garry <john.garry@...wei.com>,
        Riccardo Mancini <rickyman7@...il.com>,
        Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Leo Yan <leo.yan@...aro.org>, Andi Kleen <ak@...ux.intel.com>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Shunsuke Nakamura <nakamura.shun@...itsu.com>,
        Song Liu <song@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Miaoqian Lin <linmq006@...il.com>,
        Stephen Brennan <stephen.s.brennan@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
        German Gomez <german.gomez@....com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        masami.hiramatsu.pt@...achi.com, eranian@...gle.com
Subject: Re: [PATCH v2 0/4] Reference count checker and related fixes

On Thu, 27 Jan 2022 13:33:23 -0800
Ian Rogers <irogers@...gle.com> wrote:

> On Tue, Jan 25, 2022 at 12:46 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > This v2 patch set has the main reference count patch for cpu map from
> > the first set and then adds reference count checking to nsinfo. The
> > reference count checking on nsinfo helped diagnose a data race bug
> > which is fixed in the independent patches 2 and 3.
> >
> > The perf tool has a class of memory problems where reference counts
> > are used incorrectly. Memory/address sanitizers and valgrind don't
> > provide useful ways to debug these problems, you see a memory leak
> > where the only pertinent information is the original allocation
> > site. What would be more useful is knowing where a get fails to have a
> > corresponding put, where there are double puts, etc.
> >
> > This work was motivated by the roll-back of:
> > https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/
> > where fixing a missed put resulted in a use-after-free in a different
> > context. There was a sense in fixing the issue that a game of
> > wac-a-mole had been embarked upon in adding missed gets and puts.
> >
> > The basic approach of the change is to add a level of indirection at
> > the get and put calls. Get allocates a level of indirection that, if
> > no corresponding put is called, becomes a memory leak (and associated
> > stack trace) that leak sanitizer can report. Similarly if two puts are
> > called for the same get, then a double free can be detected by address
> > sanitizer. This can also detect the use after put, which should also
> > yield a segv without a sanitizer.
> >
> > Adding reference count checking to cpu map was done as a proof of
> > concept, it yielded little other than a location where the use of get
> > could be cleaner by using its result. Reference count checking on
> > nsinfo identified a double free of the indirection layer and the
> > related threads, thereby identifying a data race as discussed here:
> > https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
> > Accordingly the dso->lock was extended and use to cover the race.
> >
> > An alternative that was considered was ref_tracker:
> >  https://lwn.net/Articles/877603/
> > ref_tracker requires use of a reference counted struct to also use a
> > cookie/tracker. The cookie is combined with data in a ref_tracker_dir
> > to spot double puts. When an object is finished with leaks can be
> > detected, as with this approach when leak analysis happens. This
> > approach was preferred as it doesn't introduce cookies, spots use
> > after put and appears moderately more neutral to the API. Weaknesses
> > of the implemented approcah are not being able to do adhoc leak
> > detection and a preference for adding an accessor API to structs. I
> > believe there are other issues and welcome suggestions.
> 
> And so we've been here before (Dec 2015 to be exact). Namhyung pointed me to:
> https://lore.kernel.org/all/20151209021047.10245.8918.stgit@localhost.localdomain/
> by Masami Hiramatsu. In this work he adds a leak sanitizer style
> reference count checker that will describe locations of puts and gets
> for diagnosis. Firstly that's an awesome achievement! This work is
> different in that it isn't trying to invent a leak sanitizer, it is
> just using the existing one. By adding a level of indirection this
> work can catch use after put and pairs gets with puts to make lifetime
> analysis more automatic. An advantage of Masami's work is that it
> doesn't change data-structures and after the initial patch-set is
> somewhat transparent. Overall I prefer the approach in these patches,
> future patches can look to clean up the API as Masami has.

Thanks for referring my series :-D The series aimed to solve the refcount
usage issue in the perf which lead the object leaks. At that moment,
I found that there were 2 patterns, refcount start from 0 and start from 1.
That made me confused what I should do for using a object.
But the perf uses linux/refcount.h now, I hope such issue has already gone.
(but the object leakage seems not fixed fully yet, as you found.)

BTW, I think the introducing UNWRAP_* macro may put a burden on future
development. If it is inevitable, we should consider it as carefully as
possible. Or, it may cause another issue (it is easily missed that the new
patch does not use UNWRAP_* for object reference, because it is natual.)

So I agree with you that you to clean up the API. :-)
I think we can make yet another refcount.h for user space debugging and
replace it with the linux/refcount.h.

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists