[PATCH] poll: rust: allow poll_table ptrs to be null

[PATCH] poll: rust: allow poll_table ptrs to be null

[Alice Ryhl]

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@google.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@google.com>

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Benno Lossin]

On Fri Jun 20, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
>      ///
>      /// # 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 {

Returning `Option<&'a mut PollTable>` is not an option? I'd like to
avoid wrapping raw pointers...

---
Cheers,
Benno

> -        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> -        // `PollTable` type being transparent makes the cast ok.
> -        unsafe { &mut *ptr.cast() }
> -    }

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Alice Ryhl]

On Fri, Jun 20, 2025 at 2:31 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Fri Jun 20, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> >      ///
> >      /// # 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 {
>
> Returning `Option<&'a mut PollTable>` is not an option? I'd like to
> avoid wrapping raw pointers...

You're going to make people handle the Option by early-returning if
you do that, but that's wrong. You're supposed to treat null and
non-null the same.

Alice

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Benno Lossin]

On Fri Jun 20, 2025 at 3:19 PM CEST, Alice Ryhl wrote:
> On Fri, Jun 20, 2025 at 2:31 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> On Fri Jun 20, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
>> >      ///
>> >      /// # 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 {
>>
>> Returning `Option<&'a mut PollTable>` is not an option? I'd like to
>> avoid wrapping raw pointers...
>
> You're going to make people handle the Option by early-returning if
> you do that, but that's wrong. You're supposed to treat null and
> non-null the same.

Ah right...

An `PollTableInner` type that wraps the `bindings::poll_table` opaquely
sounds like too much work, so let's go with your approach.

---
Cheers,
Benno

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Benno Lossin]

On Fri Jun 20, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> 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@google.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(-)

Looks good, one safety comment concern below, with that fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

>      /// 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.

`self.table` might be null, which I think we agreed to is not "valid".

> +        // 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.

Could you use bullet points for the different requirements?

---
Cheers,
Benno

> +        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,

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Christian Brauner]

On Fri, 20 Jun 2025 11:49:35 +0000, Alice Ryhl wrote:
> 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.
> 
> [...]

Applied to the vfs-6.17.rust branch of the vfs/vfs.git tree.
Patches in the vfs-6.17.rust branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.17.rust

[1/1] poll: rust: allow poll_table ptrs to be null
      https://git.kernel.org/vfs/vfs/c/6efbf978891b

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Benno Lossin]

On Mon Jun 23, 2025 at 1:56 PM CEST, Christian Brauner wrote:
> On Fri, 20 Jun 2025 11:49:35 +0000, Alice Ryhl wrote:
>> 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.
>> 
>> [...]
>
> Applied to the vfs-6.17.rust branch of the vfs/vfs.git tree.
> Patches in the vfs-6.17.rust branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-6.17.rust
>
> [1/1] poll: rust: allow poll_table ptrs to be null
>       https://git.kernel.org/vfs/vfs/c/6efbf978891b

You took my RB without changing the safety comment that I had a concern
about. Can we fix that before you send the pull request?

---
Cheers,
Benno

Re: [PATCH] poll: rust: allow poll_table ptrs to be null

[Alice Ryhl]

On Mon, Jun 23, 2025 at 12:56 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, 20 Jun 2025 11:49:35 +0000, Alice Ryhl wrote:
> > 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.
> >
> > [...]
>
> Applied to the vfs-6.17.rust branch of the vfs/vfs.git tree.
> Patches in the vfs-6.17.rust branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-6.17.rust
>
> [1/1] poll: rust: allow poll_table ptrs to be null
>       https://git.kernel.org/vfs/vfs/c/6efbf978891b

I just sent a v2, would you be able to pick that up instead? Thanks!