Message-ID: <20241001201054.73167686.gary@garyguo.net>
Date: Tue, 1 Oct 2024 20:10:54 +0100
From: Gary Guo <gary@...yguo.net>
To: Christian Brauner <brauner@...nel.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>, rust-for-linux@...r.kernel.org, Paul
 Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, "Serge E.
 Hallyn" <serge@...lyn.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor
 <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun
 Feng <boqun.feng@...il.com>, Bjoern Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>, Peter Zijlstra
 <peterz@...radead.org>, Alexander Viro <viro@...iv.linux.org.uk>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Arve Hjonnevag
 <arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen
 <maco@...roid.com>, Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas
 <cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>, Dan Williams
 <dan.j.williams@...el.com>, Matthew Wilcox <willy@...radead.org>, Thomas
 Gleixner <tglx@...utronix.de>, Daniel Xu <dxu@...uu.xyz>, Martin Rodriguez
 Reboredo <yakoyoku@...il.com>, Trevor Gross <tmgross@...ch.edu>,
 linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, Kees Cook <kees@...nel.org>, Andreas
 Hindborg <a.hindborg@...nel.org>
Subject: Re: [PATCH v2] rust: add PidNamespace

On Tue, 01 Oct 2024 11:43:42 +0200
Christian Brauner <brauner@...nel.org> wrote:

