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

[David Laight]

On Fri,  2 Oct 2026 01:21:41 +0000
Babanpreet Singh <bbnpreetsingh@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=c382ee653fd70f5cf1bb
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Babanpreet Singh <bbnpreetsingh@xxxxxxxxx>
> ---
> 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