Re: [PATCH v9 04/18] perf capstone: Determine architecture from e_machine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional application of CS_MODE_64 and CS_MODE_32 breaks Capstone initialization for architectures like ARM, ARM64, and RISC-V.
- [High] Uninitialized `is_big_endian` on thread cache hits causes big-endian architectures to be disassembled as little-endian.
- [Medium] Lockless multi-variable data race when caching thread `e_machine` and `e_flags`.
--

commit 438f1b068410120794eb1080db889c82f269a06e
Author: Ian Rogers <irogers@google.com>

perf capstone: Determine architecture from e_machine

This commit updates Capstone initialization to use e_machine instead of arch
strings to determine the architecture. It also adds e_machine translations
for MIPS and RISCV, and removes unnecessary maybe_unused annotations.

[Note: The report format has been altered to provide a free-form summary of
the findings instead of inline diff quoting due to recitation restrictions.]

Regarding the implementation of e_machine_to_capstone(), is it intended to
unconditionally set CS_MODE_64 or CS_MODE_32 in the mode parameter for all
architectures?

Capstone defines CS_MODE_32 as 4 and CS_MODE_64 as 8. While these mode flags
are valid for architectures like x86 and MIPS, they might not be supported
for others. For instance, RISC-V uses CS_MODE_RISCV32 or CS_MODE_RISCV64,
and ARM expects CS_MODE_ARM.

If CS_MODE_64 or CS_MODE_32 is combined with the architecture-specific flags
for ARM, ARM64, or RISC-V, could it pass an invalid bitmask to Capstone and
cause cs_open() to fail with CS_ERR_MODE?

In functions like capstone__fprintf_insn_asm(), the is_big_endian variable
is initialized to false and passed by reference to thread__e_machine_endian().
If thread__e_machine_endian() finds that the architecture is already cached,
it takes an early return path.

Does this early return path bypass updating the is_big_endian out-parameter?
Since the thread cache only stores e_machine and e_flags, could this cause
big-endian architectures to be disassembled as little-endian on cache hits
because is_big_endian remains false?

Additionally, when thread__e_machine_endian() caches the architecture
details, it appears to update e_machine and e_flags sequentially without a
lock or memory barriers.

Could a concurrent thread observe the newly updated e_machine, take the early
return path, and read a stale or uninitialized e_flags value before the first
thread has completed its update?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260503002248.1040791-1-irogers@google.com?part=4