Issue: IORING_OP_POLL_ADD with signalfd

[CarterLi]

#include <unistd.h>
#include <sys/signalfd.h>
#include <sys/poll.h>

#include <liburing.h>

int main() {
    sigset_t mask;
    sigemptyset(&mask);
    sigaddset(&mask, SIGINT);

    sigprocmask(SIG_BLOCK, &mask, NULL);
    int sfd = signalfd(-1, &mask, SFD_NONBLOCK);

    struct io_uring ring;
    io_uring_queue_init(32, &ring, 0);

    struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);
    io_uring_prep_poll_add(sqe, sfd, POLLIN);
    io_uring_submit(&ring);

    struct io_uring_cqe *cqe;
    io_uring_wait_cqe(&ring, &cqe);
    io_uring_cqe_seen(&ring, cqe);
    io_uring_queue_exit(&ring);

    close(sfd);
    return 0;
}

Ctrl+C should terminate the program but it doesn't. Similar code works for epoll: https://gist.github.com/CarterLi/b8db2fcfea689b96eeae382c38130afb

Linux Ubuntu 5.3.0-10-generic #11-Ubuntu SMP Mon Sep 9 15:12:17 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

[axboe]

I tested your test app, I get the same behavior with io_uring and aio - ctrl-c does not wake either of them up?

[CarterLi]

I tested your test app, I get the same behavior with io_uring and aio - ctrl-c does not wake either of them up?

I got expected behavior for epoll and plain read.

#include <unistd.h>
#include <sys/signalfd.h>
#include <stdio.h>
#include <signal.h>

int main() {
    sigset_t mask;
    sigemptyset(&mask);
    sigaddset(&mask, SIGINT);

    sigprocmask(SIG_BLOCK, &mask, NULL);
    int sfd = signalfd(-1, &mask, 0);
    struct signalfd_siginfo res;
    read(sfd, &res, sizeof(res));
    perror("read");
    close(sfd);
    return 0;
}

[CarterLi]

Any updates? @axboe

[axboe]

I need to look closer at this, because it seems to me like we should get the ctrl-c initially. Once the work has moved off to async, if it does, then that's a different story. I've flushed out my backlog of stuff for 5.5 fixes today, so I'll get on this tomorrow.

[axboe]

Tomorrow came early, here's what I found. The initial ->poll() call, which ends up with signalfd for this case, is using the correct task. But we don't find any signals, so we queue this up for async execution. When the signal then comes in, it's delivered to the original task, it's woken up, and we call ->poll() to check for events. This time the task is the async worker, and it has no signals pending. Hence still no POLLIN, and we swallowed the SIGINT.

