[PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()

[Ingo Molnar <mingo@kernel.org>]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 12/02, Ingo Molnar wrote:
> >
> > RCU read-lock should not nest inside a read-seqlock
> > irqsave ->stats_lock IRQs-off critical section,
> 
> Hmm... I agree with this patch, but is it actually wrong?
> 
> I thought that rcu_read_lock/unlock is safe under spin_lock_irq...

Yeah, true - it's allowed and not a bug, merely 
discouraged inside irqs-off sections if it can be 
avoided, and it's an inconsistency versus 
do_io_accounting().

How about the -v2 phrasing below? I also removed the 
Fixes tags.

Thanks,

	Ingo

===================================>
From: Ingo Molnar <mingo@kernel.org>
Date: Tue, 2 Dec 2025 05:09:28 +0100
Subject: [PATCH] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()

There's two patterns of taking the RCU read-lock and the
sig->stats_lock read-seqlock in do_task_stat() and
do_io_accounting(), with a different ordering:

	# do_io_accounting():

	guard(rcu)();
	scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {

	# do_task_stat():

	scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {
	...
			rcu_read_lock();

The ordering is RCU-read+seqlock_read in the first
case, seqlock_read+RCU-read in the second case.

While technically these read locks can be taken in any order,
nevertheless it's good practice to use the more intrusive lock
on the inside (which is the IRQs-off section in this case),
and reduces head-scratching during review when done consistently,
so let's use the do_io_accounting() pattern in do_task_stat().

This will also reduce irqs-off latencies in do_task_stat() a tiny bit.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://patch.msgid.link/aS5mdHYhHi9Gi5-r@gmail.com
---
 fs/proc/array.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd4bc4a58e4..42932f88141a 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -537,27 +537,27 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	if (permitted && (!whole || num_threads < 2))
 		wchan = !task_is_running(task);
 
-	scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {
-		cmin_flt = sig->cmin_flt;
-		cmaj_flt = sig->cmaj_flt;
-		cutime = sig->cutime;
-		cstime = sig->cstime;
-		cgtime = sig->cgtime;
-
-		if (whole) {
-			struct task_struct *t;
-
-			min_flt = sig->min_flt;
-			maj_flt = sig->maj_flt;
-			gtime = sig->gtime;
-
-			rcu_read_lock();
-			__for_each_thread(sig, t) {
-				min_flt += t->min_flt;
-				maj_flt += t->maj_flt;
-				gtime += task_gtime(t);
+	scoped_guard(rcu) {
+		scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {
+			cmin_flt = sig->cmin_flt;
+			cmaj_flt = sig->cmaj_flt;
+			cutime = sig->cutime;
+			cstime = sig->cstime;
+			cgtime = sig->cgtime;
+
+			if (whole) {
+				struct task_struct *t;
+
+				min_flt = sig->min_flt;
+				maj_flt = sig->maj_flt;
+				gtime = sig->gtime;
+
+				__for_each_thread(sig, t) {
+					min_flt += t->min_flt;
+					maj_flt += t->maj_flt;
+					gtime += task_gtime(t);
+				}
 			}
-			rcu_read_unlock();
 		}
 	}