From fe95f58320e6c8dcea3bcb01336b9a7fdd7f684b Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Tue, 15 Oct 2024 14:02:12 +0000 Subject: [PATCH] rust: task: adjust safety comments in Task methods 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 Link: https://lore.kernel.org/r/20241015-task-safety-cmnts-v1-1-46ee92c82768@google.com Signed-off-by: Christian Brauner --- 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 @@ impl Task { } } + /// Returns a raw pointer to the task. + #[inline] + pub fn as_ptr(&self) -> *mut bindings::task_struct { + 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 @@ impl 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 @@ impl Task { 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) {