* [PATCH] seqlock, procfs: Fix scoped_seqlock_read() critical section in do_task_stat()
@ 2025-12-02 4:09 Ingo Molnar
2025-12-02 7:29 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2025-12-02 4:09 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, Oleg Nesterov
RCU read-lock should not nest inside a read-seqlock
irqsave ->stats_lock IRQs-off critical section, but
should go on the outside, like it's done in
fs/proc/base.c:do_io_accounting().
Looks like this was a pre-existing bug dating back to:
7601df8031fd ("fs/proc: do_task_stat: use sig->stats_lock to gather the threads/children stats")
... but this recent commit made it more apparent:
b76f72bea2c6 ("seqlock: Change do_task_stat() to use scoped_seqlock_read()")
To fix it, move the rcu_read_lock() on the outside, and
convert it to a guard(rcu)() construct. Add an extra
depth to the local scope to make sure the RCU critical
section is kept strictly to the intended area.
Fixes: b76f72bea2c6 ("seqlock: Change do_task_stat() to use scoped_seqlock_read()")
Fixes: 7601df8031fd ("fs/proc: do_task_stat: use sig->stats_lock to gather the threads/children stats")
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
fs/proc/array.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd4bc4a58e4..6a35825c45a6 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -537,27 +537,28 @@ 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);
+ {
+ 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();
}
}
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] seqlock, procfs: Fix scoped_seqlock_read() critical section in do_task_stat()
2025-12-02 4:09 [PATCH] seqlock, procfs: Fix scoped_seqlock_read() critical section in do_task_stat() Ingo Molnar
@ 2025-12-02 7:29 ` Oleg Nesterov
2025-12-02 9:05 ` [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting() Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2025-12-02 7:29 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Peter Zijlstra, linux-kernel
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...
> + {
> + guard(rcu)();
> + scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {
how about
scoped_guard(rcu) {
scoped_seqlock_read (&sig->stats_lock, ss_lock_irqsave) {
...
}
}
?
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()
2025-12-02 7:29 ` Oleg Nesterov
@ 2025-12-02 9:05 ` Ingo Molnar
2025-12-02 9:21 ` Oleg Nesterov
2025-12-02 10:23 ` [tip: locking/urgent] " tip-bot2 for Ingo Molnar
0 siblings, 2 replies; 6+ messages in thread
From: Ingo Molnar @ 2025-12-02 9:05 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Peter Zijlstra, linux-kernel
* 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();
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()
2025-12-02 9:05 ` [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting() Ingo Molnar
@ 2025-12-02 9:21 ` Oleg Nesterov
2025-12-02 10:18 ` Ingo Molnar
2025-12-02 10:23 ` [tip: locking/urgent] " tip-bot2 for Ingo Molnar
1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2025-12-02 9:21 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Peter Zijlstra, linux-kernel
On 12/02, Ingo Molnar wrote:
>
> * 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,
OK, thanks ;)
> merely
> discouraged inside irqs-off sections if it can be
> avoided, and it's an inconsistency versus
> do_io_accounting().
Yes, yes, agreed
> How about the -v2 phrasing below? I also removed the
> Fixes tags.
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()
2025-12-02 9:21 ` Oleg Nesterov
@ 2025-12-02 10:18 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2025-12-02 10:18 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Peter Zijlstra, linux-kernel
* Oleg Nesterov <oleg@redhat.com> wrote:
> On 12/02, Ingo Molnar wrote:
> >
> > * 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,
>
> OK, thanks ;)
>
> > merely
> > discouraged inside irqs-off sections if it can be
> > avoided, and it's an inconsistency versus
> > do_io_accounting().
>
> Yes, yes, agreed
>
> > How about the -v2 phrasing below? I also removed the
> > Fixes tags.
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
Thanks Oleg!
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: locking/urgent] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting()
2025-12-02 9:05 ` [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting() Ingo Molnar
2025-12-02 9:21 ` Oleg Nesterov
@ 2025-12-02 10:23 ` tip-bot2 for Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot2 for Ingo Molnar @ 2025-12-02 10:23 UTC (permalink / raw)
To: linux-tip-commits
Cc: Ingo Molnar, Oleg Nesterov, Peter Zijlstra, Linus Torvalds,
Thomas Gleixner, Christian Brauner, Al Viro, x86, linux-kernel
The following commit has been merged into the locking/urgent branch of tip:
Commit-ID: 24bc5ea5c01a7695a1308ac24435810855ec71c9
Gitweb: https://git.kernel.org/tip/24bc5ea5c01a7695a1308ac24435810855ec71c9
Author: Ingo Molnar <mingo@kernel.org>
AuthorDate: Tue, 02 Dec 2025 10:05:10 +01:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 02 Dec 2025 11:21:07 +01:00
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>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Link: https://patch.msgid.link/aS6rwnaPbHFCdHp1@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 cbd4bc4..42932f8 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();
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-12-02 10:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-02 4:09 [PATCH] seqlock, procfs: Fix scoped_seqlock_read() critical section in do_task_stat() Ingo Molnar
2025-12-02 7:29 ` Oleg Nesterov
2025-12-02 9:05 ` [PATCH -v2] seqlock, procfs: Match scoped_seqlock_read() critical section vs. RCU ordering in do_task_stat() to do_io_accounting() Ingo Molnar
2025-12-02 9:21 ` Oleg Nesterov
2025-12-02 10:18 ` Ingo Molnar
2025-12-02 10:23 ` [tip: locking/urgent] " tip-bot2 for Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox