linux-kernel - Re: [RFC 09/13] objtool: Add option to trace function validation

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: <hijyjj2ammxiyeogytqtkjj7hbt3kmz7l65rueusftoy7u3pnv@vwtp3ctv7r2u>
Date: Tue, 10 Jun 2025 17:48:36 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Alexandre Chartre <alexandre.chartre@...cle.com>
Cc: linux-kernel@...r.kernel.org, mingo@...nel.org, peterz@...radead.org
Subject: Re: [RFC 09/13] objtool: Add option to trace function validation

On Fri, Jun 06, 2025 at 05:34:36PM +0200, Alexandre Chartre wrote:
> +++ b/tools/objtool/builtin-check.c
> @@ -99,6 +99,7 @@ static const struct option check_options[] = {
>  	OPT_STRING('o',  "output", &opts.output, "file", "output file name"),
>  	OPT_BOOLEAN(0,   "sec-address", &opts.sec_address, "print section addresses in warnings"),
>  	OPT_BOOLEAN(0,   "stats", &opts.stats, "print statistics"),
> +	OPT_STRING(0,	"trace", &opts.trace, "func", "trace function validation"),

Here, "trace" is vertically misaligned with the other long options.

> @@ -36,6 +37,80 @@ static struct cfi_state force_undefined_cfi;
>  
>  static size_t sym_name_max_len;
>  
> +static bool vtrace;
> +static int vtrace_depth;

I'm not sure what the "v" means here and elsewhere, can we remove it to
improve readability?  The option is called "trace" after all.

> +/*
> + * Validation traces are sent to stderr so that they are output
> + * on the same flow as warnings.
> + */
> +#define VTRACE_PRINTF(fmt, ...)		fprintf(stderr, fmt, ##__VA_ARGS__)

I'm not sure this comment is really needed.  At least the reason for the
stderr seems obvious to me.  Besides that, it might make sense for
tracing to use stderr anyway, regardless of whether warnings existed.

Also I think the "_PRINTF" is self-evident, can we call it TRACE() or
so?  That would be more analagous to the existing WARN()/ERROR().

> +	/* print message if any */
> +	if (format) {
> +		VTRACE_PRINTF(" - ");
> +		va_start(args, format);
> +		vfprintf(stderr, format, args);

This breaks the macro abstraction, do we need a wrapper for
"vfprintf(stderr...)" as well?

VTRACE() maybe? (assuming the base macro gets renamed to TRACE)

> @@ -3580,10 +3665,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  		ret = validate_insn(file, func, insn, &state,
>  				    prev_insn, next_insn,
>  				    &validate_next);
> -		if (!validate_next)
> -			break;
>  
> -		if (!next_insn) {
> +		if (validate_next && !next_insn) {
>  			if (state.cfi.cfa.base == CFI_UNDEFINED)
>  				return 0;
>  			if (file->ignore_unreachables)
> @@ -3595,9 +3678,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  			return 1;
>  		}
>  
> +		if (!insn->vtrace) {
> +			if (ret)
> +				VTRACE_INSN(insn, "validated (%d)", ret);

I'm not sure what "validated" communicates here, should this instead
indicate there was an warning?

> +			else
> +				VTRACE_INSN(insn, NULL);
> +		}

Should these VTRACE_INSN()s be done before the !next_insn clause above
so we can see the last instruction before the branch validation stopped?

> @@ -3696,13 +3791,24 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
>  		return 1;
>  
>  	if (insn->alts) {
> +		int i, count;
> +
> +		count = 0;
> +		for (alt = insn->alts; alt; alt = alt->next)
> +			count++;

"count" is rather vague, how about "num_alts" or so.

> @@ -3733,13 +3841,21 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
>  	case INSN_JUMP_CONDITIONAL:
>  	case INSN_JUMP_UNCONDITIONAL:
>  		if (is_sibling_call(insn)) {
> +			VTRACE_INSN(insn, "sibling call");
>  			ret = validate_sibling_call(file, insn, statep);
>  			if (ret)
>  				return ret;
>  
>  		} else if (insn->jump_dest) {
> -			ret = validate_branch(file, func,
> -					      insn->jump_dest, *statep);
> +			if (insn->type == INSN_JUMP_UNCONDITIONAL) {
> +				VTRACE_INSN(insn, "unconditional jump");
> +				ret = do_validate_branch(file, func,
> +							 insn->jump_dest, *statep);
> +			} else {
> +				VTRACE_INSN(insn, "jump taken");
> +				ret = validate_branch(file, func,
> +						      insn->jump_dest, *statep);
> +			}

This can be simplified:

			if (insn->type == INSN_JUMP_UNCONDITIONAL)
				VTRACE_INSN(insn, "unconditional jump");
			else
				VTRACE_INSN(insn, "jump taken");

			ret = do_validate_branch(file, func, insn->jump_dest, *statep);
>  			if (ret) {
>  				BT_INSN(insn, "(branch)");
>  				return ret;
> @@ -3749,10 +3865,12 @@ static int validate_insn(struct objtool_file *file, struct symbol *func,
>  		if (insn->type == INSN_JUMP_UNCONDITIONAL)
>  			return 0;
>  
> +		VTRACE_INSN(insn, "jump not taken");
>  		break;
>  
>  	case INSN_JUMP_DYNAMIC:
>  	case INSN_JUMP_DYNAMIC_CONDITIONAL:
> +		VTRACE_INSN(insn, "dynamic jump");

Let's call it "indirect jump" (and yes, those enums are poorly named).

Shall we have a distinct trace for indirect calls (INSN_CALL_DYNAMIC) as
well?

> @@ -4253,9 +4391,22 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
>  	if (opts.uaccess)
>  		state->uaccess = sym->uaccess_safe;
>  
> +	if (opts.trace && fnmatch(opts.trace, sym->name, 0) == 0) {

Please use "!fnmatch(...)" instead of "fnmatch(...) == 0".

> +++ b/tools/objtool/include/objtool/check.h
> @@ -81,6 +81,8 @@ struct instruction {
>  	struct symbol *sym;
>  	struct stack_op *stack_ops;
>  	struct cfi_state *cfi;
> +
> +	u32 vtrace;
>  };

Can this just be a single bit instead of u32?  This struct is one of the
biggest memory hogs, vmlinux.o can have a gazillion instructions.  There
are actually some bits already reserved in this struct.

-- 
Josh