Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Zhang, Qiang1" <qiang1.zhang@intel.com>
To:     "paulmck@kernel.org" <paulmck@kernel.org>
CC:     "frederic@kernel.org" <frederic@kernel.org>,
        "quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] rcu: Fix race in set and clear TICK_DEP_BIT_RCU_EXP
 bitmask
Thread-Topic: [PATCH] rcu: Fix race in set and clear TICK_DEP_BIT_RCU_EXP
 bitmask
Thread-Index: AQHZFGUUOrkEkQdHgkyyhC/0ix1REa54x0mAgADkn5A=
Date:   Thu, 22 Dec 2022 09:48:14 +0000
Message-ID: <PH0PR11MB58806135271427BC1A13AF34DAE89@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20221220112520.3596920-1-qiang1.zhang@intel.com>
 <20221221200849.GG4001@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20221221200849.GG4001@paulmck-ThinkPad-P17-Gen-1>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?5pzgTeXrQDWEghve0TGUe3CwQ5NZWbxz0bx3mSITXIQKHS9NCwgn8PxZNG/7?=
 =?us-ascii?Q?MIE2dtQP2asplZpM+7kADlP7I+INJfFcXoa8JDTKdvoq4dgdy7zTD2HKXSvg?=
 =?us-ascii?Q?aFVW4md68ZW/vEXqjtF4dVydmaem70idoI6sq08zDzrVYU2GHTEWerwEN76A?=
 =?us-ascii?Q?76WCDIp6PNIidzlxtgGVPcRDl3KyNQVC4YGFmWkZ9CGthS2AvLXNUSp6V2ng?=
 =?us-ascii?Q?mQdmNQ/yJwRVwLrfUinxz7KP/mBySOnM2VhitCkXPIA5dpGWHf/h+8Ctjzrk?=
 =?us-ascii?Q?D/fewXmlzBzcKpbPXU3mmwQhGV4jhlY/KT9tZpiXTS/i6cCVi7xP6fFR8ld9?=
 =?us-ascii?Q?RwJOPgVr5jFk+vFkkyGAa/6BwHfCqc3U3ojc8mkUX6cGpyGcLWsCuEshmceq?=
 =?us-ascii?Q?gfG4uI0Rgms7xgCnHNs7M7S2iJt1VIIrYZ+6z0ACn9OQ4FrwLh0bOp77gC/D?=
 =?us-ascii?Q?4qMgJS2mR5/xSUlAvidKK/GAToBnKDng+s6COiJZZRjzkQRUAz9jh7vtdOOC?=
 =?us-ascii?Q?NPJbaVvJ9kiMUsI8A2VLx7rfdGxlakjNi/xTwlMgCx30gAXSrgb7ta7OdBiR?=
 =?us-ascii?Q?ljYF6Ym732LywBuAyXgWPPHyOnYeeYL/Z7KyIwxam6beHOpHSJs3bfTtO/2C?=
 =?us-ascii?Q?U/nfX7ky0XVP9+sLW1iysJfVaRoY1G7KLGsEr0KQcy+/7ju3g2AE6Yqs4V/b?=
 =?us-ascii?Q?5g880IpAjkmOIC6ZxKlf26WzXOvF6nl6qJYgqPEQM1y79HIQXu4XPqaKCnW5?=
 =?us-ascii?Q?xux+Xeqja6dBFrU7ZcYsKwPDnjc1CaTlj5cOoPBtrNerG1S+XowDYGojHr7G?=
 =?us-ascii?Q?C8C5Nx2tUNWyHEAD2T+g2Ye5J2QQQqYau3SF+3fRJa6xPVU0ZTXGUwZBv4N8?=
 =?us-ascii?Q?ymrKOQWC+P6Aa+3dNhtLpRAxFSP7I54ICEDz16P04Ui2zGwUk6FR+ZGIz0uv?=
 =?us-ascii?Q?UWW4WN11usb1B4GLHRn7PfYOkEVAsp/SsdS1cYfL8Ya4yPYu1zgosSOreX4Q?=
 =?us-ascii?Q?umV7yJTzkOS0y6YLBX4E2a3Jw1lFv+4jAS3k4ukVhuMF6z44k3OT21DVahYS?=
 =?us-ascii?Q?LsfhTILBJufC9MWs10sbCcsH3moNEV/ZhPo8xRauDpfjeRMpUYXylCiwxlcM?=
 =?us-ascii?Q?REfSLG5Q4I7WBURIDSI2dzDCqR5/tEuHSM1Pu3BJ+6vzLkvFqWpedTcbLa7F?=
 =?us-ascii?Q?UXXJWTGJGvKiCpyU1o/XsjlK2/vVEe1Y9+UgHcJSjw2h7ZUjsaz6oR3rtBzB?=
 =?us-ascii?Q?tXy0yg++yHk8V5i1YkuKWjP88O7ZPYIbDb14UdKNEgIIJnVWIXY6mt15seK0?=
 =?us-ascii?Q?ZMAbxSUfny6KmIPROGIDkEFkk9duzfiaW4Ifhyklg2Wt0cpBE+mU9mc9+BNI?=
 =?us-ascii?Q?W9m07mS1VeZeiMOhYviQGppbHBe/c6FEYO3OyhJZIjR7wJ01BfDF+Hro6xNP?=
 =?us-ascii?Q?Y2v+AH6EvYfjSa+Iv1W4/qhYi8iE601NgYLBxnyJfbFCc+ZfjdOvrZ05IMqx?=
 =?us-ascii?Q?REnAC2P96V+zAH7mp0dG6FpdT8VsKYXOM/peASZZyxLm20tm2W7DThDLVCos?=
 =?us-ascii?Q?UB5BQKy1rOX4YOfCFoMHI6jGz3Xhb1TIPL8qRqJg?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5880.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: deda7f00-9738-4d9a-661d-08dae401a548
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Dec 2022 09:48:14.4963
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: qYn229u0OUxQWZT+cHZkepKlsnBwCJRFDJ4Yv4LGrVahI86AmMshDHtFcHvpoJoWOUcB3D7yaenKrXCNbCyaSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5662
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> For the kernel bulit with CONFIG_NO_HZ_FULL enabled and the following=20
> cpus is nohz_full cpus:
>=20
> 	CPU1                                                 CPU2
> rcu_report_exp_cpu_mult                          synchronize_rcu_expedite=
d_wait
>    acquires rnp->lock                               mask =3D rnp->expmask=
;
>                                                     for_each_leaf_node_cp=
u_mask(rnp, cpu, mask)
>    rnp->expmask =3D rnp->expmask & ~mask;                rdp =3D per_cpu_=
ptr(&rcu_data, cpu1);
>    for_each_leaf_node_cpu_mask(rnp, cpu, mask)
>       rdp =3D per_cpu_ptr(&rcu_data, cpu1);
>       if (!rdp->rcu_forced_tick_exp)
>              continue;                                 rdp->rcu_forced_ti=
ck_exp =3D true;
>                                                       =20
> tick_dep_set_cpu(cpu1, TICK_DEP_BIT_RCU_EXP);
>=20
> In the above scenario, after CPU1 reported the quiescent state, CPU1=20
> misses the opportunity to clear the TICK_DEP_BIT_RCU_EXP bitmask, it=20
> will not be cleared until the next expedited grace period starts and=20
> the CPU1 quiescent state is reported again. during this window period,=20
> the CPU1 whose tick can not be stopped, if CPU1 has only one runnable=20
> task and this task has aggressive real-time response constraints, this=20
> task may have one of the worst response times.
>=20
> Therefore, this commit add rnp->lock when set TICK_DEP_BIT_RCU_EXP=20
> bitmask to fix this race.
>=20
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>Good eyes, thank you!!!
>
>Queued for testing and further review as follows, as always, please check =
for errors.
>

