[PATCH V2 1/2] arch/powerpc/perf: Check the instruction type before creating sample with perf_mem_data_src

2025-01-18 Thread Athira Rajeev
perf mem report aborts as below sometimes (during some corner
case) in powerpc:

   # ./perf mem report 1>out
   *** stack smashing detected ***: terminated
   Aborted (core dumped)

The backtrace is as below:
   __pthread_kill_implementation ()
   raise ()
   abort ()
   __libc_message
   __fortify_fail
   __stack_chk_fail
   hist_entry.lvl_snprintf
   __sort__hpp_entry
   __hist_entry__snprintf
   hists.fprintf
   cmd_report
   cmd_mem

Snippet of code which triggers the issue
from tools/perf/util/sort.c

   static int hist_entry__lvl_snprintf(struct hist_entry *he, char *bf,
size_t size, unsigned int width)
   {
char out[64];

perf_mem__lvl_scnprintf(out, sizeof(out), he->mem_info);
return repsep_snprintf(bf, size, "%-*s", width, out);
   }

The value of "out" is filled from perf_mem_data_src value.
Debugging this further showed that for some corner cases, the
value of "data_src" was pointing to wrong value. This resulted
in bigger size of string and causing stack check fail.

The perf mem data source values are captured in the sample via
isa207_get_mem_data_src function. The initial check is to fetch
the type of sampled instruction. If the type of instruction is
not valid (not a load/store instruction), the function returns.

Since 'commit e16fd7f2cb1a ("perf: Use sample_flags for data_src")',
data_src field is not initialized by the perf_sample_data_init()
function. If the PMU driver doesn't set the data_src value to zero if
type is not valid, this will result in uninitailised value for data_src.
The uninitailised value of data_src resulted in stack check fail
followed by abort for "perf mem report".

When requesting for data source information in the sample, the
instruction type is expected to be load or store instruction.
In ISA v3.0, due to hardware limitation, there are corner cases
where the instruction type other than load or store is observed.
In ISA v3.0 and before values "0" and "7" are considered reserved.
In ISA v3.1, value "7" has been used to indicate "larx/stcx".
Drop the sample if instruction type has reserved values for this
field with a ISA version check. Initialize data_src to zero in
isa207_get_mem_data_src if the instruction type is not load/store.

Reported-by: Disha Goel 
Signed-off-by: Athira Rajeev 
---
Changelog:
 v1 -> v2
 Define macros for ISA207_SIER_TYPE_SHIFT and
 ISA207_SIER_TYPE_MASK and remove include for "isa207-common.h"
 so that code compiles on 32 bit platform also.
 Reported-by: kernel test robot 
 Closes: 
https://lore.kernel.org/oe-kbuild-all/202501180825.lprg6wye-...@intel.com/

 arch/powerpc/perf/core-book3s.c   | 20 
 arch/powerpc/perf/isa207-common.c |  4 +++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 2b79171ee185..49831cd93cd3 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -,6 +,10 @@ static struct pmu power_pmu = {
 #define PERF_SAMPLE_ADDR_TYPE  (PERF_SAMPLE_ADDR | \
PERF_SAMPLE_PHYS_ADDR | \
PERF_SAMPLE_DATA_PAGE_SIZE)
+
+#define ISA207_SIER_TYPE_SHIFT 15
+#define ISA207_SIER_TYPE_MASK  (0x7ull << ISA207_SIER_TYPE_SHIFT)
+
 /*
  * A counter has overflowed; update its count and record
  * things if requested.  Note that interrupts are hard-disabled
@@ -2290,6 +2294,22 @@ static void record_and_restart(struct perf_event *event, 
unsigned long val,
is_kernel_addr(mfspr(SPRN_SIAR)))
record = 0;
 
+   /*
+* SIER[46-48] presents instruction type of the sampled instruction.
+* In ISA v3.0 and before values "0" and "7" are considered reserved.
+* In ISA v3.1, value "7" has been used to indicate "larx/stcx".
+* Drop the sample if "type" has reserved values for this field with a
+* ISA version check.
+*/
+   if (event->attr.sample_type & PERF_SAMPLE_DATA_SRC &&
+   ppmu->get_mem_data_src) {
+   val = (regs->dar & ISA207_SIER_TYPE_MASK) >> 
ISA207_SIER_TYPE_SHIFT;
+   if (val == 0 || (val == 7 && 
!cpu_has_feature(CPU_FTR_ARCH_31))) {
+   record = 0;
+   atomic64_inc(&event->lost_samples);
+   }
+   }
+
/*
 * Finally record data if requested.
 */
