[3/3] signalfd: convert to ->read_iter()

Commit Message

Jens Axboe April 3, 2024, 2:02 p.m. UTC

Rather than use the older style ->read() hook, use ->read_iter() so that
signalfd can support both O_NONBLOCK and IOCB_NOWAIT for non-blocking
read attempts.

Split the fd setup into two parts, so that signalfd can mark the file
mode with FMODE_NOWAIT before installing it into the process table.

Signed-off-by: Jens Axboe <[e-mail]>
---
 fs/signalfd.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

Comments

Al Viro April 3, 2024, 10:57 p.m. UTC | #1

On Wed, Apr 03, 2024 at 08:02:54AM -0600, Jens Axboe wrote:
> Rather than use the older style ->read() hook, use ->read_iter() so that
> signalfd can support both O_NONBLOCK and IOCB_NOWAIT for non-blocking
> read attempts.
> 
> Split the fd setup into two parts, so that signalfd can mark the file
> mode with FMODE_NOWAIT before installing it into the process table.

Same issue with copy_to_iter() calling conventions; what's more, userland
really does not expect partial copies here, so it might be worth adding

static inline
bool copy_to_iter_full(void *addr, size_t bytes, struct iov_iter *i)
{
        size_t copied = copy_to_iter(addr, bytes, i);
	if (likely(copied == bytes))
		return true;
	iov_iter_revert(i, copied);
	return false;
}

to include/linux/uio.h for the sake of those suckers.  Then
they could go for
        return copy_to_iter_full(&new, sizeof(new), to) ? sizeof(new) : -EFAULT;
and similar in other two.

NOTE: the userland ABI is somewhat sucky here - if the buffer goes
unmapped (or r/o) at the offset that is *not* a multiple of
sizeof(struct signalfd_siginfo), you get an event quietly lost.
Not sure what can be done with that - it is a user-visible ABI.

Jens Axboe April 4, 2024, 12:52 p.m. UTC | #2

On 4/3/24 4:57 PM, Al Viro wrote:
> On Wed, Apr 03, 2024 at 08:02:54AM -0600, Jens Axboe wrote:
>> Rather than use the older style ->read() hook, use ->read_iter() so that
>> signalfd can support both O_NONBLOCK and IOCB_NOWAIT for non-blocking
>> read attempts.
>>
>> Split the fd setup into two parts, so that signalfd can mark the file
>> mode with FMODE_NOWAIT before installing it into the process table.
> 
> Same issue with copy_to_iter() calling conventions; what's more, userland
> really does not expect partial copies here, so it might be worth adding
> 
> static inline
> bool copy_to_iter_full(void *addr, size_t bytes, struct iov_iter *i)
> {
>         size_t copied = copy_to_iter(addr, bytes, i);
> 	if (likely(copied == bytes))
> 		return true;
> 	iov_iter_revert(i, copied);
> 	return false;
> }
> 
> to include/linux/uio.h for the sake of those suckers.  Then
> they could go for
>         return copy_to_iter_full(&new, sizeof(new), to) ? sizeof(new) : -EFAULT;
> and similar in other two.

That's a good idea, makes the transformations easier too and avoids
dealine with the partial case.

> NOTE: the userland ABI is somewhat sucky here - if the buffer goes
> unmapped (or r/o) at the offset that is *not* a multiple of
> sizeof(struct signalfd_siginfo), you get an event quietly lost.
> Not sure what can be done with that - it is a user-visible ABI.

The ABI could be nicer, but isn't this a case of "well don't do that
then"?

Patch

diff --git a/fs/signalfd.c b/fs/signalfd.c
index e20d1484c663..72b6796a9a40 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -68,8 +68,7 @@  static __poll_t signalfd_poll(struct file *file, poll_table *wait)
 /*
  * Copied from copy_siginfo_to_user() in kernel/signal.c
  */
-static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
-			     kernel_siginfo_t const *kinfo)
+static int signalfd_copyinfo(struct iov_iter *to, kernel_siginfo_t const *kinfo)
 {
 	struct signalfd_siginfo new;
 
@@ -146,10 +145,7 @@  static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
 		break;
 	}
 
-	if (copy_to_user(uinfo, &new, sizeof(struct signalfd_siginfo)))
-		return -EFAULT;
-
-	return sizeof(*uinfo);
+	return copy_to_iter(&new, sizeof(struct signalfd_siginfo), to);
 }
 
 static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, kernel_siginfo_t *info,
@@ -199,28 +195,27 @@  static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, kernel_siginfo_t *info
  * error code. The "count" parameter must be at least the size of a
  * "struct signalfd_siginfo".
  */
-static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
-			     loff_t *ppos)
+static ssize_t signalfd_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
+	struct file *file = iocb->ki_filp;
 	struct signalfd_ctx *ctx = file->private_data;
-	struct signalfd_siginfo __user *siginfo;
-	int nonblock = file->f_flags & O_NONBLOCK;
+	size_t count = iov_iter_count(to);
 	ssize_t ret, total = 0;
 	kernel_siginfo_t info;
+	bool nonblock;
 
 	count /= sizeof(struct signalfd_siginfo);
 	if (!count)
 		return -EINVAL;
 
-	siginfo = (struct signalfd_siginfo __user *) buf;
+	nonblock = file->f_flags & O_NONBLOCK || iocb->ki_flags & IOCB_NOWAIT;
 	do {
 		ret = signalfd_dequeue(ctx, &info, nonblock);
 		if (unlikely(ret <= 0))
 			break;
-		ret = signalfd_copyinfo(siginfo, &info);
+		ret = signalfd_copyinfo(to, &info);
 		if (ret < 0)
 			break;
-		siginfo++;
 		total += ret;
 		nonblock = 1;
 	} while (--count);
@@ -246,7 +241,7 @@  static const struct file_operations signalfd_fops = {
 #endif
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
-	.read		= signalfd_read,
+	.read_iter	= signalfd_read_iter,
 	.llseek		= noop_llseek,
 };
 
@@ -265,20 +260,35 @@  static int do_signalfd4(int ufd, sigset_t *mask, int flags)
 	signotset(mask);
 
 	if (ufd == -1) {
+		struct file *file;
+
 		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 		if (!ctx)
 			return -ENOMEM;
 
 		ctx->sigmask = *mask;
 
+		ufd = get_unused_fd_flags(O_RDWR |
+					  (flags & (O_CLOEXEC | O_NONBLOCK)));
+		if (ufd < 0) {
+			kfree(ctx);
+			return ufd;
+		}
+
+		file = anon_inode_getfile("[signalfd]", &signalfd_fops, ctx,
+				       O_RDWR | (flags & (O_CLOEXEC | O_NONBLOCK)));
+		if (IS_ERR(file)) {
+			put_unused_fd(ufd);
+			kfree(ctx);
+			return ufd;
+		}
+		file->f_mode |= FMODE_NOWAIT;
+
 		/*
 		 * When we call this, the initialization must be complete, since
 		 * anon_inode_getfd() will install the fd.
 		 */
-		ufd = anon_inode_getfd("[signalfd]", &signalfd_fops, ctx,
-				       O_RDWR | (flags & (O_CLOEXEC | O_NONBLOCK)));
-		if (ufd < 0)
-			kfree(ctx);
+		fd_install(ufd, file);
 	} else {
 		struct fd f = fdget(ufd);
 		if (!f.file)