[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZQsFjN5euQgt5CI7@smile.fi.intel.com>
Date: Wed, 20 Sep 2023 17:45:32 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Petr Mladek <pmladek@...e.com>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
Luis Chamberlain <mcgrof@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Peter Zijlstra <peterz@...radead.org>,
Josh Poimboeuf <jpoimboe@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
"Guilherme G. Piccoli" <gpiccoli@...lia.com>
Subject: Re: [PATCH printk v2 09/11] panic: Add atomic write enforcement to
oops
On Wed, Sep 20, 2023 at 04:26:12PM +0206, John Ogness wrote:
> On 2023-09-20, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
> > On Wed, Sep 20, 2023 at 01:14:54AM +0206, John Ogness wrote:
...
> >> + if (atomic_read(&oops_cpu) == smp_processor_id()) {
> >> + oops_nesting--;
> >> + if (oops_nesting == 0) {
> >> + atomic_set(&oops_cpu, -1);
> >
> > Between read and set the variable can change, can't it?
>
> CPU migration is disabled. @oops_cpu contains the CPU ID of the only CPU
> that is printing the oops. (Perhaps the variable should be called
> "oops_printing_cpu"?)
>
> If this matches smp_processor_id(), then the current CPU is the only one
> that is allowed to change it back to -1. So no, if the first condition
> is true, it cannot change before atomic_set(). And if the second
> condition is true, this is the only CPU+context that is allowed to
> change it back to -1;
>
> > If not, why this variable is atomic then? Or, why it's not a problem?
> > If the latter is the case, perhaps a comment to explain this?
>
> If not atomic, it will be a data race since one CPU might be changing
> @oops_cpu and another is reading it. For type "int" such a data race
> would be fine because it doesn't matter which side of the race the
> reader was on, both values will not match the current CPU ID.
>
> The reason that I didn't implement it using cmpxchg(),
> data_race(READ_ONCE()), and WRITE_ONCE() is because I once learned that
> you should never mix cmpxchg() with READ_ONCE()/WRITE_ONCE() because
> there are architectures that do not support cmpxchg() as an atomic
> instruction. The answer was always: "use atomic_t instead... that is
> what it is for".
>
> But AFAICT for this case it would be fine because obviously cmpxchg()
> will not race with itself. And successfully reading a matching CPU ID
> means there cannot be any cmpxchg() in progress. And writing only occurs
> after seeing a matching CPU ID.
>
> So I can change it from atomic_t to int. Although I do feel like that
> might require explanation about why the data race is safe.
Either way a comment is needed, but I think the usage of atomic above
is a bit confusing as you see I immediately rose the concern.
> Or perhaps it is enough just to have something like this:
>
> /**
> * oops_printing_cpu - The ID of the CPU responsible for printing the
> * OOPS message(s) to the consoles.
> *
> * This is atomic_t because multiple CPUs can read this variable
> * simultaneously when exiting OOPS while another CPU can be
> * modifying this variable to begin or end its printing duties.
> */
> static atomic_t oops_printing_cpu = ATOMIC_INIT(-1);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists