linux-kernel - Re: [PATCH 1/3] pidfd_poll: report POLLHUP when pid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240205140848.GA15853@redhat.com>
Date: Mon, 5 Feb 2024 15:08:49 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Andy Lutomirski <luto@...capital.net>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Tycho Andersen <tycho@...ho.pizza>, linux-api@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] pidfd_poll: report POLLHUP when pid_task() == NULL

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) {
 		/*