Date:   Mon, 26 Jun 2023 10:38:07 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Sasha Levin <sashal@...nel.org>,
        Markus Elfring <Markus.Elfring@....de>,
        linux-perf-users@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Ivan Babrou <ivan@...udflare.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>, cocci@...ia.fr
Subject: Re: [PATCH v2] perf unwind: Fix map reference counts

On Mon, Jun 26, 2023 at 5:42 AM Dan Carpenter <dan.carpenter@...aro.org> wrote:
>
> On Fri, Jun 23, 2023 at 10:49:36AM -0700, Ian Rogers wrote:
> > >
> > > How do you think about to add the tag “Fixes”?
> >
> > In general we've not been adding Fixes as there is a danger a backport
> > will introduce a use-after-free.
>
> I feel like we have been discussing issues around Perf backports
> recently.  Wasn't there some build breakage that wasn't detected?  Why
> not just ask Sasha to leave perf out of the -stable tree?
>
> Also Sasha has a tag to explain that patch AAA is included because
> patch BBB depends on it.  I feel like maybe those tags are backwards,
> it would be nicer to tag AAA as depending on BBB.  That way we could
> add the dependency tags here.
>
> I think at Linaro we have recently been testing taking the latest Perf
> tools and using them on older kernels.  I don't know the details around
> why we can't just use the perf that ships with the kernel...

Using the perf tool that ships with the kernel should be fine but the
perf tool is backward compatible with older kernels. There are new
features, such as using BPF for kernel lock analysis, that are
available in the latest perf tool and so it could be nice to have
these available/tested on older kernels.

> To tell the truth, I also don't really understand the problem for this
> patch specifically. From what I can see, the Fixes tag would have been:
>
> Fixes: 0dd5041c9a0e ("perf addr_location: Add init/exit/copy functions")

No, the issue is fixing a long standing problem in the perf tool where
reference counting has been broken. We have implemented a poor man's
RAII using leak analysis and these patches stem from that, but the
errant code pre-dates it. Fwiw, more details on the reference count
checker is here:
https://perf.wiki.kernel.org/index.php/Reference_Count_Checking

> 1) Adding a Fixes tag would have automatically prevented any backports.
> 2) I don't see any possible use after frees.  That probably means I have
> identified the wrong Fixes tag?

You'd need tests that stress libunwind unwinding, such as "perf top
-g". Prior to the reference count checker work the code contained
unnecessary gets to avoid lowering reference counts and causing
use-after-free (a memory leak considered less bad than crashing). By
backporting the code you are taking part of the fix and potentially
creating a new problem.

Thanks,
Ian

> I'm not going to dig further than that because I don't care.  I'm just
> looking at it because Markus added kernel-janitors to the CC list.  But
> for subsystems where I'm more involved then I always look at how a bug
> is introduced.  That information is essential to me as a reviewer.  So
> if I'm writing a patch and even if it's not a bug fix but let's say it
> deletes dead code then I often include include the information under the
> --- cut off line.
>
> ---
> This dead code was introduced by commit 23423423 ("blah blah blah").
>
> regards,
> dan carpenter

Powered by blists - more mailing lists