Messages in this thread | | | Date | Tue, 17 Oct 2023 11:24:40 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH] stop_machine: Avoid potential race behaviour of multi_stop_data::state |
| |
On Tue, Oct 17, 2023 at 04:50:52PM +0800, Rong Tao wrote: > From: Rong Tao <rongtao@cestc.cn> > > In commit b1fc58333575 ("stop_machine: Avoid potential race behaviour") > fix both multi_cpu_stop() and set_state() access multi_stop_data::state, > We should ensure that multi_stop_data::state is accessed using the rwonce > method. > > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > kernel/stop_machine.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c > index cedb17ba158a..73de9ab77132 100644 > --- a/kernel/stop_machine.c > +++ b/kernel/stop_machine.c > @@ -191,7 +191,7 @@ static void set_state(struct multi_stop_data *msdata, > static void ack_state(struct multi_stop_data *msdata) > { > if (atomic_dec_and_test(&msdata->thread_ack)) > - set_state(msdata, msdata->state + 1); > + set_state(msdata, READ_ONCE(msdata->state) + 1);
IIUC this is bening, as the state machine only ever has a single writer to msdata->state (though which thread is the writer may change per iteration of the loop).
At this point we know that the active thread *is* the writer, and so no other threads can write to msdata->state, so there is no race and reading that non-atomically is fine.
I'm not opposed to making this use READ_ONCE(), but I don't think that it's strictly necessary to do so.
That said, if we really want to avoid the non-atomic read, it's probably better to have multi_cpu_stop() pass curstate as a paramter to ack_state. That or fold ack_state() into multi_cpu_stop() and use curstate directly.
Mark.
> } > > notrace void __weak stop_machine_yield(const struct cpumask *cpumask) > -- > 2.42.0 >
| |