Re: [linux-safety] [RFC PATCH 2/2] bust_spinlocks: do not decrement oops_in_progress unconditionally

From: "Lukas Bulwahn" <lukas.bulwahn@gmail.com>
To: "Paoloni, Gabriele" <gabriele.paoloni@intel.com>
Cc: "linux-safety@lists.elisa.tech" <linux-safety@lists.elisa.tech>
Subject: Re: [linux-safety] [RFC PATCH 2/2] bust_spinlocks: do not decrement oops_in_progress unconditionally
Date: Thu, 15 Oct 2020 08:44:35 +0200	[thread overview]
Message-ID: <CAKXUXMwb3c58Qc1vYQe6WsddUAnKgQmV2vhM1TuiQpEZtWmpSQ@mail.gmail.com> (raw)
In-Reply-To: <MN2PR11MB4158AB45198CAE0BD2EBFA0188050@MN2PR11MB4158.namprd11.prod.outlook.com>

On Wed, Oct 14, 2020 at 2:05 PM Paoloni, Gabriele
<gabriele.paoloni@intel.com> wrote:
>
> Hi Lukas
>
> > -----Original Message-----
> > From: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Sent: Wednesday, October 14, 2020 7:53 AM
> > To: Paoloni, Gabriele <gabriele.paoloni@intel.com>
> > Cc: linux-safety@lists.elisa.tech
> > Subject: Re: [linux-safety] [RFC PATCH 2/2] bust_spinlocks: do not
> > decrement oops_in_progress unconditionally
> >
> >
> >
> > On Tue, 13 Oct 2020, Paoloni, Gabriele wrote:
> >
> > > In the current implementation if the input flag is 0
> > > oops_in_progress is unconditionally decremented, thus allowing
> > > to become a negative number. Since right now oops_in_progress
> > > is a global variable used in the kernel as a conditional flag
> > > to check if oops, panic(), BUG() or die() is in progress the
> > > current unconditional decrement may lead to unexpected behavior
> > > in the Kernel paths conditionally executing over this flag.
> > >
> > > This patch only decrement oops_in_progress if it is non zero
> > >
> > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@intel.com>
> > > ---
> > >  lib/bust_spinlocks.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/bust_spinlocks.c b/lib/bust_spinlocks.c
> > > index 594b270161d9..842633ac9130 100644
> > > --- a/lib/bust_spinlocks.c
> > > +++ b/lib/bust_spinlocks.c
> > > @@ -23,6 +23,9 @@
> > >   * @yes: input flag; if zero decreases oops_in_progress,
> > >   * otherwise increases it.
> > >   *
> > > + * Note: if oops_in_progress is already 0 it will not
> > > + * be decreased
> > > + *
> > >   */
> > >  void bust_spinlocks(int yes)
> > >  {
> > > @@ -33,7 +36,9 @@ void bust_spinlocks(int yes)
> > >             unblank_screen();
> > >  #endif
> > >             console_unblank();
> > > -           if (--oops_in_progress == 0)
> > > +           if (oops_in_progress)
> > > +                   oops_in_progress--;
> > > +           if (!oops_in_progress)
> > >                     wake_up_klogd();
> >
> > I did not get the original motivation stated above.
> >
> > But I believe you meant:
> >
> > 'I think there is race condition here (before this patch).'
> >
> > So do something in this patch: 'And now the race condition is gone?'
> >
> >
> > I think:
> >
> > 'The could be a race condition before, and probably the race condition is
> > still there after this patch.'
> >
> > But maybe I did even get the intent of this patch in the first place...
>
> What I meant is the following scenario:
> Let's assume oops_in_progress = 0, then we have
>
> func_a()
> {
>         bust_spinlocks(0);
> }
>
> In this case after the call, with the current implementation oops_in_progress = -1; that is not acceptable...
>

Okay, but that is just the contract of this bust_spinlocks() function, right?

As you wrote every caller must call bust_spinlocks(1) and ONLY then
when they are done bust_spinlocks(0) [if the machine has not
halted...].

Maybe if the functions would be bust_spinlocks_{en,dis}able() or
bust_spinlocks_{start,stop}() the contract is more clear. But in the
end, there are only a few users as far as see, in fault, panic, etc.

Of course, if you call bust_spinlocks_stop() before start() bad things
happen... use a static analyzer/model checker to see that that pattern
never appears :)

As I said, the patch looks good; I am looking forward to the feedback.

Lukas