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

["Benno Lossin" <lossin@kernel.org>]

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,