I hacked with it a bit, this is what I came up with. It works for me, but not yet sure how palatable it'll be to the greater kernel community. I'll check.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index d8ea9b4f83a7..d9a4c9aac958 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -299,6 +299,7 @@ struct io_poll_iocb {
 	bool				done;
 	bool				canceled;
 	struct wait_queue_entry		wait;
+	struct task_struct		*task;
 };
 
 struct io_timeout {
@@ -2021,7 +2022,10 @@ static void io_poll_complete_work(struct io_wq_work **workptr)
 	struct io_wq_work *work = *workptr;
 	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
 	struct io_poll_iocb *poll = &req->poll;
-	struct poll_table_struct pt = { ._key = poll->events };
+	struct poll_table_struct pt = {
+		._key = poll->events,
+		.task = poll->task
+	};
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_kiocb *nxt = NULL;
 	__poll_t mask = 0;
@@ -2139,9 +2143,11 @@ static int io_poll_add(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	poll->head = NULL;
 	poll->done = false;
 	poll->canceled = false;
+	poll->task = current;
 
 	ipt.pt._qproc = io_poll_queue_proc;
 	ipt.pt._key = poll->events;
+	ipt.pt.task = poll->task;
 	ipt.req = req;
 	ipt.error = -EINVAL; /* same as no support for IOCB_CMD_POLL */
 
diff --git a/fs/signalfd.c b/fs/signalfd.c
index 44b6845b071c..a7f31758db1a 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -61,16 +61,17 @@ static int signalfd_release(struct inode *inode, struct file *file)
 static __poll_t signalfd_poll(struct file *file, poll_table *wait)
 {
 	struct signalfd_ctx *ctx = file->private_data;
+	struct task_struct *tsk = wait->task ?: current;
 	__poll_t events = 0;
 
-	poll_wait(file, &current->sighand->signalfd_wqh, wait);
+	poll_wait(file, &tsk->sighand->signalfd_wqh, wait);
 
-	spin_lock_irq(&current->sighand->siglock);
-	if (next_signal(&current->pending, &ctx->sigmask) ||
-	    next_signal(&current->signal->shared_pending,
+	spin_lock_irq(&tsk->sighand->siglock);
+	if (next_signal(&tsk->pending, &ctx->sigmask) ||
+	    next_signal(&tsk->signal->shared_pending,
 			&ctx->sigmask))
 		events |= EPOLLIN;
-	spin_unlock_irq(&current->sighand->siglock);
+	spin_unlock_irq(&tsk->sighand->siglock);
 
 	return events;
 }
diff --git a/include/linux/poll.h b/include/linux/poll.h
index 1cdc32b1f1b0..6d2b6d923b2b 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -43,6 +43,7 @@ typedef void (*poll_queue_proc)(struct file *, wait_queue_head_t *, struct poll_
 typedef struct poll_table_struct {
 	poll_queue_proc _qproc;
 	__poll_t _key;
+	struct task_struct *task;
 } poll_table;
 
 static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
@@ -76,6 +77,7 @@ static inline void init_poll_funcptr(poll_table *pt, poll_queue_proc qproc)
 {
 	pt->_qproc = qproc;
 	pt->_key   = ~(__poll_t)0; /* all events enabled */
+	pt->task = NULL;
 }
 
 static inline bool file_can_poll(struct file *file)

[axboe]

Here's a cleaner variant, that just fixes signalfd instead:

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 44b6845b071c..cc72b5b08946 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -50,6 +50,7 @@ void signalfd_cleanup(struct sighand_struct *sighand)
 
 struct signalfd_ctx {
 	sigset_t sigmask;
+	struct task_struct *task;
 };
 
 static int signalfd_release(struct inode *inode, struct file *file)
@@ -63,14 +64,14 @@ static __poll_t signalfd_poll(struct file *file, poll_table *wait)
 	struct signalfd_ctx *ctx = file->private_data;
 	__poll_t events = 0;
 
-	poll_wait(file, &current->sighand->signalfd_wqh, wait);
+	poll_wait(file, &ctx->task->sighand->signalfd_wqh, wait);
 
-	spin_lock_irq(&current->sighand->siglock);
-	if (next_signal(&current->pending, &ctx->sigmask) ||
-	    next_signal(&current->signal->shared_pending,
+	spin_lock_irq(&ctx->task->sighand->siglock);
+	if (next_signal(&ctx->task->pending, &ctx->sigmask) ||
+	    next_signal(&ctx->task->signal->shared_pending,
 			&ctx->sigmask))
 		events |= EPOLLIN;
-	spin_unlock_irq(&current->sighand->siglock);
+	spin_unlock_irq(&ctx->task->sighand->siglock);
 
 	return events;
 }
@@ -280,6 +281,7 @@ static int do_signalfd4(int ufd, sigset_t *mask, int flags)
 			return -ENOMEM;
 
 		ctx->sigmask = *mask;
+		ctx->task = current;
 
 		/*
 		 * When we call this, the initialization must be complete, since

[axboe]

I feel it's important to point out that it's signalfd that's broken here, it's not aio or io_uring. signalfd should be looking at the task that created the signalfd in the first place, not whatever process happens to be calling ->poll() on its behalf.

[axboe]

Since this might take a bit of time to hash out, I'd recommend just using a sigset_t with io_uring_enter() instead.

[CarterLi]

Since this might take a bit of time to hash out, I'd recommend just using a sigset_t with io_uring_enter() instead.

Using io_uring_enter directly is rather complex.

I can wait for it. Thanks.

[axboe]

Using io_uring_enter directly is rather complex.

You don't have to use it directly, there is also io_uring_wait_cqe_timeout()
But point taken, I'll work on getting the signalfd stuff fixed upstream.

[CarterLi]

Since we now support non-iter based read/write. Does IORING_OP_READV with signalfd have similar problem like IORING_OP_POLL_ADD?

[CarterLi]

I tested IORING_OP_READV with signalfd on Linux 5.4.0. It didn't work too.

#define _POSIX_C_SOURCE 1
#include <unistd.h>
#include <sys/signalfd.h>
#include <stdio.h>
#include <liburing.h>

int main() {
    sigset_t mask;
    sigemptyset(&mask);
    sigaddset(&mask, SIGINT);

    sigprocmask(SIG_BLOCK, &mask, NULL);
    int sfd = signalfd(-1, &mask, 0);

    struct io_uring ring;
    io_uring_queue_init(32, &ring, 0);

    struct io_uring_sqe *sqe = io_uring_get_sqe(&ring);

    struct signalfd_siginfo res;
    struct iovec iov = { &res, sizeof(res) };
    io_uring_prep_readv(sqe, sfd, &iov, 1, 0);
    io_uring_submit(&ring);

    struct io_uring_cqe *cqe;
    io_uring_wait_cqe(&ring, &cqe);
    io_uring_cqe_seen(&ring, cqe);
    io_uring_queue_exit(&ring);

    printf("%d, %d\n", SIGINT, res.ssi_signo);
    puts("OK");
    close(sfd);
}

Any progress? @axboe

[axboe]

Working on it... https://lore.kernel.org/io-uring/254505c9-2b76-ebeb-306c-02aaf1704b88@kernel.dk/T/#u

…

On Nov 26, 2019, at 6:59 PM, Carter Li ***@***.***> wrote:  I tested IORING_OP_READV with signalfd on Linux 5.4.0. It didn't work too. #define _POSIX_C_SOURCE 1 #include <unistd.h> #include <sys/signalfd.h> #include <stdio.h> #include <liburing.h> int main() { sigset_t mask; sigemptyset(&mask); sigaddset(&mask, SIGINT); sigprocmask(SIG_BLOCK, &mask, NULL); int sfd = signalfd(-1, &mask, 0); struct io_uring ring; io_uring_queue_init(32, &ring, 0); struct io_uring_sqe *sqe = io_uring_get_sqe(&ring); struct signalfd_siginfo res; struct iovec iov = { &res, sizeof(res) }; io_uring_prep_readv(sqe, sfd, &iov, 1, 0); io_uring_submit(&ring); struct io_uring_cqe *cqe; io_uring_wait_cqe(&ring, &cqe); io_uring_cqe_seen(&ring, cqe); io_uring_queue_exit(&ring); printf("%d, %d\n", SIGINT, res.ssi_signo); puts("OK"); close(sfd); } Any progress? @axboe — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.