Re: [PATCH] debug: Allow forcing entering debug mode on panic/exception

[Doug Anderson]

Hi,

On Tue, Dec 18, 2018 at 9:05 AM Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
> On Sun, Dec 09, 2018 at 06:36:49PM -0800, Douglas Anderson wrote:
> > Ever since commit 5516fd7b92a7 ("debug: prevent entering debug mode on
> > panic/exception.") (yes, years ago) my kgdb workflow has been broken.
> > On Chrome OS we have 'kernel.panic = -1' in
> > '/etc/sysctl.d/00-sysctl.conf'.  That means that when userspace starts
> > up it will tell the kernel "please reboot on panic".  ...and so when I
> > get a panic then the system reboots instead of letting me debug it.
> >
> > While I could go in an change the 'sysctl.conf' and I could go in and
> > hack the kernel myself, these things are inconvenient.  I either need
> > to keep a private kernel patch or or remember to edit a file every
> > time I install an updated version of Chrome OS.  What is convenient
> > (for me) is to have a CONFIG option that makes kgdb override the panic
> > request.  This is because the Chrome OS build system makes it very
> > easy for me to add some extra CONFIG "fragments" to my debug kernels.
> >
> > Hopefully having this extra config option is OK and useful to others
> > who would also prefer to make sure that kgdb is always entered on a
> > panic no matter what userspace might request.
> >
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> Sorry to be late with this review. I forgot to search for "debug:" when
> I was checking for missed patches earlier.
>
> Mind you... one of the reasons I deferred review when you first sent it
> in was that my gut reaction was "I don't like it" so I decided to wait
> until I could offer a head reaction instead.
>
> Ultimately I'm not sure this should be solved within kgdb. Perhaps best
> phrased as: is the problem that kgdb *misinterprets* panic_timeout or is
> the problem that Doug wants to *override* panic_timeout?
>
> I think the answer to this question is the later meaning I'm interested
> in what happens if you introduce a CONFIG_PANIC_TIMEOUT_FORCE (c.f.
> CONFIG_CMDLINE_FORCE) to prevent the userspace changing the
> panic_timeout (either by avoiding registering the panic sysctl or, if
> that is a huge ABI problem attaching it to a different variable).
>
> TBH I'm not sure if such a patch would be accepted but I think it makes
> more semantic sense!
>
> (there is a small review comment below but the above is more important)

Thanks for the review.  Yeah, it definitely was a bit of a
questionable patch but I figured I'd throw it out there to see what
folks thought.  I think we should just drop it.  I talked with Brian
about this offline and we agree that it actually should be OK to just
drop the setting from '00-sysctl.conf'.  I have my patch at
crrev.com/c/1382879 and it looks like folks are happy with it.  :-)


-Doug