On 02/03, Christian Brauner wrote: > > On Sat, Feb 03, 2024 at 01:04:26PM +0100, Oleg Nesterov wrote: > > > > - wake_up_all(&pid->wait_pidfd); > > + __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0, > > + poll_to_key(EPOLLIN | EPOLLRDNORM)); > > > Ok, care to just send me a full patch for this? Will do in a minute, > Second, I agree that this looks ugly. ;/ Agreed ;) > So here's a very likely a stupid idea. To make that clean we essentially > need kernel private information that can't be set in userspace (Btw, > look at EPOLL_URING_WAKE which is similar in that it cannot be set from > userspace. It's not the same thing ofc but it is a private bit.). Which > is the gist of your proposal in a way. > > So we would have to grab a new private bit in the epoll flag space. Agreed, but just in case: - EPOLLMSG (ab)used by this patch can't "leak" to userspace even if it was (erroneously) set in pollfd.events - If EPOLLMSG was set by userspace nothing bad can happen, just poll(non-PIDFD_THREAD-pidfd) will get the spurious wakeups. So. I am attaching the patch for the record, in case we return to this later. It seems to work fine, but when I look into fs/eventpoll.c I suspect it is not epoll friendly. I _think_ that the neccessary fix is trivial, ep_item_poll() should just copy pt->_key to epi->event.events after vfs_poll(), but I am not sure. So lets forget it for now. Oleg. --- diff --git a/include/linux/pid.h b/include/linux/pid.h index 8124d57752b9..7467cdb9735b 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -74,7 +74,7 @@ struct pid *pidfd_pid(const struct file *file); struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags); int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret); -void do_notify_pidfd(struct task_struct *task); +void do_notify_pidfd(struct task_struct *task, bool thread); static inline struct pid *get_pid(struct pid *pid) { diff --git a/kernel/exit.c b/kernel/exit.c index 493647fd7c07..c31f36d3a1ed 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -744,7 +744,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead) * PIDFD_THREAD waiters. */ if (!thread_group_empty(tsk)) - do_notify_pidfd(tsk); + do_notify_pidfd(tsk, true); if (unlikely(tsk->ptrace)) { int sig = thread_group_leader(tsk) && diff --git a/kernel/fork.c b/kernel/fork.c index 8d08a2d1b095..3b4474ff6f4a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2081,6 +2081,15 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) struct task_struct *task; __poll_t poll_flags = 0; + if (pts && pts->_qproc) { + /* + * We are not registered yet. Update pts->_key to mark us as + * a non POLLHUP-only PIDFD_THREAD waiter for do_notify_pidfd, + * _pollwait() will copy this _key to poll_table_entry->key. + */ + if (thread && (pts->_key & (EPOLLIN | EPOLLRDNORM))) + pts->_key |= EPOLLMSG; + } poll_wait(file, &pid->wait_pidfd, pts); /* * Depending on PIDFD_THREAD, inform pollers when the thread diff --git a/kernel/signal.c b/kernel/signal.c index c3fac06937e2..f51070dec132 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2019,14 +2019,15 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type) return ret; } -void do_notify_pidfd(struct task_struct *task) +void do_notify_pidfd(struct task_struct *task, bool thread) { + /* see the usage of EPOLLMSG in pidfd_poll() */ + __poll_t m = thread ? EPOLLMSG : EPOLLIN | EPOLLRDNORM; struct pid *pid = task_pid(task); WARN_ON(task->exit_state == 0); - __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0, - poll_to_key(EPOLLIN | EPOLLRDNORM)); + __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0, poll_to_key(m)); } /* @@ -2056,7 +2057,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig) * non-PIDFD_THREAD waiters. */ if (thread_group_empty(tsk)) - do_notify_pidfd(tsk); + do_notify_pidfd(tsk, false); if (sig != SIGCHLD) { /*