| Message ID | 20201013094938.356837-3-gabriele.paoloni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | improve bust_spinlocks dependability | expand |
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... Lukas -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#108): https://lists.elisa.tech/g/linux-safety/message/108 Mute This Topic: https://lists.elisa.tech/mt/77479837/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
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... Thanks Gab > > Lukas --------------------------------------------------------------------- INTEL CORPORATION ITALIA S.p.A. con unico socio Sede: Milanofiori Palazzo E 4 CAP 20094 Assago (MI) Capitale Sociale Euro 104.000,00 interamente versato Partita I.V.A. e Codice Fiscale 04236760155 Repertorio Economico Amministrativo n. 997124 Registro delle Imprese di Milano nr. 183983/5281/33 Soggetta ad attivita' di direzione e coordinamento di INTEL CORPORATION, USA This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#111): https://lists.elisa.tech/g/linux-safety/message/111 Mute This Topic: https://lists.elisa.tech/mt/77479837/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Gab, On 14/10/2020 13:05, Paoloni, Gabriele 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 >> <snip> >> >> 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... > I am not able to see how this can happen. I think all calls of bust_spinlocks(0) is always after bust_spinlocks(1) has been done. Do you have any particular usecase or any codepath which can make this happen? Like, if 'x' happens then bust_spinlocks(0) will called without a preceding call to bust_spinlocks(1)..
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 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#114): https://lists.elisa.tech/g/linux-safety/message/114 Mute This Topic: https://lists.elisa.tech/mt/77479837/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
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(); } }
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(-)