* [PATCH] rust: task: add `as_raw()` to `Task`
@ 2024-01-16 2:28 Antonio Hickey
2024-01-16 2:28 ` [PATCH] rust: task: use safe `current!` macro Antonio Hickey
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Antonio Hickey @ 2024-01-16 2:28 UTC (permalink / raw)
To: ojeda, alex.gaynor, wedsonaf
Cc: boqun.feng, gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl,
rust-for-linux, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] rust: task: use safe `current!` macro
2024-01-16 2:28 [PATCH] rust: task: add `as_raw()` to `Task` Antonio Hickey
@ 2024-01-16 2:28 ` Antonio Hickey
2024-01-16 8:57 ` Alice Ryhl
2024-01-16 7:20 ` [PATCH] rust: task: add `as_raw()` to `Task` Greg KH
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Antonio Hickey @ 2024-01-16 2:28 UTC (permalink / raw)
To: ojeda, alex.gaynor, wedsonaf
Cc: boqun.feng, gary, bjorn3_gh, benno.lossin, a.hindborg, aliceryhl,
rust-for-linux, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: add `as_raw()` to `Task`
2024-01-16 2:28 [PATCH] rust: task: add `as_raw()` to `Task` Antonio Hickey
2024-01-16 2:28 ` [PATCH] rust: task: use safe `current!` macro Antonio Hickey
@ 2024-01-16 7:20 ` Greg KH
2024-01-16 7:29 ` Wedson Almeida Filho
2024-01-16 18:24 ` Boqun Feng
3 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2024-01-16 7:20 UTC (permalink / raw)
To: Antonio Hickey
Cc: ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, rust-for-linux
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: add `as_raw()` to `Task`
2024-01-16 2:28 [PATCH] rust: task: add `as_raw()` to `Task` Antonio Hickey
2024-01-16 2:28 ` [PATCH] rust: task: use safe `current!` macro Antonio Hickey
2024-01-16 7:20 ` [PATCH] rust: task: add `as_raw()` to `Task` Greg KH
@ 2024-01-16 7:29 ` Wedson Almeida Filho
2024-01-16 8:53 ` Alice Ryhl
2024-01-16 18:24 ` Boqun Feng
3 siblings, 1 reply; 8+ messages in thread
From: Wedson Almeida Filho @ 2024-01-16 7:29 UTC (permalink / raw)
To: Antonio Hickey
Cc: ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, rust-for-linux
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`?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: add `as_raw()` to `Task`
2024-01-16 7:29 ` Wedson Almeida Filho
@ 2024-01-16 8:53 ` Alice Ryhl
0 siblings, 0 replies; 8+ messages in thread
From: Alice Ryhl @ 2024-01-16 8:53 UTC (permalink / raw)
To: Wedson Almeida Filho
Cc: Antonio Hickey, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, rust-for-linux
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: use safe `current!` macro
2024-01-16 2:28 ` [PATCH] rust: task: use safe `current!` macro Antonio Hickey
@ 2024-01-16 8:57 ` Alice Ryhl
0 siblings, 0 replies; 8+ messages in thread
From: Alice Ryhl @ 2024-01-16 8:57 UTC (permalink / raw)
To: Antonio Hickey
Cc: ojeda, alex.gaynor, wedsonaf, boqun.feng, gary, bjorn3_gh,
benno.lossin, a.hindborg, rust-for-linux
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/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: add `as_raw()` to `Task`
2024-01-16 2:28 [PATCH] rust: task: add `as_raw()` to `Task` Antonio Hickey
` (2 preceding siblings ...)
2024-01-16 7:29 ` Wedson Almeida Filho
@ 2024-01-16 18:24 ` Boqun Feng
2024-01-16 19:03 ` Miguel Ojeda
3 siblings, 1 reply; 8+ messages in thread
From: Boqun Feng @ 2024-01-16 18:24 UTC (permalink / raw)
To: Antonio Hickey
Cc: ojeda, alex.gaynor, wedsonaf, gary, bjorn3_gh, benno.lossin,
a.hindborg, aliceryhl, rust-for-linux
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rust: task: add `as_raw()` to `Task`
2024-01-16 18:24 ` Boqun Feng
@ 2024-01-16 19:03 ` Miguel Ojeda
0 siblings, 0 replies; 8+ messages in thread
From: Miguel Ojeda @ 2024-01-16 19:03 UTC (permalink / raw)
To: Boqun Feng
Cc: Antonio Hickey, ojeda, alex.gaynor, wedsonaf, gary, bjorn3_gh,
benno.lossin, a.hindborg, aliceryhl, rust-for-linux
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-01-16 19:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-16 2:28 [PATCH] rust: task: add `as_raw()` to `Task` Antonio Hickey
2024-01-16 2:28 ` [PATCH] rust: task: use safe `current!` macro Antonio Hickey
2024-01-16 8:57 ` Alice Ryhl
2024-01-16 7:20 ` [PATCH] rust: task: add `as_raw()` to `Task` Greg KH
2024-01-16 7:29 ` Wedson Almeida Filho
2024-01-16 8:53 ` Alice Ryhl
2024-01-16 18:24 ` Boqun Feng
2024-01-16 19:03 ` Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).