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@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.
>
> Oleg.