Re: [PATCH v2] rust: add PidNamespace
From: Gary Guo
Date: Tue Oct 01 2024 - 15:11:19 EST
On Tue, 01 Oct 2024 11:43:42 +0200
Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> +
> +//! 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