> The lifetime of `PidNamespace` is bound to `Task` and `struct pid`.
> 
> The `PidNamespace` of a `Task` doesn't ever change once the `Task` is
> alive. A `unshare(CLONE_NEWPID)` or `setns(fd_pidns/pidfd, CLONE_NEWPID)`
> will not have an effect on the calling `Task`'s pid namespace. It will
> only effect the pid namespace of children created by the calling `Task`.
> This invariant guarantees that after having acquired a reference to a
> `Task`'s pid namespace it will remain unchanged.
> 
> When a task has exited and been reaped `release_task()` will be called.
> This will set the `PidNamespace` of the task to `NULL`. So retrieving
> the `PidNamespace` of a task that is dead will return `NULL`. Note, that
> neither holding the RCU lock nor holding a referencing count to the
> `Task` will prevent `release_task()` being called.
> 
> In order to retrieve the `PidNamespace` of a `Task` the
> `task_active_pid_ns()` function can be used. There are two cases to
> consider:
> 
> (1) retrieving the `PidNamespace` of the `current` task (2) retrieving
> the `PidNamespace` of a non-`current` task
> 
> From system call context retrieving the `PidNamespace` for case (1) is
> always safe and requires neither RCU locking nor a reference count to be
> held. Retrieving the `PidNamespace` after `release_task()` for current
> will return `NULL` but no codepath like that is exposed to Rust.
> 
> Retrieving the `PidNamespace` from system call context for (2) requires
> RCU protection. Accessing `PidNamespace` outside of RCU protection
> requires a reference count that must've been acquired while holding the
> RCU lock. Note that accessing a non-`current` task means `NULL` can be
> returned as the non-`current` task could have already passed through
> `release_task()`.
> 
> To retrieve (1) the `current_pid_ns!()` macro should be used which
> ensure that the returned `PidNamespace` cannot outlive the calling
> scope. The associated `current_pid_ns()` function should not be called
> directly as it could be abused to created an unbounded lifetime for
> `PidNamespace`. The `current_pid_ns!()` macro allows Rust to handle the
> common case of accessing `current`'s `PidNamespace` without RCU
> protection and without having to acquire a reference count.
> 
> For (2) the `task_get_pid_ns()` method must be used. This will always
> acquire a reference on `PidNamespace` and will return an `Option` to
> force the caller to explicitly handle the case where `PidNamespace` is
> `None`, something that tends to be forgotten when doing the equivalent
> operation in `C`. Missing RCU primitives make it difficult to perform
> operations that are otherwise safe without holding a reference count as
> long as RCU protection is guaranteed. But it is not important currently.
> But we do want it in the future.
> 
> Note for (2) the required RCU protection around calling
> `task_active_pid_ns()` synchronizes against putting the last reference
> of the associated `struct pid` of `task->thread_pid`. The `struct pid`
> stored in that field is used to retrieve the `PidNamespace` of the
> caller. When `release_task()` is called `task->thread_pid` will be
> `NULL`ed and `put_pid()` on said `struct pid` will be delayed in
> `free_pid()` via `call_rcu()` allowing everyone with an RCU protected
> access to the `struct pid` acquired from `task->thread_pid` to finish.
> 
> Signed-off-by: Christian Brauner <brauner@...nel.org>
> ---
>  rust/helpers/helpers.c       |   1 +
>  rust/helpers/pid_namespace.c |  26 ++++++++++
>  rust/kernel/lib.rs           |   1 +
>  rust/kernel/pid_namespace.rs |  70 +++++++++++++++++++++++++
>  rust/kernel/task.rs          | 119 ++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 211 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 62022b18caf5ec17231fd0e7be1234592d1146e3..d553ad9361ce17950d505c3b372a568730020e2f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -17,6 +17,7 @@
>  #include "kunit.c"
>  #include "mutex.c"
>  #include "page.c"
> +#include "pid_namespace.c"
>  #include "rbtree.c"
>  #include "refcount.c"
>  #include "security.c"
> diff --git a/rust/helpers/pid_namespace.c b/rust/helpers/pid_namespace.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f41482bdec9a7c4e84b81ec141027fbd65251230
> --- /dev/null
> +++ b/rust/helpers/pid_namespace.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/pid_namespace.h>
> +#include <linux/cleanup.h>
> +
> +struct pid_namespace *rust_helper_get_pid_ns(struct pid_namespace *ns)
> +{
> +	return get_pid_ns(ns);
> +}
> +
> +void rust_helper_put_pid_ns(struct pid_namespace *ns)
> +{
> +	put_pid_ns(ns);
> +}
> +
> +/* Get a reference on a task's pid namespace. */
> +struct pid_namespace *rust_helper_task_get_pid_ns(struct task_struct *task)
> +{
> +	struct pid_namespace *pid_ns;
> +
> +	guard(rcu)();
> +	pid_ns = task_active_pid_ns(task);
> +	if (pid_ns)
> +		get_pid_ns(pid_ns);
> +	return pid_ns;
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ff7d88022c57ca232dc028066dfa062f3fc84d1c..0e78ec9d06e0199dfafc40988a2ae86cd5df949c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -44,6 +44,7 @@
>  #[cfg(CONFIG_NET)]
>  pub mod net;
>  pub mod page;
> +pub mod pid_namespace;
>  pub mod prelude;
>  pub mod print;
>  pub mod sizes;
> diff --git a/rust/kernel/pid_namespace.rs b/rust/kernel/pid_namespace.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..9a0509e802b4939ad853a802ee6d069a5f00c9df
> --- /dev/null
> +++ b/rust/kernel/pid_namespace.rs
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (c) 2024 Christian Brauner <brauner@...nel.org>
> +
> +//! Pid namespaces.
> +//!
> +//! C header: [`include/linux/pid_namespace.h`](srctree/include/linux/pid_namespace.h) and
> +//! [`include/linux/pid.h`](srctree/include/linux/pid.h)
> +
> +use crate::{
> +    bindings,
> +    types::{AlwaysRefCounted, Opaque},
> +};
> +use core::{
> +    ptr,
> +};
> +
> +/// Wraps the kernel's `struct pid_namespace`. Thread safe.
> +///
> +/// This structure represents the Rust abstraction for a C `struct pid_namespace`. This
> +/// implementation abstracts the usage of an already existing C `struct pid_namespace` within Rust
> +/// code that we get passed from the C side.
> +#[repr(transparent)]
> +pub struct PidNamespace {
> +    inner: Opaque<bindings::pid_namespace>,
> +}
> +
> +impl PidNamespace {
> +    /// Returns a raw pointer to the inner C struct.
> +    #[inline]
> +    pub fn as_ptr(&self) -> *mut bindings::pid_namespace {
> +        self.inner.get()
> +    }
> +
> +    /// Creates a reference to a [`PidNamespace`] from a valid pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> +    /// returned [`PidNamespace`] reference.
> +    pub unsafe fn from_ptr<'a>(ptr: *const bindings::pid_namespace) -> &'a Self {
> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> +        // `PidNamespace` type being transparent makes the cast ok.
> +        unsafe { &*ptr.cast() }
> +    }
> +}
> +
> +// SAFETY: Instances of `PidNamespace` are always reference-counted.
> +unsafe impl AlwaysRefCounted for PidNamespace {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::get_pid_ns(self.as_ptr()) };
> +    }
> +
> +    #[inline]
> +    unsafe fn dec_ref(obj: ptr::NonNull<PidNamespace>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::put_pid_ns(obj.cast().as_ptr()) }
> +    }
> +}
> +
> +// SAFETY:
> +// - `PidNamespace::dec_ref` can be called from any thread.
> +// - It is okay to send ownership of `PidNamespace` across thread boundaries.
> +unsafe impl Send for PidNamespace {}
> +
> +// SAFETY: It's OK to access `PidNamespace` through shared references from other threads because
> +// we're either accessing properties that don't change or that are properly synchronised by C code.
> +unsafe impl Sync for PidNamespace {}
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 1a36a9f193685393e7211793b6e6dd7576af8bfd..92603cdb543d9617f1f7d092edb87ccb66c9f0c1 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -6,7 +6,8 @@
>  
>  use crate::{
>      bindings,
> -    types::{NotThreadSafe, Opaque},
> +    pid_namespace::PidNamespace,
> +    types::{ARef, NotThreadSafe, Opaque},
>  };
>  use core::{
>      cmp::{Eq, PartialEq},
> @@ -36,6 +37,65 @@ macro_rules! current {
>      };
>  }
>  
> +/// Returns the currently running task's pid namespace.
> +///
> +/// The lifetime of `PidNamespace` is bound to `Task` and `struct pid`.
> +///
> +/// The `PidNamespace` of a `Task` doesn't ever change once the `Task` is alive. A
> +/// `unshare(CLONE_NEWPID)` or `setns(fd_pidns/pidfd, CLONE_NEWPID)` will not have an effect on the
> +/// calling `Task`'s pid namespace. It will only effect the pid namespace of children created by
> +/// the calling `Task`. This invariant guarantees that after having acquired a reference to a
> +/// `Task`'s pid namespace it will remain unchanged.
> +///
> +/// When a task has exited and been reaped `release_task()` will be called. This will set the
> +/// `PidNamespace` of the task to `NULL`. So retrieving the `PidNamespace` of a task that is dead
> +/// will return `NULL`. Note, that neither holding the RCU lock nor holding a referencing count to
> +/// the `Task` will prevent `release_task()` being called.
> +///
> +/// In order to retrieve the `PidNamespace` of a `Task` the `task_active_pid_ns()` function can be
> +/// used. There are two cases to consider:
> +///
> +/// (1) retrieving the `PidNamespace` of the `current` task
> +/// (2) retrieving the `PidNamespace` of a non-`current` task
> +///
> +/// From system call context retrieving the `PidNamespace` for case (1) is always safe and requires
> +/// neither RCU locking nor a reference count to be held. Retrieving the `PidNamespace` after
> +/// `release_task()` for current will return `NULL` but no codepath like that is exposed to Rust.
> +///
> +/// Retrieving the `PidNamespace` from system call context for (2) requires RCU protection.
> +/// Accessing `PidNamespace` outside of RCU protection requires a reference count that must've been
> +/// acquired while holding the RCU lock. Note that accessing a non-`current` task means `NULL` can
> +/// be returned as the non-`current` task could have already passed through `release_task()`.
> +///
> +/// To retrieve (1) the `current_pid_ns!()` macro should be used which ensure that the returned
> +/// `PidNamespace` cannot outlive the calling scope. The associated `current_pid_ns()` function
> +/// should not be called directly as it could be abused to created an unbounded lifetime for
> +/// `PidNamespace`. The `current_pid_ns!()` macro allows Rust to handle the common case of
> +/// accessing `current`'s `PidNamespace` without RCU protection and without having to acquire a
> +/// reference count.
> +///
> +/// For (2) the `task_get_pid_ns()` method must be used. This will always acquire a reference on
> +/// `PidNamespace` and will return an `Option` to force the caller to explicitly handle the case
> +/// where `PidNamespace` is `None`, something that tends to be forgotten when doing the equivalent
> +/// operation in `C`. Missing RCU primitives make it difficult to perform operations that are
> +/// otherwise safe without holding a reference count as long as RCU protection is guaranteed. But
> +/// it is not important currently. But we do want it in the future.
> +///
> +/// Note for (2) the required RCU protection around calling `task_active_pid_ns()` synchronizes
> +/// against putting the last reference of the associated `struct pid` of `task->thread_pid`.
> +/// The `struct pid` stored in that field is used to retrieve the `PidNamespace` of the caller.
> +/// When `release_task()` is called `task->thread_pid` will be `NULL`ed and `put_pid()` on said
> +/// `struct pid` will be delayed in `free_pid()` via `call_rcu()` allowing everyone with an RCU
> +/// protected access to the `struct pid` acquired from `task->thread_pid` to finish.

