linux-kernel - Re: [PATCH v5 00/17] Reference count checker and related fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Apr 2023 11:47:26 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Ian Rogers <irogers@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Darren Hart <dvhart@...radead.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        James Clark <james.clark@....com>,
        John Garry <john.g.garry@...cle.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>,
        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>,
        Masami Hiramatsu <mhiramat@...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>,
        Hao Luo <haoluo@...gle.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v5 00/17] Reference count checker and related fixes

On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
>>> On 4/04/23 18:58, Ian Rogers wrote:
>>>> Ping. It would be nice to have this landed or at least the first 10
>>>> patches that refactor the map API and are the bulk of the
>>>> lines-of-code changed. Having those landed would make it easier to
>>>> rebase in the future, but I also think the whole series is ready to
>>>> go.
>>>
>>> I was wondering if the handling of dynamic data like struct map makes
>>> any sense at present.  Perhaps someone can reassure me.
>>>
>>> A struct map can be updated when an MMAP event is processed.  So it
>>
>> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
>>
>> So:
>>
>> 	perf_event__process_mmap()
>> 	  machine__process_mmap2_event()
>> 	    map__new() + thread__insert_map(thread, map)
>> 	    	maps__fixup_overlappings()
>> 			maps__insert(thread->maps, map);
>>
>> Ok, from this point on new samples on ] map->start .. map->end ] will
>> grab a refcount to this new map in its hist_entry, right?
>>
>> When we want to sort by dso we will look at hist_entry->map->dso, etc.
> 
> And in 'perf top' we go decaying hist entries, when we delete the
> hist_entry, drop the reference count to things it holds, that will then
> be finally deleted when no more hist_entries point to it.
> 
>>> seems like anything racing with event processing is already broken, and
>>> reference counting / locking cannot help - unless there is also
>>> copy-on-write (which there isn't at present)?

So I checked, and struct map *is* copy-on-write in
maps__fixup_overlappings(), so that should not be a problem.

>>
>>> For struct maps, referencing it while simultaneously processing
>>> events seems to make even less sense?
>>
>> Can you elaborate some more?

Only that the maps are not necessarily stable e.g. the map that you
need has been replaced in the meantime.

But upon investigation, the only user at the moment is
maps__find_ams().  If we kept the removed maps (we used to),
it might be possible to make maps__find_ams() work correctly
in any case.