[PATCH] rust: task: add `as_raw()` to `Task`

[PATCH] rust: task: add `as_raw()` to `Task`

[Antonio Hickey]

Added new function `Task::as_raw()` which returns the raw pointer
for the underlying task struct. I also refactored `Task` to instead
use the newly created function instead of `self.0.get()` as I feel
like `self.as_raw()` is more intuitive.

Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
---
 rust/kernel/task.rs | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index d2f2615fe4a1..72737f5d86ab 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -124,11 +124,16 @@ fn deref(&self) -> &Self::Target {
         }
     }
 
+    /// Returns a raw pointer to the underlying C task struct.
+    pub fn as_raw(&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
+        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always
         // have a valid group_leader.
-        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
+        let ptr = unsafe { *ptr::addr_of!((*self.as_raw()).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
@@ -138,43 +143,43 @@ 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
+        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always
         // have a valid pid.
-        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
+        unsafe { *ptr::addr_of!((*self.as_raw()).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: By the type invariant, we know that `self.as_raw()` is valid.
+        Kuid::from_raw(unsafe { bindings::task_uid(self.as_raw()) })
     }
 
     /// 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: By the type invariant, we know that `self.as_raw()` is valid.
+        Kuid::from_raw(unsafe { bindings::task_euid(self.as_raw()) })
     }
 
     /// 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: By the type invariant, we know that `self.as_raw()` is valid.
+        unsafe { bindings::signal_pending(self.as_raw()) != 0 }
     }
 
     /// Returns the given task's pid in the current pid namespace.
     pub fn pid_in_current_ns(&self) -> Pid {
         // SAFETY: Calling `task_active_pid_ns` with the current task is always safe.
         let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) };
-        // SAFETY: We know that `self.0.get()` is valid by the type invariant.
-        unsafe { bindings::task_tgid_nr_ns(self.0.get(), namespace) }
+        // SAFETY: We know that `self.raw()` is valid by the type invariant.
+        unsafe { bindings::task_tgid_nr_ns(self.as_raw(), namespace) }
     }
 
     /// 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.
+        // SAFETY: By the type invariant, we know that `self.raw()` is non-null and valid.
         // And `wake_up_process` is safe to be called for any valid task, even if the task is
         // running.
-        unsafe { bindings::wake_up_process(self.0.get()) };
+        unsafe { bindings::wake_up_process(self.as_raw()) };
     }
 }
 
-- 
2.43.0

[PATCH] rust: task: use safe `current!` macro

[Antonio Hickey]

Refactor the `Task::pid_in_current_ns()` to use the safe abstraction
`current!()` instead of the unsafe `bindings::get_current()` binding.

Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
---
 rust/kernel/task.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 72737f5d86ab..ffe08c348b72 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -168,8 +168,9 @@ pub fn signal_pending(&self) -> bool {
 
     /// Returns the given task's pid in the current pid namespace.
     pub fn pid_in_current_ns(&self) -> Pid {
+        let current = current!();
         // SAFETY: Calling `task_active_pid_ns` with the current task is always safe.
-        let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) };
+        let namespace = unsafe { bindings::task_active_pid_ns(current.as_raw()) };
         // SAFETY: We know that `self.raw()` is valid by the type invariant.
         unsafe { bindings::task_tgid_nr_ns(self.as_raw(), namespace) }
     }
-- 
2.43.0

Re: [PATCH] rust: task: use safe `current!` macro

[Alice Ryhl]

On Tue, Jan 16, 2024 at 3:29 AM Antonio Hickey
<antoniohickey99@gmail.com> wrote:
>
> Refactor the `Task::pid_in_current_ns()` to use the safe abstraction
> `current!()` instead of the unsafe `bindings::get_current()` binding.
>
> Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>

It looks like this patch is based on my patchset with file bindings
[1]. Please make it clear what you are basing your patch on when it is
based on top of another pending patchset.

There will be another version of [1] before it can go upstream, and
the next version will not need this fix.

Alice

[1]: https://lore.kernel.org/all/20231206-alice-file-v2-5-af617c0d9d94@google.com/

Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Greg KH]

On Mon, Jan 15, 2024 at 09:28:22PM -0500, Antonio Hickey wrote:
> Added new function `Task::as_raw()` which returns the raw pointer
> for the underlying task struct. I also refactored `Task` to instead
> use the newly created function instead of `self.0.get()` as I feel
> like `self.as_raw()` is more intuitive.

