[PATCH v3 3/4] sched/fair: Use list_for_each_entry_rcu() in print_cfs_stats()

Aaron Tomlin <[email protected]>
Newsgroups gmane.linux.kernel
Message-ID <[email protected]>
In print_cfs_stats(), rq->leaf_cfs_rq_list is traversed using
for_each_leaf_cfs_rq_safe(), which expands to list_for_each_entry_safe().
Although rq->leaf_cfs_rq_list is RCU-protected, list_for_each_entry_safe()
is a non-RCU iteration macro. It dereferences pointer links without
READ_ONCE() and pre-fetches the next pointer.

When a writer concurrently adds a new cfs_rq to the list using
list_add_rcu(), a reader traversing with list_for_each_entry_safe()
lacks READ_ONCE() protection. Without READ_ONCE(), the compiler is free
to re-fetch pointers or reorder instructions. As a result, the reader
can observe a newly inserted cfs_rq's pointer before its internal fields
are fully visible, leading to reading uninitialised data or
dereferencing invalid pointers. This is illustrated below:

           CPU 0                                        CPU 1
    Writer (under rq->lock)                      Reader (print_cfs_stats)
   ---------------------------                  --------------------------
    1. Initialise cfs_rq                         rcu_read_lock()
         cfs_rq->tg = tg
         cfs_rq->load = 1024
    2. list_add_rcu(&cfs_rq->list, ...):         3. for_each_leaf_cfs_rq_safe():
         smp_store_release()                          Reads prev->next (cfs_rq)
         prev->next = cfs_rq           ----->         Missing READ_ONCE()
                                                 4. Reads cfs_rq fields; sees
                                                    garbage data
                                                      cfs_rq->tg
                                                      cfs_rq->load

Fix this by introducing for_each_leaf_cfs_rq_rcu(), which expands to
list_for_each_entry_rcu(). This uses READ_ONCE() during list traversal.

Fixes: 0601267ca673 ("sched/fair: Rewrite list_add_leaf_cfs_rq()")
Reported-by: sashiko-bot <[email protected]>
Signed-off-by: Aaron Tomlin <[email protected]>
---
 kernel/sched/fair.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d78467ec6ee1..25f941ad637a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -412,6 +412,10 @@ static inline void assert_list_leaf_cfs_rq(struct rq *rq)
 	list_for_each_entry_safe(cfs_rq, pos, &rq->leaf_cfs_rq_list,	\
 				 leaf_cfs_rq_list)
 
+#define for_each_leaf_cfs_rq_rcu(rq, cfs_rq)				\
+	list_for_each_entry_rcu(cfs_rq, &(rq)->leaf_cfs_rq_list,	\
+				leaf_cfs_rq_list)
+
 /* Do the two (enqueued) entities belong to the same group ? */
 static inline struct cfs_rq *
 is_same_group(struct sched_entity *se, struct sched_entity *pse)
@@ -497,6 +501,9 @@ static inline void assert_list_leaf_cfs_rq(struct rq *rq)
 #define for_each_leaf_cfs_rq_safe(rq, cfs_rq, pos)	\
 		for (cfs_rq = &rq->cfs, pos = NULL; cfs_rq; cfs_rq = pos)
 
+#define for_each_leaf_cfs_rq_rcu(rq, cfs_rq)	\
+		for (cfs_rq = &rq->cfs; cfs_rq; cfs_rq = NULL)
+
 static inline struct sched_entity *parent_entity(struct sched_entity *se)
 {
 	return NULL;
@@ -15401,10 +15408,10 @@ DEFINE_SCHED_CLASS(fair) = {
 
 void print_cfs_stats(struct seq_file *m, int cpu)
 {
-	struct cfs_rq *cfs_rq, *pos;
+	struct cfs_rq *cfs_rq;
 
 	rcu_read_lock();
-	for_each_leaf_cfs_rq_safe(cpu_rq(cpu), cfs_rq, pos)
+	for_each_leaf_cfs_rq_rcu(cpu_rq(cpu), cfs_rq)
 		print_cfs_rq(m, cpu, cfs_rq);
 	rcu_read_unlock();
 }
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.