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

[David Laight]

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

> On 10/03, David Laight wrote:
> >
> > 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?  
> 
> 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@xxxxxxxxxx/
> 
> 	> 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.
>