linux-kernel - Re: [PATCH v3 03/10] perf annotate: Track address registers via TSR_KIND

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]
Message-ID: <aN9kjm4cLV6mp8Ep@google.com>
Date: Fri, 3 Oct 2025 14:52:14 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Zecheng Li <zecheng@...gle.com>
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>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Xu Liu <xliuprof@...gle.com>, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 03/10] perf annotate: Track address registers via
 TSR_KIND_POINTER

On Wed, Sep 17, 2025 at 07:58:01PM +0000, Zecheng Li wrote:
> Introduce TSR_KIND_POINTER to improve the data type profiler's ability
> to track pointer-based memory accesses and address register variables.
> 
> TSR_KIND_POINTER represents a register that holds the address of the
> type in the `type_state_reg`. The semantics match the `breg` registers
> that describe a memory location.
> 
> This change implements handling for this new kind in mov instructions
> and in the check_matching_type() function. When a TSR_KIND_POINTER is
> moved to the stack, the stack state size is set to the architecture's
> pointer size.
> 
> Signed-off-by: Zecheng Li <zecheng@...gle.com>
> ---
>  tools/perf/arch/x86/annotate/instructions.c | 19 +++++++-
>  tools/perf/util/annotate-data.c             | 52 +++++++++++++++++++--
>  tools/perf/util/annotate-data.h             |  1 +
>  3 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index da98a4e3c52c..698cbb299c6d 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -391,7 +391,7 @@ static void update_insn_state_x86(struct type_state *state,
>  		tsr->ok = true;
>  
>  		/* To copy back the variable type later (hopefully) */
> -		if (tsr->kind == TSR_KIND_TYPE)
> +		if (tsr->kind == TSR_KIND_TYPE || tsr->kind == TSR_KIND_POINTER)
>  			tsr->copied_from = src->reg1;
>  
>  		pr_debug_dtp("mov [%x] reg%d -> reg%d",
> @@ -418,6 +418,10 @@ static void update_insn_state_x86(struct type_state *state,
>  			if (stack == NULL) {
>  				tsr->ok = false;
>  				return;
> +			} else if (stack->kind == TSR_KIND_POINTER) {
> +				tsr->type = stack->type;
> +				tsr->kind = stack->kind;
> +				tsr->ok = true;
>  			} else if (!stack->compound) {

Looks like you can reues the !stack->compound block below.  But you need
to update set_stack_state() not to set it for POINTER types.


>  				tsr->type = stack->type;
>  				tsr->kind = stack->kind;
> @@ -455,6 +459,19 @@ static void update_insn_state_x86(struct type_state *state,
>  				     insn_offset, src->offset, sreg, dst->reg1);
>  			pr_debug_type_name(&tsr->type, tsr->kind);
>  		}
> +		/* Handle dereference of TSR_KIND_POINTER registers */
> +		else if (has_reg_type(state, sreg) && state->regs[sreg].ok &&
> +			 state->regs[sreg].kind == TSR_KIND_POINTER &&
> +			 die_get_member_type(&state->regs[sreg].type,
> +					     src->offset, &type_die)) {
> +			tsr->type = state->regs[sreg].type;
> +			tsr->kind = TSR_KIND_TYPE;
> +			tsr->ok = true;
> +
> +			pr_debug_dtp("mov [%x] addr %#x(reg%d) -> reg%d",
> +				     insn_offset, src->offset, sreg, dst->reg1);
> +			pr_debug_type_name(&tsr->type, tsr->kind);
> +		}
>  		/* Or check if it's a global variable */
>  		else if (sreg == DWARF_REG_PC) {
>  			struct map_symbol *ms = dloc->ms;
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 903027a6fb7d..31b5896276f1 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -59,6 +59,10 @@ void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
>  		pr_info(" constant\n");
>  		return;
>  	case TSR_KIND_PERCPU_POINTER:
> +		pr_info(" percpu pointer");
> +		/* it also prints the type info */
> +		break;
> +	case TSR_KIND_POINTER:
>  		pr_info(" pointer");
>  		/* it also prints the type info */
>  		break;
> @@ -578,7 +582,9 @@ void set_stack_state(struct type_state_stack *stack, int offset, u8 kind,
>  	int tag;
>  	Dwarf_Word size;
>  
> -	if (dwarf_aggregate_size(type_die, &size) < 0)
> +	if (kind == TSR_KIND_POINTER)
> +		size = 8;

Maybe better to use 'sizeof(void *)'.  Later, we may support different
architectures with different pointer size, but that would need many more
work, I guess. :)

Thanks,
Namhyung


> +	else if (dwarf_aggregate_size(type_die, &size) < 0)
>  		size = 0;
>  
>  	tag = dwarf_tag(type_die);
> @@ -898,13 +904,25 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
>  
>  			reg = &state->regs[var->reg];
>  
> -			/* For gp registers, skip the address registers for now */
> -			if (var->is_reg_var_addr)
> +			if (reg->ok && reg->kind == TSR_KIND_TYPE &&
> +			   (!is_better_type(&reg->type, &mem_die) || var->is_reg_var_addr))
>  				continue;
>  
> -			if (reg->ok && reg->kind == TSR_KIND_TYPE &&
> -			    !is_better_type(&reg->type, &mem_die))
> +			/* Handle address registers with TSR_KIND_POINTER */
> +			if (var->is_reg_var_addr) {
> +				if (reg->ok && reg->kind == TSR_KIND_POINTER &&
> +				    !is_better_type(&reg->type, &mem_die))
> +					continue;
> +
> +				reg->type = mem_die;
> +				reg->kind = TSR_KIND_POINTER;
> +				reg->ok = true;
> +
> +				pr_debug_dtp("var [%"PRIx64"] reg%d addr offset %x",
> +					     insn_offset, var->reg, var->offset);
> +				pr_debug_type_name(&mem_die, TSR_KIND_POINTER);
>  				continue;
> +			}
>  
>  			orig_type = reg->type;
>  
> @@ -1116,6 +1134,30 @@ static enum type_match_result check_matching_type(struct type_state *state,
>  		return PERF_TMR_OK;
>  	}
>  
> +	if (state->regs[reg].kind == TSR_KIND_POINTER) {
> +		struct strbuf sb;
> +
> +		strbuf_init(&sb, 32);
> +		die_get_typename_from_type(&state->regs[reg].type, &sb);
> +		pr_debug_dtp("(ptr->%s)", sb.buf);
> +		strbuf_release(&sb);
> +
> +		/*
> +		 * Register holds a pointer (address) to the target variable.
> +		 * The type is the type of the variable it points to.
> +		 */
> +		*type_die = state->regs[reg].type;
> +
> +		dloc->type_offset = dloc->op->offset;
> +
> +		/* Get the size of the actual type */
> +		if (dwarf_aggregate_size(type_die, &size) < 0 ||
> +		    (unsigned)dloc->type_offset >= size)
> +			return PERF_TMR_BAD_OFFSET;
> +
> +		return PERF_TMR_OK;
> +	}
> +
>  	if (state->regs[reg].kind == TSR_KIND_PERCPU_POINTER) {
>  		pr_debug_dtp("percpu ptr");
>  
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index dd3807b55208..fd0d1084bc4e 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -35,6 +35,7 @@ enum type_state_kind {
>  	TSR_KIND_PERCPU_BASE,
>  	TSR_KIND_CONST,
>  	TSR_KIND_PERCPU_POINTER,
> +	TSR_KIND_POINTER,
>  	TSR_KIND_CANARY,
>  };
>  
> -- 
> 2.51.0.384.g4c02a37b29-goog
>