linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alice Ryhl <aliceryhl@google.com>
To: rust-for-linux@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Miguel Ojeda <ojeda@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Christian Brauner <brauner@kernel.org>
Cc: "Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <benno.lossin@proton.me>,
	"Alice Ryhl" <aliceryhl@google.com>,
	linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: [RFC PATCH v1 5/5] rust: file: add `DeferredFdCloser`
Date: Thu, 20 Jul 2023 15:28:20 +0000	[thread overview]
Message-ID: <20230720152820.3566078-6-aliceryhl@google.com> (raw)
In-Reply-To: <20230720152820.3566078-1-aliceryhl@google.com>

This adds a new type called `DeferredFdCloser` that can be used to close
files by their fd in a way that is safe even if the file is currently
held using `fdget`.

This is done by grabbing an extra refcount to the file and dropping it
in a task work once we return to userspace.

See comments on `binder_do_fd_close` and commit `80cd795630d65` for
motivation.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
This is an implementation of `binder_deferred_fd_close` in Rust.

I think the fact that binder needs to close fds in this way raises the
question of how we want the Rust APIs for closing files to look.
Apparently, fdget is not just used in easily reviewable regions, but
also around things like the ioctl syscall, meaning that all ioctls must
abide by the fdget safety requirements.

 rust/bindings/bindings_helper.h |  2 +
 rust/helpers.c                  |  7 +++
 rust/kernel/file.rs             | 80 ++++++++++++++++++++++++++++++++-
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
index 7281264cbaa1..9b1f4efdf7ac 100644
--- a/rust/kernel/file.rs
+++ b/rust/kernel/file.rs
@@ -11,7 +11,8 @@
     error::{code::*, Error, Result},
     types::{ARef, AlwaysRefCounted, Opaque},
 };
-use core::{marker::PhantomData, ptr};
+use alloc::boxed::Box;
+use core::{alloc::AllocError, marker::PhantomData, mem, ptr};
 
 mod poll_table;
 pub use self::poll_table::{PollCondVar, PollTable};
@@ -241,6 +242,83 @@ fn drop(&mut self) {
     }
 }
 
+/// Helper used for closing file descriptors in a way that is safe even if the file is currently
+/// held using `fdget`.
+///
+/// See comments on `binder_do_fd_close` and commit `80cd795630d65`.
+pub struct DeferredFdCloser {
+    inner: Box<DeferredFdCloserInner>,
+}
+
+/// SAFETY: This just holds an allocation with no real content, so there's no safety issue with
+/// moving it across threads.
+unsafe impl Send for DeferredFdCloser {}
+unsafe impl Sync for DeferredFdCloser {}
+
+#[repr(C)]
+struct DeferredFdCloserInner {
+    twork: mem::MaybeUninit<bindings::callback_head>,
+    file: *mut bindings::file,
+}
+
+impl DeferredFdCloser {
+    /// Create a new `DeferredFdCloser`.
+    pub fn new() -> Result<Self, AllocError> {
+        Ok(Self {
+            inner: Box::try_new(DeferredFdCloserInner {
+                twork: mem::MaybeUninit::uninit(),
+                file: core::ptr::null_mut(),
+            })?,
+        })
+    }
+
+    /// Schedule a task work that closes the file descriptor when this task returns to userspace.
+    pub fn close_fd(mut self, fd: u32) {
+        let file = unsafe { bindings::close_fd_get_file(fd) };
+        if !file.is_null() {
+            self.inner.file = file;
+
+            // SAFETY: Since DeferredFdCloserInner is `#[repr(C)]`, casting the pointers gives a
+            // pointer to the `twork` field.
+            let inner = Box::into_raw(self.inner) as *mut bindings::callback_head;
+
+            // SAFETY: Getting a pointer to current is always safe.
+            let current = unsafe { bindings::get_current() };
+            // SAFETY: The `file` pointer points at a valid file.
+            unsafe { bindings::get_file(file) };
+            // SAFETY: Due to the above `get_file`, even if the current task holds an `fdget` to
+            // this file right now, the refcount will not drop to zero until after it is released
+            // with `fdput`. This is because when using `fdget`, you must always use `fdput` before
+            // returning to userspace, and our task work runs after any `fdget` users have returned
+            // to user space.
+            //
+            // Note: fl_owner_t is currently a void pointer.
+            unsafe { bindings::filp_close(file, current as bindings::fl_owner_t) };
+            // SAFETY: The `inner` pointer is compatible with the `do_close_fd` method.
+            //
+            // The call to `task_work_add` can't fail, because we are scheduling the task work to
+            // the current task.
+            unsafe {
+                bindings::init_task_work(inner, Some(Self::do_close_fd));
+                bindings::task_work_add(current, inner, bindings::task_work_notify_mode_TWA_RESUME);
+            }
+        } else {
+            // Free the allocation.
+            drop(self.inner);
+        }
+    }
+
+    unsafe extern "C" fn do_close_fd(inner: *mut bindings::callback_head) {
+        // SAFETY: In `close_fd` we use this method together with a pointer that originates from a
+        // `Box<DeferredFdCloserInner>`, and we have just been given ownership of that allocation.
+        let inner = unsafe { Box::from_raw(inner as *mut DeferredFdCloserInner) };
+        // SAFETY: This drops a refcount we acquired in `close_fd`.
+        unsafe { bindings::fput(inner.file) };
+        // Free the allocation.
+        drop(inner);
+    }
+}
+
 /// Represents the EBADF error code.
 ///
 /// Used for methods that can only fail with EBADF.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 7d83e1a7a362..6d0d044fa8cd 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,7 @@
 
 #include <linux/cred.h>
 #include <linux/errname.h>