diff --git a/arch/powerpc/perf/isa207-common.c 
b/arch/powerpc/perf/isa207-common.c
index 56301b2bc8ae..031a2b63c171 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -321,8 +321,10 @@ void isa207_get_mem_data_src(union perf_mem_data_src 
*dsrc, u32 flags,
 
sier = mfspr(SPRN_SIER);
val = (sier & ISA207_SIER_TYPE_MASK) >> ISA207_SIER_TYPE_SHIFT;
-   if (val != 1 && val != 2 && !(val == 7 && 
cpu_has_feature(CPU_FTR_ARCH_31)))
+   if (val != 1 && val != 2 &&

[PATCH V2 2/2] arch/powerpc/perf: Update get_mem_data_src function to use saved values of sier and mmcra regs

2025-01-18 Thread Athira Rajeev
During performance monitor interrupt handling, the regs are setup using
perf_read_regs function. Here some of the pt_regs fields is overloaded.
Samples Instruction Event Register (SIER) is loaded into pt_regs,
overloading regs->dar. And regs->dsisr to store MMCRA (Monitor Mode
Control Register A) so that we only need to read these once on each
interrupt.

Update the isa207_get_mem_data_src function to use regs->dar instead of
reading from SPRN_SIER again. Also use regs->dsisr to read the mmcra
value

Signed-off-by: Athira Rajeev 
---
 arch/powerpc/perf/isa207-common.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/perf/isa207-common.c 
b/arch/powerpc/perf/isa207-common.c
index 031a2b63c171..2b3547fdba4a 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -319,7 +319,13 @@ void isa207_get_mem_data_src(union perf_mem_data_src 
*dsrc, u32 flags,
return;
}
 
-   sier = mfspr(SPRN_SIER);
+   /*
+* Use regs-dar for SPRN_SIER which is saved
+* during perf_read_regs at the beginning
+* of the PMU interrupt handler to avoid multiple
+* reads of SPRN_SIER
+*/
+   sier = regs->dar;
val = (sier & ISA207_SIER_TYPE_MASK) >> ISA207_SIER_TYPE_SHIFT;
if (val != 1 && val != 2 && !(val == 7 && 
cpu_has_feature(CPU_FTR_ARCH_31))) {
dsrc->val = 0;
@@ -340,8 +346,12 @@ void isa207_get_mem_data_src(union perf_mem_data_src 
*dsrc, u32 flags,
 * to determine the exact instruction type. If the sampling
 * criteria is neither load or store, set the type as default
 * to NA.
+*
+* Use regs->dsisr for MMCRA which is saved during 
perf_read_regs
+* at the beginning of the PMU interrupt handler to avoid
+* multiple reads of SPRN_MMCRA
 */
-   mmcra = mfspr(SPRN_MMCRA);
+   mmcra = regs->dsisr;
 
op_type = (mmcra >> MMCRA_SAMP_ELIG_SHIFT) & 
MMCRA_SAMP_ELIG_MASK;
switch (op_type) {
-- 
2.39.3




Re: [PATCH V2] tools/perf/builtin-lock: Fix return code for functions in __cmd_contention

2025-01-18 Thread Namhyung Kim
On Fri, 10 Jan 2025 15:07:30 +0530, Athira Rajeev wrote:

> perf lock contention returns zero exit value even if the lock contention
> BPF setup failed.
> 
>   # ./perf lock con -b true
>   libbpf: kernel BTF is missing at '/sys/kernel/btf/vmlinux', was 
> CONFIG_DEBUG_INFO_BTF enabled?
>   libbpf: failed to find '.BTF' ELF section in 
> /lib/modules/6.13.0-rc3+/build/vmlinux
>   libbpf: failed to find valid kernel BTF
>   libbpf: kernel BTF is missing at '/sys/kernel/btf/vmlinux', was 
> CONFIG_DEBUG_INFO_BTF enabled?
>   libbpf: failed to find '.BTF' ELF section in 
> /lib/modules/6.13.0-rc3+/build/vmlinux
>   libbpf: failed to find valid kernel BTF
>   libbpf: Error loading vmlinux BTF: -ESRCH
>   libbpf: failed to load object 'lock_contention_bpf'
>   libbpf: failed to load BPF skeleton 'lock_contention_bpf': -ESRCH
>   Failed to load lock-contention BPF skeleton
>   lock contention BPF setup failed
>   # echo $?
>0
> 
> [...]

Applied to perf-tools-next, thanks!

Best regards,
Namhyung