Message-ID: <20250620-poll-table-null-v1-1-b3fe92a4fd0d@google.com>
Date: Fri, 20 Jun 2025 11:49:35 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, 
	Miguel Ojeda <ojeda@...nel.org>, Christian Brauner <brauner@...nel.org>
Cc: Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, Alice Ryhl <aliceryhl@...gle.com>
Subject: [PATCH] poll: rust: allow poll_table ptrs to be null

It's possible for a poll_table to be null. This can happen if an
end-user just wants to know if a resource has events right now without
registering a waiter for when events become available. Furthermore,
these null pointers should be handled transparently by the API, so we
should not change `from_ptr` to return an `Option`. Thus, change
`PollTable` to wrap a raw pointer rather than use a reference so that
you can pass null.

Comments mentioning `struct poll_table` are changed to just `poll_table`
since `poll_table` is a typedef. (It's a typedef because it's supposed
to be opaque.)

Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
---
This issue was discovered from a syzkaller report on Rust Binder.

Intended for Christian Brauner's tree.
---
 rust/helpers/helpers.c   |  1 +
 rust/helpers/poll.c      | 10 ++++++++
 rust/kernel/sync/poll.rs | 65 +++++++++++++++++-------------------------------
 3 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0f1b5d11598591bc62bb6439747211af164b76d6..ff65073fe3a5220e7792306fc9ae74c416935a52 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -30,6 +30,7 @@
 #include "platform.c"
 #include "pci.c"
 #include "pid_namespace.c"
+#include "poll.c"
 #include "rbtree.c"
 #include "rcu.c"
 #include "refcount.c"
diff --git a/rust/helpers/poll.c b/rust/helpers/poll.c
new file mode 100644
index 0000000000000000000000000000000000000000..7e5b1751c2d526f2dd467fcd61dcf49294d3c20d
--- /dev/null
+++ b/rust/helpers/poll.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/export.h>
+#include <linux/poll.h>
+
+void rust_helper_poll_wait(struct file *filp, wait_queue_head_t *wait_address,
+			   poll_table *p)
+{
+	poll_wait(filp, wait_address, p);
+}
diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs
index d7e6e59e124b09d5f0d62244ca46089e0748d544..f86e3e565e7e6a6d244a01d34113beb9e799e424 100644
--- a/rust/kernel/sync/poll.rs
+++ b/rust/kernel/sync/poll.rs
@@ -9,9 +9,8 @@
     fs::File,
     prelude::*,
     sync::{CondVar, LockClassKey},
-    types::Opaque,
 };
-use core::ops::Deref;
+use core::{marker::PhantomData, ops::Deref};
 
 /// Creates a [`PollCondVar`] initialiser with the given name and a newly-created lock class.
 #[macro_export]
@@ -23,58 +22,40 @@ macro_rules! new_poll_condvar {
     };
 }
 
-/// Wraps the kernel's `struct poll_table`.
+/// Wraps the kernel's `poll_table`.
 ///
 /// # Invariants
 ///
-/// This struct contains a valid `struct poll_table`.
-///
-/// For a `struct poll_table` to be valid, its `_qproc` function must follow the safety
-/// requirements of `_qproc` functions:
-///
-/// * The `_qproc` function is given permission to enqueue a waiter to the provided `poll_table`
-///   during the call. Once the waiter is removed and an rcu grace period has passed, it must no
-///   longer access the `wait_queue_head`.
+/// The pointer must be null or reference a valid `poll_table`.
 #[repr(transparent)]
-pub struct PollTable(Opaque<bindings::poll_table>);
+pub struct PollTable<'a> {
+    table: *mut bindings::poll_table,
+    _lifetime: PhantomData<&'a bindings::poll_table>,
+}
 
-impl PollTable {
-    /// Creates a reference to a [`PollTable`] from a valid pointer.
+impl<'a> PollTable<'a> {
+    /// Creates a [`PollTable`] from a valid pointer.
     ///
     /// # Safety
     ///
-    /// The caller must ensure that for the duration of `'a`, the pointer will point at a valid poll
-    /// table (as defined in the type invariants).
-    ///
-    /// The caller must also ensure that the `poll_table` is only accessed via the returned
-    /// reference for the duration of `'a`.
-    pub unsafe fn from_ptr<'a>(ptr: *mut bindings::poll_table) -> &'a mut PollTable {
-        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
-        // `PollTable` type being transparent makes the cast ok.
-        unsafe { &mut *ptr.cast() }
-    }
-
-    fn get_qproc(&self) -> bindings::poll_queue_proc {
-        let ptr = self.0.get();
-        // SAFETY: The `ptr` is valid because it originates from a reference, and the `_qproc`
-        // field is not modified concurrently with this call since we have an immutable reference.
-        unsafe { (*ptr)._qproc }
+    /// The pointer must be null or reference a valid `poll_table` for the duration of `'a`.
+    pub unsafe fn from_raw(table: *mut bindings::poll_table) -> Self {
+        // INVARIANTS: The safety requirements are the same as the struct invariants.
+        PollTable {
+            table,
+            _lifetime: PhantomData,
+        }
     }
 
     /// Register this [`PollTable`] with the provided [`PollCondVar`], so that it can be notified
     /// using the condition variable.
-    pub fn register_wait(&mut self, file: &File, cv: &PollCondVar) {
-        if let Some(qproc) = self.get_qproc() {
-            // SAFETY: The pointers to `file` and `self` need to be valid for the duration of this
-            // call to `qproc`, which they are because they are references.
-            //
-            // The `cv.wait_queue_head` pointer must be valid until an rcu grace period after the
-            // waiter is removed. The `PollCondVar` is pinned, so before `cv.wait_queue_head` can
-            // be destroyed, the destructor must run. That destructor first removes all waiters,
-            // and then waits for an rcu grace period. Therefore, `cv.wait_queue_head` is valid for
-            // long enough.
-            unsafe { qproc(file.as_ptr() as _, cv.wait_queue_head.get(), self.0.get()) };
-        }
+    pub fn register_wait(&self, file: &File, cv: &PollCondVar) {
+        // SAFETY: The pointers `self.table` and `file` are valid for the duration of this call.
+        // The `cv.wait_queue_head` pointer must be valid until an rcu grace period after the
+        // waiter is removed. The `PollCondVar` is pinned, so before `cv.wait_queue_head` can be
+        // destroyed, the destructor must run. That destructor first removes all waiters, and then
+        // waits for an rcu grace period. Therefore, `cv.wait_queue_head` is valid for long enough.
+        unsafe { bindings::poll_wait(file.as_ptr(), cv.wait_queue_head.get(), self.table) }
     }
 }
 

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250620-poll-table-null-bf9a6a6c569e

Best regards,
-- 
Alice Ryhl <aliceryhl@...gle.com>

Powered by blists - more mailing lists