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