Message-ID: <20150914141023.GA13856@treble.redhat.com>
Date:	Mon, 14 Sep 2015 09:10:23 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
	Michal Marek <mmarek@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Pedro Alves <palves@...hat.com>,
	Namhyung Kim <namhyung@...il.com>,
	Bernd Petrovitsch <bernd@...rovitsch.priv.at>,
	Chris J Arges <chris.j.arges@...onical.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	David Vrabel <david.vrabel@...rix.com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Chris Wright <chrisw@...s-sol.org>,
	Alok Kataria <akataria@...are.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	Pavel Machek <pavel@....cz>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <len.brown@...el.com>,
	Matt Fleming <matt.fleming@...el.com>,
	Arnaldo Carvalho de Melo <acme@...radead.org>
Subject: Re: [PATCH v11 00/20] Compile-time stack validation

On Mon, Sep 14, 2015 at 03:19:52PM +0200, Ingo Molnar wrote:
> > > In this case it would be a simple:
> > > 
> > >    debuginfo check all
> > > 
> > > to check everything. You can also make the selection of debuginfo components 
> > > to check a regular option, not a subcommand.
> > 
> > The reason I proposed a name change is that it will soon do *more* than just 
> > checking.  It will also do CFI generation by modifying the object file.
> > 
> > What subcommand would you suggest for the following?
> > 
> > - do frame pointer validation; and
> > 
> > - if CFI exists, do CFI validation, else do CFI generation.
> 
> The main functionality here is to fix up the CFI info, so I'd name it:
> 
>    debuginfo fix cfi
> 
> where the 'fix' subcommand would use functionality from the 'check' subcommand to 
> see whether there's CFI info present (and if yes, sanity check it and warn if it's 
> not good).

I still don't see how that would work.  Here's how we would achieve that
example with my latest proposal:

  stacktool --check-frame-pointer --check-cfi --gen-cfi

Notice how clear it is *exactly* what the tool is doing.

On the other hand, "debuginfo fix cfi" hides two of the three tasks.

It's also confusing and inconsistent: use the "check" subcommand for
checking, but use the "fix" subcommand for both checking *and*
generation?  That's far from obvious for the user.

Further, with my proposal all three options can be added and removed in
various combinations.  So you can do things like:

stacktool --gen-cfi
stacktool --check-frame-pointer --gen-cfi
stacktool --check-cfi --gen-cfi

How would you do those with subcommands?

> > But note these examples are still related to stacks, so having "stack" in the 
> > name of the tool wouldn't be limiting (for these examples at least).
> 
> Absolutely, I'd name it 'debuginfo' at minimum to not unnecessarily limit things 
> at the inception of the tool with 'stackfix'.

Actually I would use the same argument *against* debuginfo.  We've
already identified a realistic potential usage that's unrelated to debug
info: stack size checking.  "debuginfo" is too limiting for that case.

OTOH, I don't think we've yet conceived of any non-stack-related uses of
the tool (other than some suggestions which are outside the scope of its
core functionality of analyzing all code paths).  So I really think
something with "stack" in the name would be appropriate.

> > I proposed the "fix" in "stackfix" because it will do more than just checking: 
> > it will also be able to modify the object file (as I describe above).  And 
> > "stack" because thus far the proposed scope of the tool is strictly related to 
> > stacks.
> > 
> > I think "debuginfo" is limiting in its own way.  The core functionality of the 
> > tool is to analyze all possible code paths, which isn't directly related to 
> > debuginfo.  We might want to do other kinds of code path analysis which are 
> > unrelated to debuginfo.
> 
> So if you can think of an even more generic name than 'debuginfo', that would be 
> even better - what I objected to was the limiting 'stackfix' name.
> 
> For example 'binary' might work well too, here's a few mockup subcommands:
> 
>    binary check fp                   # checks framepointers in a binary
>    binary check all                  # checks everything it can in a binary
>    binary generate cfi               # generates CFI info
>    binary ls                         # prints section sizes
>    binary compress                   # strip out NOPs and other padding from a binary if possible
> 
> (But 'fix' instead of 'generate' would work as well.)
> 
> Note how intuitive the wording it, it's almost a free flowing English sentence.

That's way too generic IMO.  A tool named "binary" could do practically
anything.  The name doesn't give the user any idea about what it
actually does.

Also the proposed subcommands "ls" and "compress" have nothing to do
with recursive code path analysis and really belong in separate tools.

I really don't see any benefit to making a big monolithic tool which
does all things related to ELF/DWARF binary analysis.  There are already
a lot of good tools out there which do a lot of those things.

I don't see any good reason to grow this tool beyond its core
functionality of recursive code analysis.  At some point we have to draw
a reasonable line in the sand about what it will eventually do.
Otherwise we should just call it "tool" ;-)

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists