linux-kernel - Re: [PATCH] kdb: Fix compiling on architectures w/out DBG_MAX_REG

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:   Wed, 5 Feb 2020 10:01:17 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Anatoly Pugachev <matorola@...il.com>,
        Sparc kernel list <sparclinux@...r.kernel.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Chuhong Yuan <hslester96@...il.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] kdb: Fix compiling on architectures w/out DBG_MAX_REG_NUM defined

Hi,

On Wed, Feb 5, 2020 at 9:30 AM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> On Tue, Feb 04, 2020 at 02:12:25PM -0800, Douglas Anderson wrote:
> > In commit bbfceba15f8d ("kdb: Get rid of confusing diag msg from "rd"
> > if current task has no regs") I tried to clean things up by using "if"
> > instead of "#ifdef".  Turns out we really need "#ifdef" since not all
> > architectures define some of the structures that the code is referring
> > to.
> >
> > Let's switch to #ifdef again, but at least avoid using it inside of
> > the function.
> >
> > Fixes: bbfceba15f8d ("kdb: Get rid of confusing diag msg from "rd" if current task has no regs")
> > Reported-by: Anatoly Pugachev <matorola@...il.com>
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
>
> Thanks for being so quick with this (especially when if I had been less
> delinquent with linux-next it might have been spotted sooner).
>
>
> > ---
> > I don't have a sparc64 compiler but I'm pretty sure this should work.
> > Testing appreciated.
>
> I've just add sparc64 into my pre-release testing (although I have had to
> turn off a bunch of additional compiler warnings in order to do so).
>
>
> >  kernel/debug/kdb/kdb_main.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index b22292b649c4..c84e61747267 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -1833,6 +1833,16 @@ static int kdb_go(int argc, const char **argv)
> >  /*
> >   * kdb_rd - This function implements the 'rd' command.
> >   */
> > +
> > +/* Fallback to Linux showregs() if we don't have DBG_MAX_REG_NUM */
> > +#if DBG_MAX_REG_NUM <= 0
> > +static int kdb_rd(int argc, const char **argv)
> > +{
> > +     if (!kdb_check_regs())
> > +             kdb_dumpregs(kdb_current_regs);
> > +     return 0;
> > +}
> > +#else
>
> The original kdb_rd (and kdb_rm which still exists in this file) place
> the #if inside the function and users > 0 so the common case was
> covered at the top and the fallback at the bottom.
>
> Why change style when re-introducing this code?

My opinion is that #if / #ifdef leads to hard-to-follow code, so I
have always taken the policy that #if / #ifdef don't belong anywhere
inside a function if it can be avoided.  This seems to be the policy
in Linux in general, though not as much in the existing kgdb code.
IMO kgdb should be working to reduce #if / #ifdef inside functions.

In this case, the duplicated code is 1 line: the call to
kdb_check_regs().  It seemed better to duplicate.  Another option that
would avoid the #if / #ifdef in the function would be as follows.
Happy to change my patch like this if you prefer:

#if DBG_MAX_REG_NUM <= 0
static int _kdb_rd(void)
{
  kdb_dumpregs(kdb_current_regs);
  return 0;
}
#else
static int _kdb_rd(void)
{
 ...
}
#endif

static int kdb_rd(int argc, const char **argv)
{
  if (kdb_check_regs())
    return 0;
  return _kdb_rd();
}

...or if you just want to get something quickly so we have time to
debate the finer points, I wouldn't object to a simple Revert and I
can put it on my plate to resubmit the patch later.

-Doug