Hi Tycho, On 01/26, Tycho Andersen wrote: > > On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote: > > What do you think? > > Thank you, it passes all my tests. Great, thanks! OK, I'll make v2 on top of the recent "pidfd: cleanup the usage of __pidfd_prepare's flags" but we need to finish our discussion with Christian about the usage of O_EXCL. As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but I think this needs another discussion too, lets do this later. > > + /* unnecessary if do_notify_parent() was already called, > > + we can do better */ > > + do_notify_pidfd(tsk); > > "do better" here could be something like, > > [...snip...] No, no, please see below. For the moment, please forget about PIDFD_THREAD, lets discuss the current behaviour. > but even with that, there's other calls in the tree to > do_notify_parent() that might double notify. Yes, and we can't avoid this. Well, perhaps do_notify_parent() can do something like if (ptrace_reparented()) do_notify_pidfd(); so that only the "final" do_notify_parent() does do_notify_pidfd() but this needs another discussion and in fact I don't think this would be right or make much sense. Lets forget this for now. Now. Even without PIDFD_THREAD, I think it makes sense to change do_notify_parent() to do if (thread_group_empty(tsk)) do_notify_pidfd(tsk); thread_group_empty(tsk) can only be true if tsk is a group leader and it is the last thread. And this is exactly what pidfd_poll() currently needs. In fact I'd even prefer to do this in a separate patch for the documentation purposes. Now, PIDFD_THREAD can just add if (!thread_group_empty(tsk)) do_notify_pidfd(tsk); right after "tsk->exit_state = EXIT_ZOMBIE", that is all. This also preserves the do_notify_pidfd/__wake_up_parent ordering. Not that I think this is important, just for consistency. > This brings up another interesting behavior that I noticed while > testing this, if you do a poll() on pidfd, followed quickly by a > pidfd_getfd() on the same thread you just got an event on, you can > sometimes get an EBADF from __pidfd_fget() instead of the more > expected ESRCH higher up the stack. exit_notify() is called after exit_files(). pidfd_getfd() returns ESRCH if the exiting thread completes release_task(), otherwise it returns EBADF because ->files == NULL. This too doesn't really depend on PIDFD_THREAD. > I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED? > Then we can refuse further pidfd syscall operations in a sane way, and But how? We only have "struct pid *", how can we find all files "attached" to this pid? > also "do better" above by checking this flag from do_pidfd_notify() > before doing it again? and even it was possible, I don't think it makes a lot of sense, see also above. but perhaps I understood you... Oleg.
