linux-kernel - Re: [PATCH v1 4/5] perf capstone: Support for dlopen-ing libcapstone.so

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] [day] [month] [year] [list]

Message-ID: <CAP-5=fXx=q-LHcb9BtjE1AFZGmV55NBWy3HvfWQaABo-YO=4gA@mail.gmail.com>
Date: Thu, 23 Jan 2025 08:07:48 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
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@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	"Steinar H. Gunderson" <sesse@...gle.com>, Charlie Jenkins <charlie@...osinc.com>, 
	Changbin Du <changbin.du@...wei.com>, "Masami Hiramatsu (Google)" <mhiramat@...nel.org>, 
	James Clark <james.clark@...aro.org>, Kajol Jain <kjain@...ux.ibm.com>, 
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, Li Huafei <lihuafei1@...wei.com>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v1 4/5] perf capstone: Support for dlopen-ing libcapstone.so

On Wed, Jan 22, 2025 at 2:42 PM Ian Rogers <irogers@...gle.com> wrote:
>
> On Wed, Jan 22, 2025 at 2:27 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hi Ian,
> >
> > On Mon, Jan 20, 2025 at 09:32:07AM -0800, Ian Rogers wrote:
> > > If perf wasn't built against libcapstone, no HAVE_LIBCAPSTONE_SUPPORT,
> > > support dlopen-ing libcapstone.so and then calling the necessary
> > > functions by looking them up using dlsym. Reverse engineer the types
> > > in the API using pahole, adding only what's used in the perf code or
> > > necessary for the sake of struct size and alignment.
> >
> > I think reverse engineering the types is fragile.  Can we simply change
> > to dlopen if libcapstone is available?
>
> The current behavior is if HAVE_LIBCAPSTONE_SUPPORT then we will link
> against libcapstone and can assume the header files are present.
>
> The reverse engineering of structs and constants I agree is brittle,
> but these things shouldn't change, it is used in this code when the
> capstone/capstone.h isn't present. If capstone/capstone.h is required
> we could change the ifdefs to indicate:
> 1) capstone is present and we're linking against it (this is
> HAVE_LIBCAPSTONE_SUPPORT today);
> 2) capstone is present but use it by way of dlopen;
> 3) capstone isn't present don't #include, don't link and don't dlopen.
>
> My thought was that this is less good than the reverse engineering.
> (1) relies on the builder having libcapstone, which at build time is a
> warning that flies by. (2) also relies on the builder having to have
> libcapstone and to opt into a dlopen behavior, they'll need to read
> Makefiles to discover this. (3) gives a less good perf tool with in
> the source lots of __maybe_unused and ifdef-ed out functions all over
> the place.
>
> With the reverse engineered values if you have libcapstone then
> HAVE_LIBCAPSTONE_SUPPORT works as before - no change. If you don't
> then you will be opted into getting something better than objdump -
> albeit it could break if the reverse engineered values drift from
> those in capstone/capstone.h. Less build options also is less
> complexity and cleaner code.

To be clear, if the header file values/structs drift then libcapstone
will need to version values/structs, we'll need to update the perf
code to use these, etc. Otherwise a tool linked against an older
libcapstone will break when libcapstone is updated. The code handling
not having any libcapstone, dlopen-ed or not, would be significant as
every static function will need an #ifdef to avoid a not used warning,
every parameter will need a __maybe_unused and the overall code
quality in perf will be worse imo because of this. The no libcapstone
default when not detected would be crappy for users. The values/types
added here are fewer than 20, and so are not a huge maintenance
burden. I did a similar thing for the LLVM C++ symbolizer code and its
maintenance would have been a burden and so abandoned it in the
llvm-c-helpers case that mainly affects addr2line and not disassembly.
Hopefully later LLVM C APIs will make this unnecessary.

Anyway, because of this I won't be changing this in later versions. I
will be carrying these changes in Google's tree and think ideally it
would be carried in the regular tree.

Thanks,
Ian