Messages in this thread | | | Subject | Re: [PATCH v3 13/16] exit: Factor thread_group_exited out of pidfd_poll | From | Daniel Borkmann <> | Date | Wed, 8 Jul 2020 02:05:03 +0200 |
| |
On 7/7/20 7:09 PM, Eric W. Biederman wrote: > Christian Brauner <christian.brauner@ubuntu.com> writes: >> On Fri, Jul 03, 2020 at 04:37:47PM -0500, Eric W. Biederman wrote: >>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: >>> >>>> On Thu, Jul 02, 2020 at 11:41:37AM -0500, Eric W. Biederman wrote: >>>>> Create an independent helper thread_group_exited report return true >>>>> when all threads have passed exit_notify in do_exit. AKA all of the >>>>> threads are at least zombies and might be dead or completely gone. >>>>> >>>>> Create this helper by taking the logic out of pidfd_poll where >>>>> it is already tested, and adding a missing READ_ONCE on >>>>> the read of task->exit_state. >>>>> >>>>> I will be changing the user mode driver code to use this same logic >>>>> to know when a user mode driver needs to be restarted. >>>>> >>>>> Place the new helper thread_group_exited in kernel/exit.c and >>>>> EXPORT it so it can be used by modules. >>>>> >>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >>>>> --- >>>>> include/linux/sched/signal.h | 2 ++ >>>>> kernel/exit.c | 24 ++++++++++++++++++++++++ >>>>> kernel/fork.c | 6 +----- >>>>> 3 files changed, 27 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h >>>>> index 0ee5e696c5d8..1bad18a1d8ba 100644 >>>>> --- a/include/linux/sched/signal.h >>>>> +++ b/include/linux/sched/signal.h >>>>> @@ -674,6 +674,8 @@ static inline int thread_group_empty(struct task_struct *p) >>>>> #define delay_group_leader(p) \ >>>>> (thread_group_leader(p) && !thread_group_empty(p)) >>>>> >>>>> +extern bool thread_group_exited(struct pid *pid); >>>>> + >>>>> extern struct sighand_struct *__lock_task_sighand(struct task_struct *task, >>>>> unsigned long *flags); >>>>> >>>>> diff --git a/kernel/exit.c b/kernel/exit.c >>>>> index d3294b611df1..a7f112feb0f6 100644 >>>>> --- a/kernel/exit.c >>>>> +++ b/kernel/exit.c >>>>> @@ -1713,6 +1713,30 @@ COMPAT_SYSCALL_DEFINE5(waitid, >>>>> } >>>>> #endif >>>>> >>>>> +/** >>>>> + * thread_group_exited - check that a thread group has exited >>>>> + * @pid: tgid of thread group to be checked. >>>>> + * >>>>> + * Test if thread group is has exited (all threads are zombies, dead >>>>> + * or completely gone). >>>>> + * >>>>> + * Return: true if the thread group has exited. false otherwise. >>>>> + */ >>>>> +bool thread_group_exited(struct pid *pid) >>>>> +{ >>>>> + struct task_struct *task; >>>>> + bool exited; >>>>> + >>>>> + rcu_read_lock(); >>>>> + task = pid_task(pid, PIDTYPE_PID); >>>>> + exited = !task || >>>>> + (READ_ONCE(task->exit_state) && thread_group_empty(task)); >>>>> + rcu_read_unlock(); >>>>> + >>>>> + return exited; >>>>> +} >>>> >>>> I'm not sure why you think READ_ONCE was missing. >>>> It's different in wait_consider_task() where READ_ONCE is needed because >>>> of multiple checks. Here it's done once. >>> >>> In practice it probably has no effect on the generated code. But >>> READ_ONCE is about telling the compiler not to be clever. Don't use >>> tearing loads or stores etc. When all of the other readers are using >>> READ_ONCE I just get nervous if we have a case that doesn't. >> >> That's not true. The only place where READ_ONCE(->exit_state) is used is >> in wait_consider_task() and nowhere else. We had that discussion a while >> ago where I or someone proposed to simply place a READ_ONCE() around all >> accesses to exit_state for the sake of kcsan and we agreed that it's >> unnecessary and not to do this. >> But it obviously doesn't hurt to have it. > > There is a larger discussion to be had around the proper handling of > exit_state. > > In this particular case because we are accessing exit_state with > only rcu_read_lock protection, because the outcome of the read > is about correctness, and because the compiler has nothing else > telling it not to re-read exit_state, I believe we actually need > the READ_ONCE. > > At the same time it would take a pretty special compiler to want to > reaccess that field in thread_group_exited. > > I have looked through and I don't find any of the other access of > exit_state where the result is about correctness (so that we care) > and we don't hold tasklist_lock. > > But I have removed the necessary wording from the commit comment.
Hey Eric, are you planning to push the final version into a topic branch so it can be pulled into bpf-next as discussed earlier?
Thanks, Daniel
| |