linux-kernel - Re: [PATCH RFC] io_uring: make signalfd work with io

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]

Message-ID: <85e8e954-d09c-f0b4-0944-598208098c8c@kernel.dk>
Date:   Thu, 14 Nov 2019 08:27:01 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Jann Horn <jannh@...gle.com>
Cc:     io-uring@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH RFC] io_uring: make signalfd work with io_uring (and aio)
 POLL

On 11/14/19 8:20 AM, Jens Axboe wrote:
> On 11/14/19 8:19 AM, Rasmus Villemoes wrote:
>> On 14/11/2019 16.09, Jens Axboe wrote:
>>> On 11/14/19 7:12 AM, Rasmus Villemoes wrote:
>>
>>>> So, I can't really think of anybody that might be relying on inheriting
>>>> a signalfd instead of just setting it up in the child, but changing the
>>>> semantics of it now seems rather dangerous. Also, I _can_ imagine
>>>> threads in a process sharing a signalfd (initial thread sets it up and
>>>> blocks the signals, all threads subsequently use that same fd), and for
>>>> that case it would be wrong for one thread to dequeue signals directed
>>>> at the initial thread. Plus the lifetime problems.
>>>
>>> What if we just made it specific SFD_CLOEXEC?
>>
>> O_CLOEXEC can be set and removed afterwards. Sure, we're far into
>> "nobody does that" land, but having signalfd() have wildly different
>> semantics based on whether it was initially created with O_CLOEXEC seems
>> rather dubious.
>>
>>    I don't want to break
>>> existing applications, even if the use case is nonsensical, but it is
>>> important to allow signalfd to be properly used with use cases that are
>>> already in the kernel (aio with IOCB_CMD_POLL, io_uring with
>>> IORING_OP_POLL_ADD). Alternatively, if need be, we could add a specific
>>> SFD_ flag for this.
>>
>> Yeah, if you want another signalfd flavour, adding it via a new SFD_
>> flag seems the way to go. Though I can't imagine the resulting code
>> would be very pretty.
> 
> Well, it's currently _broken_ for the listed in-kernel use cases, so
> I think making it work is the first priority here.

How about something like this, then? Not tested.

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 44b6845b071c..d8b183ec1d4e 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -50,6 +50,8 @@ void signalfd_cleanup(struct sighand_struct *sighand)
 
 struct signalfd_ctx {
 	sigset_t sigmask;
+	struct task_struct *task;
+	u32 task_exec_id;
 };
 
 static int signalfd_release(struct inode *inode, struct file *file)
@@ -61,16 +63,22 @@ 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 = ctx->task ?: current;
 	__poll_t events = 0;
 
-	poll_wait(file, &current->sighand->signalfd_wqh, wait);
+	if (ctx->task && ctx->task->self_exec_id == ctx->task_exec_id)
+		tsk = ctx->task;
+	else
+		tsk = current;
 
-	spin_lock_irq(&current->sighand->siglock);
-	if (next_signal(&current->pending, &ctx->sigmask) ||
-	    next_signal(&current->signal->shared_pending,
+	poll_wait(file, &tsk->sighand->signalfd_wqh, wait);
+
+	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;
 }
@@ -267,19 +275,26 @@ static int do_signalfd4(int ufd, sigset_t *mask, int flags)
 	/* Check the SFD_* constants for consistency.  */
 	BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);
+	BUILD_BUG_ON(SFD_TASK & (SFD_CLOEXEC | SFD_NONBLOCK));
 
-	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK))
+	if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK | SFD_TASK))
+		return -EINVAL;
+	if ((flags & (SFD_CLOEXEC | SFD_TASK)) == SFD_TASK)
 		return -EINVAL;
 
 	sigdelsetmask(mask, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	signotset(mask);
 
 	if (ufd == -1) {
-		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+		ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 		if (!ctx)
 			return -ENOMEM;
 
 		ctx->sigmask = *mask;
+		if (flags & SFD_TASK) {
+			ctx->task = current;
+			ctx->task_exec_id = current->self_exec_id;
+		}
 
 		/*
 		 * When we call this, the initialization must be complete, since
diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h
index 83429a05b698..064c5dc3eb99 100644
--- a/include/uapi/linux/signalfd.h
+++ b/include/uapi/linux/signalfd.h
@@ -16,6 +16,7 @@
 /* Flags for signalfd4.  */
 #define SFD_CLOEXEC O_CLOEXEC
 #define SFD_NONBLOCK O_NONBLOCK
+#define SFD_TASK 00000001
 
 struct signalfd_siginfo {
 	__u32 ssi_signo;

-- 
Jens Axboe