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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 2 Feb 2024 18:24:25 +0100
From: Christian Brauner <brauner@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
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

> I think we need a simpler patch. I was going to send it as 4/4, but I'd
> like to think more, _perhaps_ we can also discriminate the PIDFD_THREAD
> and non-PIDFD_THREAD waiters. I'll try to make the patch(es) tomorrow or

Right, I didn't go that far.

> at least provided more info.
> 
> 3 notes for now:
> 
> 	1. we can't use wake_up_poll(), it passes nr_exclusive => 1

Bah. So we need the same stuff we did for io_uring and use
__wake_up() directly. Or we add wake_up_all_poll() and convert the other
three callsites:

// uncompiled, untested

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 8aa3372f21a0..210ee0d69b6f 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -234,6 +234,8 @@ void __wake_up_pollfree(struct wait_queue_head *wq_head);
 #define key_to_poll(m) ((__force __poll_t)(uintptr_t)(void *)(m))
 #define wake_up_poll(x, m)                                                     \
        __wake_up(x, TASK_NORMAL, 1, poll_to_key(m))
+#define wake_up_all_poll(x, m)                                                 \
+       __wake_up(x, TASK_NORMAL, 0, poll_to_key(m))
 #define wake_up_poll_on_current_cpu(x, m)                                      \
        __wake_up_on_current_cpu(x, TASK_NORMAL, poll_to_key(m))
 #define wake_up_locked_poll(x, m)                                              \
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 04e33f25919c..65dcd5dc9645 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -228,8 +228,7 @@ static inline void io_commit_cqring(struct io_ring_ctx *ctx)
 static inline void io_poll_wq_wake(struct io_ring_ctx *ctx)
 {
        if (wq_has_sleeper(&ctx->poll_wq))
-               __wake_up(&ctx->poll_wq, TASK_NORMAL, 0,
-                               poll_to_key(EPOLL_URING_WAKE | EPOLLIN));
+               wake_up_all_poll(&ctx->poll_wq, EPOLL_URING_WAKE | EPOLLIN);
 }

 static inline void io_cqring_wake(struct io_ring_ctx *ctx)
@@ -245,8 +244,7 @@ static inline void io_cqring_wake(struct io_ring_ctx *ctx)
         * epoll and should terminate multishot poll at that point.
         */
        if (wq_has_sleeper(&ctx->cq_wait))
-               __wake_up(&ctx->cq_wait, TASK_NORMAL, 0,
-                               poll_to_key(EPOLL_URING_WAKE | EPOLLIN));
+               wake_up_all_poll(&ctx->cq_wait, EPOLL_URING_WAKE | EPOLLIN);
 }

 static inline bool io_sqring_full(struct io_ring_ctx *ctx)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 51e38f5f4701..ee849fb35603 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -208,7 +208,7 @@ EXPORT_SYMBOL_GPL(__wake_up_sync);  /* For internal
use only */

 void __wake_up_pollfree(struct wait_queue_head *wq_head)
  {
  -       __wake_up(wq_head, TASK_NORMAL, 0, poll_to_key(EPOLLHUP |
  POLLFREE));
  +       wake_up_all_poll(wq_head, EPOLLHUP | POLLFREE);
          /* POLLFREE must have cleared the queue. */
	          WARN_ON_ONCE(waitqueue_active(wq_head));
		   }

> 
> 	2. exit_notify() should not pass EPOLLHUP to wake_up, we do
> 	   not want to wake up the { .events = POLLHUP } waiters.

Indeed.

> 
> 	3. we do not need to change __change_pid().
> 
> 	   Well, _perhaps_ it can/should use __wake_up_pollfree(), but
> 	   I need to check if fs/select.c use "autoremove" or not.
> 
> 
> > -static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> > +static __poll_t pidfd_poll(struct file *file, poll_table *wait)
> >  {
> >  	struct pid *pid = file->private_data;
> >  	bool thread = file->f_flags & PIDFD_THREAD;
> >  	struct task_struct *task;
> >  	__poll_t poll_flags = 0;
> >  
> > -	poll_wait(file, &pid->wait_pidfd, pts);
> > +	poll_wait(file, &pid->wait_pidfd, wait);
> 
> This is correct but only cosemtic and has nothing to do with what
> we discuss?

No, I just folded all of the changes because it was just a draft. See
the updated draft I appended.

View attachment "0001-UNTESTED-UNCOMPILED.patch" of type "text/x-diff" (5212 bytes)