From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56A6ECDB474 for ; Tue, 17 Oct 2023 10:24:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234812AbjJQKYt (ORCPT ); Tue, 17 Oct 2023 06:24:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37952 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343534AbjJQKYp (ORCPT ); Tue, 17 Oct 2023 06:24:45 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 01FFD9F for ; Tue, 17 Oct 2023 03:24:44 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 472E52F4; Tue, 17 Oct 2023 03:25:24 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.68.232]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A2A7A3F762; Tue, 17 Oct 2023 03:24:42 -0700 (PDT) Date: Tue, 17 Oct 2023 11:24:40 +0100 From: Mark Rutland To: Rong Tao Cc: elver@google.com, tglx@linutronix.de, peterz@infradead.org, Rong Tao , open list Subject: Re: [PATCH] stop_machine: Avoid potential race behaviour of multi_stop_data::state Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 17, 2023 at 04:50:52PM +0800, Rong Tao wrote: > From: Rong Tao > > 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 > --- > 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 >