It looks more clear now, thank you!

Thanks
Zqiang

>							Thanx, Paul
>
>------------------------------------------------------------------------

commit acfe689f2e473fb59b6d2c95af5fe36198bb9a84
Author: Zqiang <qiang1.zhang@intel.com>
Date:   Tue Dec 20 19:25:20 2022 +0800

    rcu: Fix set/clear TICK_DEP_BIT_RCU_EXP bitmask race
   =20
    For kernels built with CONFIG_NO_HZ_FULL=3Dy, the following scenario ca=
n result
    in the scheduling-clock interrupt remaining enabled on a holdout CPU af=
ter
    its quiescent state has been reported:
   =20
            CPU1                                                 CPU2
    rcu_report_exp_cpu_mult                          synchronize_rcu_expedi=
ted_wait
       acquires rnp->lock                               mask =3D rnp->expma=
sk;
                                                        for_each_leaf_node_=
cpu_mask(rnp, cpu, mask)
       rnp->expmask =3D rnp->expmask & ~mask;                rdp =3D per_cp=
u_ptr(&rcu_data, cpu1);
       for_each_leaf_node_cpu_mask(rnp, cpu, mask)
          rdp =3D per_cpu_ptr(&rcu_data, cpu1);
          if (!rdp->rcu_forced_tick_exp)
                 continue;                                 rdp->rcu_forced_=
tick_exp =3D true;
                                                           tick_dep_set_cpu=
(cpu1, TICK_DEP_BIT_RCU_EXP);
   =20
    The problem is that CPU2's sampling of rnp->expmask is obsolete by the
    time it invokes tick_dep_set_cpu(), and CPU1 is not guaranteed to see
    CPU2's store to ->rcu_forced_tick_exp in time to clear it.  And even if
    CPU1 does see that store, it might invoke tick_dep_clear_cpu() before
    CPU2 got around to executing its tick_dep_set_cpu(), which would still
    leave the victim CPU with its scheduler-clock tick running.
   =20
    Either way, an nohz_full real-time application running on the victim
    CPU would have its latency needlessly degraded.
   =20
    Note that expedited RCU grace periods look at context-tracking
    information, and so if the CPU is executing in nohz_full usermode
    throughout, that CPU cannot be victimized in this manner.
   =20
    This commit therefore causes synchronize_rcu_expedited_wait to hold
    the rcu_node structure's ->lock when checking for holdout CPUs, setting
    TICK_DEP_BIT_RCU_EXP, and invoking tick_dep_set_cpu(), thus preventing
    this race.
   =20
    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h index 249c2967d9=
e6c..7cc4856da0817 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -594,6 +594,7 @@ static void synchronize_rcu_expedited_wait(void)
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
 	struct rcu_node *rnp_root =3D rcu_get_root();
+	unsigned long flags;
=20
 	trace_rcu_exp_grace_period(rcu_state.name, rcu_exp_gp_seq_endval(), TPS("=
startwait"));
 	jiffies_stall =3D rcu_exp_jiffies_till_stall_check();
@@ -602,17 +603,17 @@ static void synchronize_rcu_expedited_wait(void)
 		if (synchronize_rcu_expedited_wait_once(1))
 			return;
 		rcu_for_each_leaf_node(rnp) {
+			raw_spin_lock_irqsave_rcu_node(rnp, flags);
 			mask =3D READ_ONCE(rnp->expmask);
 			for_each_leaf_node_cpu_mask(rnp, cpu, mask) {
 				rdp =3D per_cpu_ptr(&rcu_data, cpu);
 				if (rdp->rcu_forced_tick_exp)
 					continue;
 				rdp->rcu_forced_tick_exp =3D true;
-				preempt_disable();
 				if (cpu_online(cpu))
 					tick_dep_set_cpu(cpu, TICK_DEP_BIT_RCU_EXP);
-				preempt_enable();
 			}
+			raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 		}
 		j =3D READ_ONCE(jiffies_till_first_fqs);
 		if (synchronize_rcu_expedited_wait_once(j + HZ))