Oleg Nesterov <oleg@xxxxxxxxxx> writes: > On 04/23, Eric W. Biederman wrote: >> >> When the thread group leader changes during exec and the old leaders >> thread is reaped proc_flush_pid > > This is off-topic, but let me mention this before I forget... > > Note that proc_flush_pid() does nothing if CONFIG_PROC_FS=n, this mean > that in this case release_task() leaks thread_pid. Good catch. Wow. I seem to be introducing almost as many bugs as I am fixing right now. Ouch. >> +void exchange_tids(struct task_struct *ntask, struct task_struct *otask) >> +{ >> + /* pid_links[PIDTYPE_PID].next is always NULL */ >> + struct pid *npid = READ_ONCE(ntask->thread_pid); >> + struct pid *opid = READ_ONCE(otask->thread_pid); >> + >> + rcu_assign_pointer(opid->tasks[PIDTYPE_PID].first, &ntask->pid_links[PIDTYPE_PID]); >> + rcu_assign_pointer(npid->tasks[PIDTYPE_PID].first, &otask->pid_links[PIDTYPE_PID]); >> + rcu_assign_pointer(ntask->thread_pid, opid); >> + rcu_assign_pointer(otask->thread_pid, npid); >> + WRITE_ONCE(ntask->pid_links[PIDTYPE_PID].pprev, &opid->tasks[PIDTYPE_PID].first); >> + WRITE_ONCE(otask->pid_links[PIDTYPE_PID].pprev, &npid->tasks[PIDTYPE_PID].first); >> + WRITE_ONCE(ntask->pid, pid_nr(opid)); >> + WRITE_ONCE(otask->pid, pid_nr(npid)); >> +} > > Oh, at first glance this breaks posix-cpu-timers.c:lookup_task(), the last > user of has_group_leader_pid(). > > I think that we should change lookup_task() to return "struct *pid", this > should simplify the code... Note that none of its callers needs task_struct. > > And, instead of thread_group_leader/has_group_leader_pid checks we should > use pid_has_task(TGID). Somehow I thought we could get away without fiddling with that right now, but on second glance I can see the races. I played with this earlier and I agree returning a struct pid * is desirable. I will see if I can track down the patches I was playing with as that definitely needs to get fixed first. > After that, this patch should kill has_group_leader_pid(). > > What do you think? I agree completely. has_group_leader_pid is the same as thread_group_leader after this so should be removed. Especially as it won't have any users. There are several other potential cleanups as well. Such as not using a hlist for PIDTYPE_PID. Which would allow us to run the hlists through struct signal_struct instead. I think that would clean things up but that touches so many things it may just be pointless code churn. Just for mentioning I am thinking we should rename PIDTYPE_PID to PIDTYPE_TID just to create a distance in peoples minds between the kernel concepts and the user space concepts. Eric