Nit, this says _what_ you are doing, but not _why_ you are doing it.

Why do you need "as_raw"?  Who is going to use it?  What is it good for?

thanks,

greg k-h

Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Wedson Almeida Filho]

On Mon, 15 Jan 2024 at 23:29, Antonio Hickey <antoniohickey99@gmail.com> wrote:
> +    /// Returns a raw pointer to the underlying C task struct.
> +    pub fn as_raw(&self) -> *mut bindings::task_struct {
> +        self.0.get()
> +    }
> +

Note that the wrapped `bindings::task_struct` in `Task` was
`pub(crate)`, but you're making `as_raw` fully public.

We don't want  modules to use the definitions in `bindings` (only
subsystems). So I'd suggest limiting `as_raw` to `pub(crate)`.

>      /// 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.
> +        // SAFETY: By the type invariant, we know that `self.raw()` is non-null and valid.

Typo in the comment: `raw` vs `as_raw`?

Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Alice Ryhl]

On Tue, Jan 16, 2024 at 8:29 AM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> On Mon, 15 Jan 2024 at 23:29, Antonio Hickey <antoniohickey99@gmail.com> wrote:
> > +    /// Returns a raw pointer to the underlying C task struct.
> > +    pub fn as_raw(&self) -> *mut bindings::task_struct {
> > +        self.0.get()
> > +    }
> > +
>
> Note that the wrapped `bindings::task_struct` in `Task` was
> `pub(crate)`, but you're making `as_raw` fully public.
>
> We don't want  modules to use the definitions in `bindings` (only
> subsystems). So I'd suggest limiting `as_raw` to `pub(crate)`.

I don't agree that this policy should be so tight as to prevent
conversions to/from raw pointers from being public. I need some such
conversions in Rust Binder to interface with binderfs.

Alice

Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Boqun Feng]

On Mon, Jan 15, 2024 at 09:28:22PM -0500, Antonio Hickey wrote:
> Added new function `Task::as_raw()` which returns the raw pointer
> for the underlying task struct. I also refactored `Task` to instead
> use the newly created function instead of `self.0.get()` as I feel
> like `self.as_raw()` is more intuitive.
> 
> Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
> ---
>  rust/kernel/task.rs | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index d2f2615fe4a1..72737f5d86ab 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -124,11 +124,16 @@ fn deref(&self) -> &Self::Target {
>          }
>      }
>  
> +    /// Returns a raw pointer to the underlying C task struct.
> +    pub fn as_raw(&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
> +        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always

You will need to update the type invariant on `as_raw()` then, if you
want to use it here ;-)

>          // have a valid group_leader.
> -        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
> +        let ptr = unsafe { *ptr::addr_of!((*self.as_raw()).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
> @@ -138,43 +143,43 @@ 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
> +        // SAFETY: By the type invariant, we know that `self.as_raw()` is a valid task. Valid tasks always
>          // have a valid pid.
> -        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> +        unsafe { *ptr::addr_of!((*self.as_raw()).pid) }
[...]
>      /// 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.
> +        // SAFETY: By the type invariant, we know that `self.raw()` is non-null and valid.

While we are at it, maybe we can clarify the terminology a bit,
according to:

	https://doc.rust-lang.org/core/ptr/index.html#safety

"valid" implies "non-null", so here we can just say:

	we know that `self.raw()` is a valid task pointer.

Of course, this only applies when we are talking about the validity of
a pointer in Rust. In other words, it's OK to say "null is the valid
input for this C function".

Regards,
Boqun

>          // And `wake_up_process` is safe to be called for any valid task, even if the task is
>          // running.
> -        unsafe { bindings::wake_up_process(self.0.get()) };
> +        unsafe { bindings::wake_up_process(self.as_raw()) };
>      }
>  }
>  
> -- 
> 2.43.0
> 

Re: [PATCH] rust: task: add `as_raw()` to `Task`

[Miguel Ojeda]

On Tue, Jan 16, 2024 at 7:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> While we are at it, maybe we can clarify the terminology a bit,
> according to:
>
>         https://doc.rust-lang.org/core/ptr/index.html#safety
>
> "valid" implies "non-null", so here we can just say:

I think Wedson and I discussed this term a few times and whether to
avoid saying non-null or not. If we go ahead with this, then it would
be nice to clean all the instances across the board (we could create a
"good first issue" for it).

Cheers,
Miguel