Re: [PATCH v11 00/20] Compile-time stack validation

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Aug 28, 2015 at 10:21:16AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Aug 26, 2015 at 09:07:23AM +0200, Ingo Molnar wrote:
> > > 
> > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > 
> > > > BTW, since I'm planning on adding a CFI generation option to this tool, I wonder 
> > > > if "stackvalidate" is a misnomer.  I can't think of anything better.  Any naming 
> > > > ideas are welcome :-)
> > > 
> > > I'd suggest 'debuginfo' and a Git-alike set of subcommands:
> > > 
> > >    debuginfo help
> > >    debuginfo check
> > >    debuginfo check framepointers
> > >    debuginfo check cfi
> > > 
> > > Such a command scheme keeps open various future commands, such as:
> > > 
> > >    debuginfo show
> > >    debuginfo size
> > >    debuginfo compress
> > >    debuginfo strip
> > > 
> > > etc.
> > > 
> > > I also checked that there does not appear to be such a Linux tool with such a name 
> > > at the moment.
> > 
> > My feeling is that the subcommand model wouldn't fit this tool very well.  Its 
> > core functionality is to analyze code paths -- which it does in a single pass, 
> > regardless of whether it's checking frame pointers, checking CFI, generating 
> > CFI, or some combination.  Splitting it up into subcommands would mean having to 
> > repeat the same code analysis pass multiple times unnecessarily.
> 
> Huh?
> 
> The subcommand approach is a user UI that does not limit the tool in any way: you 
> are free to provide subcommands that combine more atomic functionality - similarly 
> to how Git provides a 'git pull' subcommand that is a combination of 'fetch' and 
> 'merge' steps.

Sure, but it doesn't scale if *all* the subcommands are combinable.  For
n subcommands which can be combined, you'd need (2^n - 1) total
subcommands to cover all possible combinations.  In that case,
subcommands would be much more unwieldy than just having n flags that
can be easily combined.

This is an important point because I think any hypothetical future
options would be likely to be combinable if they take advantage of the
tool's main functionality, which is walking all the code paths.  If they
don't take advantage of that, they should probably be in a separate tool
anyway.

> 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 important step is to not limit the tool to 'checking' only, if there's 
> reasonable other tasks it could perform:

>  - For example in the future we might want to sanity check the dwarf debuginfo 
>    whether it's all well suited for kernel probing.

Yes, and this is planned soon.

>  - And if the tool is doing such a good job analyzing stacks, why not extend it 
>    trivially to print max stack backtrace estimations?

Sure, we could do that.

> etc. By limiting the name at inception unreasonably you make all these things less 
> obvious to add.

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).

> The tool should obviously not be named 'kitchensink' just for future 
> extensability, but at least don't limit it to stacks and checking only ...
> 
> > Also, since it's really focused on stack-related code path analysis, I wouldn't 
> > expect it to be branching out into other debuginfo-related tasks.
> > 
> > As far as naming goes, "debuginfo" usually refers to DWARF metadata. But this 
> > tool isn't DWARF-specific, so that would be confusing IMO.  I also agree with 
> > Jiri that re-using a generic name which already has another meaning would add to 
> > the confusion.
> > 
> > The goal of this tool is to fix stack traces, so how about "stackfix"?
> 
> Sigh, please don't limit the tool's purpose with its name ...
> 
> and choose a better name! :-) The tool does not 'fix' anything, and the normal 
> case would be for it to produce no warnings. I.e. it's not a fixing, it's a 
> checking tool.
> 
> So something like 'stackcheck' or 'stackinfo' - but I really think 'debuginfo' is 
> better, because for more complex kernel image operations like live patching the 
> kernel has to double check the untrusted debuginfo that compilers barf out ...
> 
> Frame pointers are simply a legacy debuginfo variant, one that the kernel happens 
> to use as its primary debuginfo. I don't think it's confusing at all, especially 
> if any dwarf logic is added ...

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.

For example, the tool could have a replacement for "make checkstack",
which generates a list of functions which are stack hogs.  That has
nothing to do with debuginfo.

(And note this is a further example of why subcommands are not a good
fit.  We would want to be able to combine this option with the others
without needing an exponential growth in the number of subcommands.)

-- 
Josh