Is the comment here in the wrong place? The macro here is just getting
`current` one. Perhaps move it to the `task_get_pid_ns`, and as a
normal comment, since this is impl detail and not something for user to
worry about (yet)?

> +#[macro_export]
> +macro_rules! current_pid_ns {
> +    () => {
> +        // SAFETY: Deref + addr-of below create a temporary `PidNamespaceRef` that cannot outlive
> +        // the caller.
> +        unsafe { &*$crate::task::Task::current_pid_ns() }
> +    };
> +}
> +
>  /// Wraps the kernel's `struct task_struct`.
>  ///
>  /// # Invariants
> @@ -145,6 +205,41 @@ fn deref(&self) -> &Self::Target {
>          }
>      }
>  
> +    /// Returns a PidNamespace reference for the currently executing task's/thread's pid namespace.
> +    ///
> +    /// This function can be used to create an unbounded lifetime by e.g., storing the returned
> +    /// PidNamespace in a global variable which would be a bug. So the recommended way to get the
> +    /// current task's/thread's pid namespace is to use the [`current_pid_ns`] macro because it is
> +    /// safe.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the returned object doesn't outlive the current task/thread.
> +    pub unsafe fn current_pid_ns() -> impl Deref<Target = PidNamespace> {
> +        struct PidNamespaceRef<'a> {
> +            task: &'a PidNamespace,
> +            _not_send: NotThreadSafe,
> +        }
> +
> +        impl Deref for PidNamespaceRef<'_> {
> +            type Target = PidNamespace;
> +
> +            fn deref(&self) -> &Self::Target {
> +                self.task
> +            }
> +        }
> +
> +        let pidns = unsafe { bindings::task_active_pid_ns(Task::current_raw()) };
> +        PidNamespaceRef {
> +            // SAFETY: If the current thread is still running, the current task and its associated
> +            // pid namespace are valid. Given that `PidNamespaceRef` is not `Send`, we know it
> +            // cannot be transferred to another thread (where it could potentially outlive the
> +            // current `Task`).
> +            task: unsafe { &*pidns.cast() },
> +            _not_send: NotThreadSafe,
> +        }
> +    }
> +
>      /// 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
> @@ -182,11 +277,23 @@ pub fn signal_pending(&self) -> bool {
>          unsafe { bindings::signal_pending(self.0.get()) != 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()) }
> +    /// Returns task's pid namespace with elevated reference count
> +    pub fn task_get_pid_ns(&self) -> Option<ARef<PidNamespace>> {

Given that this is within `Task`, the full name of the function became
`Task::task_get_pid_ns`. So this can just be `get_pid_ns`?

> +        let ptr = unsafe { bindings::task_get_pid_ns(self.0.get()) };
> +        if ptr.is_null() {
> +            None
> +        } else {
> +            // SAFETY: `ptr` is valid by the safety requirements of this function. And we own a
> +            // reference count via `task_get_pid_ns()`.
> +            // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::pid_namespace`.
> +            Some(unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr.cast::<PidNamespace>())) })
> +        }
> +    }
> +
> +    /// Returns the given task's pid in the provided pid namespace.
> +    pub fn task_tgid_nr_ns(&self, pidns: &PidNamespace) -> Pid {

Similarly, this can drop `task_` prefix as it's already scoped to
`Task`.

PS. I think I quite like the more descriptive name in Alice's patch,
maybe `Task::tgid_in_ns` could be a good name for this?

If there's concern about documentation searchability, there is a
feature in rustdoc where you can put

	#[doc(alias = "task_tgid_nr_ns")]

and then the function will be searchable with the C name.

> +        // SAFETY: We know that `self.0.get()` is valid by the type invariant.
> +        unsafe { bindings::task_tgid_nr_ns(self.0.get(), pidns.as_ptr()) }
>      }
>  
>      /// Wakes up the task.
> 
> ---
> base-commit: e9980e40804730de33c1563d9ac74d5b51591ec0
> change-id: 20241001-brauner-rust-pid_namespace-52b0c92c8359
> 
> 

Best,
Gary

Powered by blists - more mailing lists