[PATCH] pid: use READ_ONCE() in pid_alive()

[PATCH] pid: use READ_ONCE() in pid_alive()

[Babanpreet Singh]

KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
WRITE_ONCE() are enough.

Reported-by: syzbot+c382ee653fd70f5cf1bb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c382ee653fd70f5cf1bb
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
Compile tested only (gcc W=1 and the KCSAN instrumentation diff); I
could not reproduce the race in QEMU.

 include/linux/pid.h | 2 +-
 kernel/pid.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index ddaef0bbc8ba3..05a0084dc9537 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -264,7 +264,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
  */
 static inline int pid_alive(const struct task_struct *p)
 {
-	return p->thread_pid != NULL;
+	return READ_ONCE(p->thread_pid) != NULL;
 }
 
 static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
diff --git a/kernel/pid.c b/kernel/pid.c
index 95b8ccfa82690..adf7216067684 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -411,7 +411,7 @@ static void __change_pid(struct pid **pids, struct task_struct *task,
 	pid = *pid_ptr;
 
 	hlist_del_rcu(&task->pid_links[type]);
-	*pid_ptr = new;
+	WRITE_ONCE(*pid_ptr, new);
 
 	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
 		if (pid_has_task(pid, tmp))

base-commit: ed14a591175bb5f56c2936b082cffb9e4b935e6d
-- 
2.43.0

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[Bradley Morgan]

On 2 October 2026 02:21:41 BST, Babanpreet Singh <bbnpreetsingh@gmail.com>
wrote:
>KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
>under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
>WRITE_ONCE() are enough.
>
>Reported-by: syzbot+c382ee653fd70f5cf1bb@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=c382ee653fd70f5cf1bb
>Assisted-by: Claude:claude-opus-5-5

Coolio

Reviewed-by: Bradley Morgan <brads@mainlining.org>


>Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
>---
>Compile tested only (gcc W=1 and the KCSAN instrumentation diff); I
>could not reproduce the race in QEMU.
>
> include/linux/pid.h | 2 +-
> kernel/pid.c        | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/pid.h b/include/linux/pid.h
>index ddaef0bbc8ba3..05a0084dc9537 100644
>--- a/include/linux/pid.h
>+++ b/include/linux/pid.h
>@@ -264,7 +264,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
>  */
> static inline int pid_alive(const struct task_struct *p)
> {
>-	return p->thread_pid != NULL;
>+	return READ_ONCE(p->thread_pid) != NULL;
> }
> 
> static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk, struct
> pid_namespace *ns)
>diff --git a/kernel/pid.c b/kernel/pid.c
>index 95b8ccfa82690..adf7216067684 100644
>--- a/kernel/pid.c
>+++ b/kernel/pid.c
>@@ -411,7 +411,7 @@ static void __change_pid(struct pid **pids, struct task_struct *task,
> 	pid = *pid_ptr;
> 
> 	hlist_del_rcu(&task->pid_links[type]);
>-	*pid_ptr = new;
>+	WRITE_ONCE(*pid_ptr, new);
> 
> 	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
> 		if (pid_has_task(pid, tmp))
>
>base-commit: ed14a591175bb5f56c2936b082cffb9e4b935e6d
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[Oleg Nesterov]

On 10/02, Babanpreet Singh wrote:
>
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -264,7 +264,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
>   */
>  static inline int pid_alive(const struct task_struct *p)
>  {
> -	return p->thread_pid != NULL;
> +	return READ_ONCE(p->thread_pid) != NULL;
>  }
>
>  static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 95b8ccfa82690..adf7216067684 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -411,7 +411,7 @@ static void __change_pid(struct pid **pids, struct task_struct *task,
>  	pid = *pid_ptr;
>
>  	hlist_del_rcu(&task->pid_links[type]);
> -	*pid_ptr = new;
> +	WRITE_ONCE(*pid_ptr, new);

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[David Laight]

On Fri,  2 Oct 2026 01:21:41 +0000
Babanpreet Singh <bbnpreetsingh@gmail.com> wrote:

> KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
> under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
> WRITE_ONCE() are enough.

I just looked at change_pid() - isn't it completely broken?
__change_pid() uses hlist_del_rcu() to remove the item from a list.
IIUC this leaves the 'next' pointer valid to allow for concurrent readers.
I thought that had to stay valid until the end of the rcu period.
But the following attach_pid() adds the item to another list.
I think that means that a concurrent reader can switch lists and thus
fail to find an item.
This could be (mostly) mitigated by using the 'nulls' variant which
lets the reading code detect the crossed lists and rescan.

(I've not checked the history...)

David


> 
> Reported-by: syzbot+c382ee653fd70f5cf1bb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c382ee653fd70f5cf1bb
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
> ---
> Compile tested only (gcc W=1 and the KCSAN instrumentation diff); I
> could not reproduce the race in QEMU.
> 
>  include/linux/pid.h | 2 +-
>  kernel/pid.c        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index ddaef0bbc8ba3..05a0084dc9537 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -264,7 +264,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
>   */
>  static inline int pid_alive(const struct task_struct *p)
>  {
> -	return p->thread_pid != NULL;
> +	return READ_ONCE(p->thread_pid) != NULL;
>  }
>  
>  static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 95b8ccfa82690..adf7216067684 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -411,7 +411,7 @@ static void __change_pid(struct pid **pids, struct task_struct *task,
>  	pid = *pid_ptr;
>  
>  	hlist_del_rcu(&task->pid_links[type]);
> -	*pid_ptr = new;
> +	WRITE_ONCE(*pid_ptr, new);
>  
>  	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
>  		if (pid_has_task(pid, tmp))
> 
> base-commit: ed14a591175bb5f56c2936b082cffb9e4b935e6d

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[Oleg Nesterov]

On 10/03, David Laight wrote:
>
> On Fri,  2 Oct 2026 01:21:41 +0000
> Babanpreet Singh <bbnpreetsingh@gmail.com> wrote:
>
> > KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
> > under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
> > WRITE_ONCE() are enough.
>
> I just looked at change_pid() - isn't it completely broken?

No, but...

> __change_pid() uses hlist_del_rcu() to remove the item from a list.
> IIUC this leaves the 'next' pointer valid to allow for concurrent readers.
> I thought that had to stay valid until the end of the rcu period.
> But the following attach_pid() adds the item to another list.

Yep. That is why do_each_pid_task() needs tasklist_lock.

This is the known fact, let me quote the part of my old email
https://lore.kernel.org/all/20200512150936.GA28621@redhat.com/

	> Currently the tasklist_lock is shared mainly in order to observe
	> the list atomically for the PRIO_PGRP and PRIO_USER cases, as
	> the actual lookups are already rcu-safe,

	not really...

	do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
	which moves the task from one hlist to another. Yes, it is safe in
	that task_struct can't go away. But still this is not right because
	do_each_pid_task() can scan the wrong (2nd) hlist.

Somehow I thought this was documented, but it isn't. And this is not obvious.
I think this deserves a comment above do_each_pid_task(), will send the patch.

Oleg.

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[Oleg Nesterov]

Hmm. grep finds send_sigio() and send_sigurg() which use do_each_pid_task()
without tasklist.

00633c4683828acd ("fs/fcntl: fix SOFTIRQ-unsafe lock order in fasync signaling")
is wrong.

I'll send email in reply to that patch, I wasn't CC'ed...

Oleg.

On 10/04, Oleg Nesterov wrote:
>
> On 10/03, David Laight wrote:
> >
> > On Fri,  2 Oct 2026 01:21:41 +0000
> > Babanpreet Singh <bbnpreetsingh@gmail.com> wrote:
> >
> > > KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
> > > under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
> > > WRITE_ONCE() are enough.
> >
> > I just looked at change_pid() - isn't it completely broken?
>
> No, but...
>
> > __change_pid() uses hlist_del_rcu() to remove the item from a list.
> > IIUC this leaves the 'next' pointer valid to allow for concurrent readers.
> > I thought that had to stay valid until the end of the rcu period.
> > But the following attach_pid() adds the item to another list.
>
> Yep. That is why do_each_pid_task() needs tasklist_lock.
>
> This is the known fact, let me quote the part of my old email
> https://lore.kernel.org/all/20200512150936.GA28621@redhat.com/
>
> 	> Currently the tasklist_lock is shared mainly in order to observe
> 	> the list atomically for the PRIO_PGRP and PRIO_USER cases, as
> 	> the actual lookups are already rcu-safe,
>
> 	not really...
>
> 	do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
> 	which moves the task from one hlist to another. Yes, it is safe in
> 	that task_struct can't go away. But still this is not right because
> 	do_each_pid_task() can scan the wrong (2nd) hlist.
>
> Somehow I thought this was documented, but it isn't. And this is not obvious.
> I think this deserves a comment above do_each_pid_task(), will send the patch.
>
> Oleg.

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[David Laight]

On Sun, 4 Oct 2026 12:55:34 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 10/03, David Laight wrote:
> >
> > On Fri,  2 Oct 2026 01:21:41 +0000
> > Babanpreet Singh <bbnpreetsingh@gmail.com> wrote:
> >  
> > > KCSAN reports pid_alive() reading task->thread_pid while it gets cleared
> > > under tasklist_lock. The check only cares about NULL, so READ_ONCE() and
> > > WRITE_ONCE() are enough.  
> >
> > I just looked at change_pid() - isn't it completely broken?  
> 
> No, but...
> 
> > __change_pid() uses hlist_del_rcu() to remove the item from a list.
> > IIUC this leaves the 'next' pointer valid to allow for concurrent readers.
> > I thought that had to stay valid until the end of the rcu period.
> > But the following attach_pid() adds the item to another list.  
> 
> Yep. That is why do_each_pid_task() needs tasklist_lock.
> 
> This is the known fact, let me quote the part of my old email
> https://lore.kernel.org/all/20200512150936.GA28621@redhat.com/
> 
> 	> Currently the tasklist_lock is shared mainly in order to observe
> 	> the list atomically for the PRIO_PGRP and PRIO_USER cases, as
> 	> the actual lookups are already rcu-safe,  
> 
> 	not really...
> 
> 	do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
> 	which moves the task from one hlist to another. Yes, it is safe in
> 	that task_struct can't go away. But still this is not right because
> 	do_each_pid_task() can scan the wrong (2nd) hlist.
> 
> Somehow I thought this was documented, but it isn't. And this is not obvious.
> I think this deserves a comment above do_each_pid_task(), will send the patch.

I guess the rcu protection lets the task exit without holding the lock?
Is that really significant given the other things that happen during task exit.

Could do_each_pid_task() use hlist_nulls_for_each_entry_rcu() and rescan
if it got the wrong terminator.
Or does scanning twice cause grief as well.

David

> 
> Oleg.
> 

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[Oleg Nesterov]

On 10/04, David Laight wrote:
>
> On Sun, 4 Oct 2026 12:55:34 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Yep. That is why do_each_pid_task() needs tasklist_lock.
> >
> > This is the known fact, let me quote the part of my old email
> > https://lore.kernel.org/all/20200512150936.GA28621@redhat.com/
> >
> > 	> Currently the tasklist_lock is shared mainly in order to observe
> > 	> the list atomically for the PRIO_PGRP and PRIO_USER cases, as
> > 	> the actual lookups are already rcu-safe,
> >
> > 	not really...
> >
> > 	do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
> > 	which moves the task from one hlist to another. Yes, it is safe in
> > 	that task_struct can't go away. But still this is not right because
> > 	do_each_pid_task() can scan the wrong (2nd) hlist.
> >
> > Somehow I thought this was documented, but it isn't. And this is not obvious.
> > I think this deserves a comment above do_each_pid_task(), will send the patch.
>
> I guess the rcu protection lets the task exit without holding the lock?
> Is that really significant given the other things that happen during task exit.

Sorry, I don't understand your question...

> Could do_each_pid_task() use hlist_nulls_for_each_entry_rcu() and rescan
> if it got the wrong terminator.
> Or does scanning twice cause grief as well.

I don't think it can. Say, __kill_pgrp_info() is a "typical" user of
do_each_pid_task(). What can it do if it detects that get_nulls_value()
doesn't match after the main loop? The signal was already sent.

Oleg.

Re: [PATCH] pid: use READ_ONCE() in pid_alive()

[David Laight]

On Sun, 4 Oct 2026 15:14:26 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 10/04, David Laight wrote:
> >
> > On Sun, 4 Oct 2026 12:55:34 +0200
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >  
> > > Yep. That is why do_each_pid_task() needs tasklist_lock.
> > >
> > > This is the known fact, let me quote the part of my old email
> > > https://lore.kernel.org/all/20200512150936.GA28621@redhat.com/
> > >  
> > > 	> Currently the tasklist_lock is shared mainly in order to observe
> > > 	> the list atomically for the PRIO_PGRP and PRIO_USER cases, as
> > > 	> the actual lookups are already rcu-safe,  
> > >
> > > 	not really...
> > >
> > > 	do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
> > > 	which moves the task from one hlist to another. Yes, it is safe in
> > > 	that task_struct can't go away. But still this is not right because
> > > 	do_each_pid_task() can scan the wrong (2nd) hlist.
> > >
> > > Somehow I thought this was documented, but it isn't. And this is not obvious.
> > > I think this deserves a comment above do_each_pid_task(), will send the patch.  
> >
> > I guess the rcu protection lets the task exit without holding the lock?
> > Is that really significant given the other things that happen during task exit.  
> 
> Sorry, I don't understand your question...

I was wondering if the (partial) rcu protection of these lists was worth
the trouble.
If the 'add code' all the readers and have to hold the lock then does that
leave anything other than task exit doing an rcu-delete.
I wouldn't have though acquiring the lock in the task exit code would
be noticeable.
Is there some other path where rcu protection is 'good enough'?
 
> 
> > Could do_each_pid_task() use hlist_nulls_for_each_entry_rcu() and rescan
> > if it got the wrong terminator.
> > Or does scanning twice cause grief as well.  
> 
> I don't think it can. Say, __kill_pgrp_info() is a "typical" user of
> do_each_pid_task(). What can it do if it detects that get_nulls_value()
> doesn't match after the main loop? The signal was already sent.

It would have to check each entry to ensure it was on the correct list.
(That probably doesn't need the 'nulls' variant.)
The problem is that the rescan will do things twice.
This is ok for a search, but probably not for sending a signal.

David

> 
> Oleg.
>