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: Safe access to rcu_node structure's->exp_tasks
Thread-Topic: [PATCH] rcu: Safe access to rcu_node structure's->exp_tasks
Thread-Index: AQHZF1d1vL7vqzT7Rk6Qh2aIcidsQa6Q0uoAgACaqqA=
Date:   Fri, 6 Jan 2023 12:58:09 +0000
Message-ID: <PH0PR11MB5880A02CB83F4AC48A99E9F8DAFB9@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20221224052553.263199-1-qiang1.zhang@intel.com>
 <20230106034146.GM4028633@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20230106034146.GM4028633@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?+f9vvApkmVOceHzTLNh9CzOG17ZxeEF8j/GMHnDzUsLBc9bjG5nMTlWzWdR9?=
 =?us-ascii?Q?qFxKVvtjfXo/FBDEJLXQMNHZh7TWWkepUNi9QT7p9g2iA0EkeGVB30fNqA5z?=
 =?us-ascii?Q?5Ehvc6EmYS2A1aYvgGvDIC9f62HsEEojibtG0/7MejdZcuUNkbLaG0XMRC+f?=
 =?us-ascii?Q?REPaX6vSM92GHWMoW+/IcSf28U0tULhVS2rlKBIShfErAXmxgYMS8rHykg4s?=
 =?us-ascii?Q?wJ486s6P4H4NkAyvZ6gUpiVpZnRLn5VC0RBTPjdGlDax7v5tpA4N98ebp1XW?=
 =?us-ascii?Q?4Q9rVwYOweD6kLiZtFwP6kQ3Mw2At6GGA9fRtU0VhNFHAM3R1p6HWwAyCLoJ?=
 =?us-ascii?Q?t/2gUfJwia2FHtr53ooE17HrLi6e1rDAe+vnGa+MKnRo69WdmkdWuUBHAZJG?=
 =?us-ascii?Q?DeTfpBgFDH4QQ4OGlvMVPP2Y0DvhConW6DYEX6FFWyyjzcOFgCtf0nMJObNI?=
 =?us-ascii?Q?+5hAkGGdvf5/JkE/kjgrjME6aG7GtVI/6OUbbE9WGd9OQD1Fd3KKCaDUKkxJ?=
 =?us-ascii?Q?npkEY/1oJWloeFIxUo7WeYjPoWIBeegUpPFgQC/UAsFWT1cq0ET0Z14Ezyu7?=
 =?us-ascii?Q?ZsFFIurNaSVQzBLsiHAc0T2ctvj60tkGBmqm9Eo28jFULHthxo0tD5QJJcZE?=
 =?us-ascii?Q?TLJVqiltkfuhfW8ZQbgBcmv+kpAWFATiLfQVcuDP0nd4TtYbuwlU6IYn5duK?=
 =?us-ascii?Q?p3EqErqMt1ODa08aL+lBEkaIjMCS/2CZr38ZsWHNAhPMbXzDWli+CEAIvyuS?=
 =?us-ascii?Q?52vshQhThLyTV1/a3SGa3EUFAAVDJ767kxZBnkD/nfyVZwrkv5PFV4SFiHz8?=
 =?us-ascii?Q?E7Bq3XgX4NlmWBfcHPeEY6F44wt44FRlDtajdGvVzp0UCiD412zXo+rctrtm?=
 =?us-ascii?Q?HWBPKqulzBoS5XjwE3c4xupx17se84HpCbznr9gD+JzzJdFdUqvpr0aVtHoA?=
 =?us-ascii?Q?IO32IAm0jW8fjyz2Ix32mmrmYp6+kUZblIRvAKZGQukYPQD9WwdEWn1K+xKb?=
 =?us-ascii?Q?5Y4whPUl7EuKdigS3TOgM9ByE7AYRGEhOdfAGVpA+KfRj5NkaawT9KRpvAfG?=
 =?us-ascii?Q?dtwJI6/uMCPSBVYndzyiaP2Oz+/dzwP9ZDJnILmog+Pp5tmVO6z+ySLqFH3w?=
 =?us-ascii?Q?8FzpYGh4zhpPN5c84rIE/y+6GucjZS5kJCv5gRLIMgKOBpK0M4LVmkI9nBPk?=
 =?us-ascii?Q?yW3eVJLgkMBdHxfCrqbWzKgTvq/RzpHALR4FF//Dgjh7bQSDI/36EOBa1vNS?=
 =?us-ascii?Q?L1VVFzqxr9ndjtXNSKOl/kt+X84njbWG8cluuPGgBd3XXNoXbrLt/p77xOjY?=
 =?us-ascii?Q?fNSO2eBk5QC0BiuktlTFJ6u0Zim8ts7CNLAZmhX2Bt3uV+0r9b5VKxGu3UKM?=
 =?us-ascii?Q?qAjx+2QZp/FnMZ/vs++kk1hxKYPq3edZOWJ8nKU/QSohtizAoONO0vkK89Mc?=
 =?us-ascii?Q?JSi4e61+GnMogbVXAVHOKAySvtK7orGN0yZa5q8qTlW4IKnxPgFWJrC/IbMJ?=
 =?us-ascii?Q?bhBPNIk8iWcgCGutE2E4h9xUVe5q1nBzZBSXxXUfI7gW43IeMHeZdA3enrou?=
 =?us-ascii?Q?8rswx4kdZjVkJ4MZLpmVMMZ2rKjsOrmy5Mez7g4N?=
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: 96992f40-9963-4a39-ce0b-08daefe5a9a1
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jan 2023 12:58:09.8693
 (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: CuVqgRcNVywSWPbgT3HSxAWbNLKeKItVrAMUh7fwjBT+ATyU3ahL/rDmXvB/0u6ea6eThX7ggp0kxrzOIk8h1g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6236
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,
        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 kernels built with CONFIG_PREEMPT_RCU=3Dy, the following scenario
> can result system oops.
>=20
>            CPU1                                           CPU2
> rcu_preempt_deferred_qs_irqrestore                rcu_print_task_exp_stal=
l
>   if (special.b.blocked)                            READ_ONCE(rnp->exp_ta=
sks) !=3D NULL
>     raw_spin_lock_rcu_node
>     np =3D rcu_next_node_entry(t, rnp)
>     if (&t->rcu_node_entry =3D=3D rnp->exp_tasks)
>       WRITE_ONCE(rnp->exp_tasks, np)
>       ....
>       raw_spin_unlock_irqrestore_rcu_node
>                                                     raw_spin_lock_irqsave=
_rcu_node
>                                                     t =3D list_entry(rnp-=
>exp_tasks->prev,
>                                                         struct task_struc=
t, rcu_node_entry)
>                                                     (if rnp->exp_tasks is=
 NULL
>                                                        will trigger oops)
>=20
> This problem is that CPU2 accesses rcu_node structure's->exp_tasks
> without holding the rcu_node structure's ->lock and CPU2 did not
> observe CPU1's change to rcu_node structure's->exp_tasks in time,
> if rcu_node structure's->exp_tasks is set null pointer by CPU1, after
> that CPU2 accesses members of rcu_node structure's->exp_tasks will
> trigger oops.
>=20
> This commit therefore allows rcu_node structure's->exp_tasks to be
> accessed while holding rcu_node structure's ->lock.
>=20
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>Apologies for the delay and thank you for the reminder!
>
>Please check the wordsmithed version below, which I have queued.


Thanks for wordsmithed, this description is more clear.

Thanks
Zqiang


>
>						Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit 389b0eafd72829fd63548f7ff4e8d6ac90fa1f98
>Author: Zqiang <qiang1.zhang@intel.com>
>Date:   Sat Dec 24 13:25:53 2022 +0800
>
>    rcu: Protect rcu_print_task_exp_stall() ->exp_tasks access
>   =20
>    For kernels built with CONFIG_PREEMPT_RCU=3Dy, the following scenario =
can
>    result in a NULL-pointer dereference:
>   =20
>               CPU1                                           CPU2
>    rcu_preempt_deferred_qs_irqrestore                rcu_print_task_exp_s=
tall
>      if (special.b.blocked)                            READ_ONCE(rnp->exp=
_tasks) !=3D NULL
>        raw_spin_lock_rcu_node
>        np =3D rcu_next_node_entry(t, rnp)
>        if (&t->rcu_node_entry =3D=3D rnp->exp_tasks)
>          WRITE_ONCE(rnp->exp_tasks, np)
>          ....
>          raw_spin_unlock_irqrestore_rcu_node
>                                                        raw_spin_lock_irqs=
ave_rcu_node
>                                                        t =3D list_entry(r=
np->exp_tasks->prev,
>                                                            struct task_st=
ruct, rcu_node_entry)
>                                                        (if rnp->exp_tasks=
 is NULL, this
>                                                           will dereferenc=
e a NULL pointer)
>   =20
>    The problem is that CPU2 accesses the rcu_node structure's->exp_tasks
>    field without holding the rcu_node structure's ->lock and CPU2 did
>    not observe CPU1's change to rcu_node structure's ->exp_tasks in time.
>    Therefore, if CPU1 sets rcu_node structure's->exp_tasks pointer to NUL=
L,
>    then CPU2 might dereference that NULL pointer.
>   =20
>    This commit therefore holds the rcu_node structure's ->lock while
>    accessing that structure's->exp_tasks field.
>   =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 7cc4856da0817..902e7c8709c7e 100644
>--- a/kernel/rcu/tree_exp.h
>+++ b/kernel/rcu/tree_exp.h
>@@ -803,9 +803,11 @@ static int rcu_print_task_exp_stall(struct rcu_node *=
rnp)
> 	int ndetected =3D 0;
> 	struct task_struct *t;
>=20
>-	if (!READ_ONCE(rnp->exp_tasks))
>-		return 0;
> 	raw_spin_lock_irqsave_rcu_node(rnp, flags);
>+	if (!READ_ONCE(rnp->exp_tasks)) {
>+		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>+		return 0;
>+	}
> 	t =3D list_entry(rnp->exp_tasks->prev,
> 		       struct task_struct, rcu_node_entry);
> 	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry) {