Re: [PATCH] rust: task: adjust safety comments in Task methods

[Boqun Feng]

On Tue, Oct 15, 2024 at 02:02:12PM +0000, Alice Ryhl wrote:
> The `Task` struct has several safety comments that aren't so great. For
> example, the reason that it's okay to read the `pid` is that the field
> is immutable, so there is no data race, which is not what the safety
> comment says.
> 
> Thus, improve the safety comments. Also add an `as_ptr` helper. This
> makes it easier to read the various accessors on Task, as `self.0` may
> be confusing syntax for new Rust users.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> ---
> This is based on top of vfs.rust.file as the file series adds some new
> task methods. Christian, can you take this through that tree?
> ---
>  rust/kernel/task.rs | 43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 1a36a9f19368..080599075875 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -145,11 +145,17 @@ fn deref(&self) -> &Self::Target {
>          }
>      }
>  
> +    /// Returns a raw pointer to the task.
> +    #[inline]
> +    pub fn as_ptr(&self) -> *mut bindings::task_struct {

FWIW, I think the name convention is `as_raw()` for a wrapper type of
`Opaque<T>` to return `*mut T`, e.g. `kernel::device::Device`.

Otherwise this looks good to me.

Regards,
Boqun

> +        self.0.get()
> +    }
> +
>      /// Returns the group leader of the given task.
>      pub fn group_leader(&self) -> &Task {
> -        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> -        // have a valid `group_leader`.
> -        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
> +        // SAFETY: The group leader of a task never changes after initialization, so reading this
> +        // field is not a data race.
> +        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).group_leader) };
>  
>          // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
>          // and given that a task has a reference to its group leader, we know it must be valid for
> @@ -159,42 +165,41 @@ pub fn group_leader(&self) -> &Task {
>  
>      /// Returns the PID of the given task.
>      pub fn pid(&self) -> Pid {
> -        // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
> -        // have a valid pid.
> -        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> +        // SAFETY: The pid of a task never changes after initialization, so reading this field is
> +        // not a data race.
> +        unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
>      }
>  
>      /// Returns the UID of the given task.
>      pub fn uid(&self) -> Kuid {
> -        // SAFETY: By the type invariant, we know that `self.0` is valid.
> -        Kuid::from_raw(unsafe { bindings::task_uid(self.0.get()) })
> +        // SAFETY: It's always safe to call `task_uid` on a valid task.
> +        Kuid::from_raw(unsafe { bindings::task_uid(self.as_ptr()) })
>      }
>  
>      /// Returns the effective UID of the given task.
>      pub fn euid(&self) -> Kuid {
> -        // SAFETY: By the type invariant, we know that `self.0` is valid.
> -        Kuid::from_raw(unsafe { bindings::task_euid(self.0.get()) })
> +        // SAFETY: It's always safe to call `task_euid` on a valid task.
> +        Kuid::from_raw(unsafe { bindings::task_euid(self.as_ptr()) })
>      }
>  
>      /// Determines whether the given task has pending signals.
>      pub fn signal_pending(&self) -> bool {
> -        // SAFETY: By the type invariant, we know that `self.0` is valid.
> -        unsafe { bindings::signal_pending(self.0.get()) != 0 }
> +        // SAFETY: It's always safe to call `signal_pending` on a valid task.
> +        unsafe { bindings::signal_pending(self.as_ptr()) != 0 }
>      }
>  
>      /// Returns the given task's pid in the current pid namespace.
>      pub fn pid_in_current_ns(&self) -> Pid {
> -        // SAFETY: We know that `self.0.get()` is valid by the type invariant, and passing a null
> -        // pointer as the namespace is correct for using the current namespace.
> -        unsafe { bindings::task_tgid_nr_ns(self.0.get(), ptr::null_mut()) }
> +        // SAFETY: It's valid to pass a null pointer as the namespace (defaults to current
> +        // namespace). The task pointer is also valid.
> +        unsafe { bindings::task_tgid_nr_ns(self.as_ptr(), ptr::null_mut()) }
>      }
>  
>      /// Wakes up the task.
>      pub fn wake_up(&self) {
> -        // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
> -        // And `wake_up_process` is safe to be called for any valid task, even if the task is
> +        // SAFETY: It's always safe to call `signal_pending` on a valid task, even if the task
>          // running.
> -        unsafe { bindings::wake_up_process(self.0.get()) };
> +        unsafe { bindings::wake_up_process(self.as_ptr()) };
>      }
>  }
>  
> @@ -202,7 +207,7 @@ pub fn wake_up(&self) {
>  unsafe impl crate::types::AlwaysRefCounted for Task {
>      fn inc_ref(&self) {
>          // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> -        unsafe { bindings::get_task_struct(self.0.get()) };
> +        unsafe { bindings::get_task_struct(self.as_ptr()) };
>      }
>  
>      unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> 
> ---
> base-commit: 22018a5a54a3d353bf0fee7364b2b8018ed4c5a6
> change-id: 20241015-task-safety-cmnts-a7cfe4b2c470
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@xxxxxxxxxx>
>