+#include <linux/fdtable.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/poll.h>
@@ -16,6 +17,7 @@
 #include <linux/refcount.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/task_work.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/helpers.c b/rust/helpers.c
index e13a7da430b1..d147ec5bc0a3 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -31,6 +31,7 @@
 #include <linux/sched/signal.h>
 #include <linux/security.h>
 #include <linux/spinlock.h>
+#include <linux/task_work.h>
 #include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
@@ -166,6 +167,12 @@ void rust_helper_security_cred_getsecid(const struct cred *c, u32 *secid)
 EXPORT_SYMBOL_GPL(rust_helper_security_cred_getsecid);
 #endif
 
+void rust_helper_init_task_work(struct callback_head *twork, task_work_func_t func)
+{
+	init_task_work(twork, func);
+}
+EXPORT_SYMBOL_GPL(rust_helper_init_task_work);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
-- 
2.41.0.255.g8b1d071c50-goog


  parent reply	other threads:[~2023-07-20 15:29 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-20 15:28 [RFC PATCH v1 0/5] Various Rust bindings for files Alice Ryhl
2023-07-20 15:28 ` [RFC PATCH v1 1/5] rust: file: add bindings for `struct file` Alice Ryhl
2023-08-09  2:59   ` Martin Rodriguez Reboredo
2023-07-20 15:28 ` [RFC PATCH v1 2/5] rust: cred: add Rust bindings for `struct cred` Alice Ryhl
2023-07-20 15:28 ` [RFC PATCH v1 3/5] rust: file: add `FileDescriptorReservation` Alice Ryhl
2023-08-09  4:02   ` Martin Rodriguez Reboredo
2023-07-20 15:28 ` [RFC PATCH v1 4/5] rust: file: add bindings for `poll_table` Alice Ryhl
2023-07-20 15:28 ` Alice Ryhl [this message]
2023-07-20 18:22   ` [RFC PATCH v1 5/5] rust: file: add `DeferredFdCloser` Miguel Ojeda
2023-08-09  4:33   ` Martin Rodriguez Reboredo
2023-08-09  9:00     ` Miguel Ojeda
2023-08-09  9:09       ` Miguel Ojeda
2023-08-09 20:15         ` Martin Rodriguez Reboredo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230720152820.3566078-6-aliceryhl@google.com \
    --to=aliceryhl@google.com \
    --cc=alex.gaynor@gmail.com \
    --cc=benno.lossin@proton.me \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=brauner@kernel.org \
    --cc=gary@garyguo.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ojeda@kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=wedsonaf@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).