linux-kernel - Re: [RFC][PATCH v4 01/32] objtool: Prepare to merge recordmcount

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]

Date:   Tue, 9 Jun 2020 08:42:21 -0700
From:   Matt Helsley <mhelsley@...are.com>
To:     Julien Thierry <jthierry@...hat.com>
CC:     <linux-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>
Subject: Re: [RFC][PATCH v4 01/32] objtool: Prepare to merge recordmcount

On Tue, Jun 09, 2020 at 09:54:33AM +0100, Julien Thierry wrote:
> Hi Matt,
> 
> On 6/2/20 8:49 PM, Matt Helsley wrote:
> > Move recordmcount into the objtool directory. We keep this step separate
> > so changes which turn recordmcount into a subcommand of objtool don't
> > get obscured.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@...are.com>

<snip>

> > diff --git a/Makefile b/Makefile
> > index 04f5662ae61a..d353a0a65a71 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -844,6 +844,7 @@ ifdef CONFIG_DYNAMIC_FTRACE
> >   	ifdef CONFIG_HAVE_C_RECORDMCOUNT
> >   		BUILD_C_RECORDMCOUNT := y
> >   		export BUILD_C_RECORDMCOUNT
> > +		objtool_target := tools/objtool FORCE
> >   	endif
> >   endif
> >   endif
> > @@ -1023,10 +1024,10 @@ endif
> >   export mod_sign_cmd
> >   HOST_LIBELF_LIBS = $(shell pkg-config libelf --libs 2>/dev/null || echo -lelf)
> > +has_libelf := $(call try-run,\
> > +		echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
> 
> Maybe there could be some build dependency, e.g. CONFIG_OBJTOOL_SUBCMDS that
> sets the "objtool_target" and "has_libelf" when selected.
> 
> Then the CONFIG_UNWINDER_ORC, RECORD_MCOUNT and STACK_VALIDATION would just
> had to select that config option.

That might save a good amount of control flow in the Makefiles.

We could take it one step further and have specific CONFIG_OBJTOOL_<subcmd>
which might help us remove the per-architecture control-flow in
the multi-arch subcmd support found in tools/objtool/Makefile.

What do folks think of that -- too far?

> 
> >   ifdef CONFIG_STACK_VALIDATION
> > -  has_libelf := $(call try-run,\
> > -		echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
> >     ifeq ($(has_libelf),1)
> >       objtool_target := tools/objtool FORCE
> >     else
> > @@ -1163,13 +1164,15 @@ uapi-asm-generic:
> >   PHONY += prepare-objtool
> >   prepare-objtool: $(objtool_target)
> > -ifeq ($(SKIP_STACK_VALIDATION),1)
> > -ifdef CONFIG_UNWINDER_ORC
> > +ifneq ($(has_libelf),1)
> > +  ifdef CONFIG_UNWINDER_ORC
> >   	@echo "error: Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2
> >   	@false
> > -else
> > +  else
> > +    ifeq ($(SKIP_STACK_VALIDATION),1)
> >   	@echo "warning: Cannot use CONFIG_STACK_VALIDATION=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2
> 
> 
> I think this would be more readable without the else branch:
> 
> 	ifneq ($(has_libelf),1)
> 		ifdef <some objtool command config>
> 			<warn about unavailability>

Note: error not warn

> 		endif
> 		ifdef <another objtool command config>
> 			<warn ...>
> 		endif
> 		<...>
> 	endif

I think the next patch, which makes recordmcount a subcmd, makes it a
little clearer what the pattern is because it adds another ifdef block
in the way you suggest.

As for the else around the SKIP_STACK_VALIDATION check -- it is special
in a couple ways -- at least as best I can tell.

It's not a CONFIG_* -- it actually breaks the normal pattern with
CONFIG_* in that..

It's about a judgement call that it's OK to merely warn and skip the
stack validation rather than produce an error. The other, CONFIG_*
blocks produce errors.

These two reasons are why I think it makes sense to keep the logic
distinct with the "else".

Cheers,
	-Matt Helsley