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

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

[Alice Ryhl]

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@google.com>
---
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 {
+        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@google.com>

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

[Christian Brauner]

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@google.com>
> ---
> 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?

Absolutely.

> ---
>  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 {
> +        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@google.com>
> 

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

[Christian Brauner]

On Tue, 15 Oct 2024 14:02:12 +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.
> 
> [...]

Applied to the vfs.rust.file branch of the vfs/vfs.git tree.
Patches in the vfs.rust.file branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.rust.file

[1/1] rust: task: adjust safety comments in Task methods
      https://git.kernel.org/vfs/vfs/c/fe95f58320e6

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

[Alice Ryhl]

On Tue, Oct 15, 2024 at 4:06 PM Christian Brauner <brauner@kernel.org> wrote:
>
> 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@google.com>
> > ---
> > 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?
>
> Absolutely.

Thanks!

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@google.com>
> ---
> 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@google.com>
> 

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

[Alice Ryhl]

On 10/15/24 8:24 PM, Boqun Feng wrote:
> 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@google.com>
>> ---
>> 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.
Both names are in use. See e.g. Page and File that use as_ptr.

In fact, I was asked to change the name on File *to* as_ptr.

Alice

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

[Boqun Feng]

On Tue, Oct 15, 2024 at 08:37:36PM +0200, Alice Ryhl wrote:
> On 10/15/24 8:24 PM, Boqun Feng wrote:
> > 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@google.com>
> > > ---
> > > 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.
> Both names are in use. See e.g. Page and File that use as_ptr.
> 

`Page` is a different case because it currently is a pointer.

> In fact, I was asked to change the name on File *to* as_ptr.
> 

I'm not able to find the discussion on that ask. Appreciate it if you
can share a link.

Anyway, this is not important for now, and might not be in the future.
So:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> Alice

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

[Alice Ryhl]

On 10/15/24 8:45 PM, Boqun Feng wrote:
> On Tue, Oct 15, 2024 at 08:37:36PM +0200, Alice Ryhl wrote:
>> On 10/15/24 8:24 PM, Boqun Feng wrote:
>>> 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@google.com>
>>>> ---
>>>> 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.
>> Both names are in use. See e.g. Page and File that use as_ptr.
>>
> 
> `Page` is a different case because it currently is a pointer.
> 
>> In fact, I was asked to change the name on File *to* as_ptr.
>>
> 
> I'm not able to find the discussion on that ask. Appreciate it if you
> can share a link.

Hmm, it's possible that I misremember. I can't find it either. But it is 
called as_ptr on File.

> Anyway, this is not important for now, and might not be in the future.
> So:
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Thanks!

Alice