linux-kernel - Re: [PATCH] Documentation/submitting-patches: Add blurb about backtraces in commit messages

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X+odFBu+smuDiOcO@google.com>
Date:   Mon, 28 Dec 2020 09:59:48 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Jonathan Corbet <corbet@....net>, x86-ml <x86@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Documentation/submitting-patches: Add blurb about
 backtraces in commit messages

On Tue, Dec 22, 2020, Borislav Petkov wrote:
> On Tue, Dec 22, 2020 at 10:59:22AM -0800, Sean Christopherson wrote:
> > On Tue, Dec 22, 2020, Borislav Petkov wrote:
> > > +Backtraces help document the call chain leading to a problem. However,
> > > +not all backtraces are helpful. For example, early boot call chains are
> > > +unique and obvious.
> > 
> > I'd argue that there is still value in the backtrace though, e.g. I find them
> > very helpful when doing git archaeology.  A backtrace is an easily recognizable
> > signature (don't have to read a bunch of text to understand there was a splat of
> > some kind), and the call stack is often helpful even if it is unique, e.g. for
> > unfamiliar code (including early boot chains) and/or code that is substantially
> > different from the current upstream.
> 
> I think the intent of the text is to say not to include callchains which
> are *really* obvious. As in, there's no ambiguity as to how one has
> landed here.
> 
> Also, sometimes people paste backtraces from a WARN* which are almost
> always superfluous - only the warn's address is important. This is at
> least how I go about debugging those.

Obvious and superfluous for people that are intimately familiar with the code,
but explicit call stacks are extremely helpful when (re)learning code.  Using
the boot code as an example, until one fully understands all the function
pointer shenanigans for the real mode trampoline, initial_code, CPU HP, etc...,
even 100% unambiguous call chains may not be immediately obvious.  IMO, a few
"unnecessary" lines in the changelog is a worthwhile tradeoff if it's helpful to
even one person, now or in the future.

> Maybe the text should be made more precise.
> 
> > I'd prefer not to encourage people to strip the info after the function name,
> > though I do agree it's somewhat distracting (especially the offset/size).
> 
> Yes. Especially since they don't make any sense on another system or
> even on the same system but with a different .config.
>
> > The module, call site in the function, exact file/line if available,
> > etc... provides context that I find helpful for building a mental
> > model of what went wrong.
> 
> File/line is more useful, yes, but only for the current code snapshot.
> When time passes and stuff gets changed, those file/line things are not
> correct anymore so one would have to checkout the tree on which the
> splat happened.

I don't think that's a reason to encourage stripping that info though, e.g. I'll
often checkout the relevant tree when I'm deep down the blame rabbithole.
Similar to my above argument regarding call stacks, the cost of including the
may-be-stale file/line info is outweighed by the potential benefits for readers
and/or future archaeologists.

> I guess I need to make that aspect more precise too.