Message-ID: <20241002-backt-inkognito-e25cd4baa03f@brauner>
Date: Wed, 2 Oct 2024 13:05:41 +0200
From: Christian Brauner <brauner@...nel.org>
To: Gary Guo <gary@...yguo.net>
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, Oct 01, 2024 at 08:10:54PM GMT, Gary Guo wrote:
> 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)?

Sure.

> 
> > +#[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`?

Fair.

> 
> > +        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?

I'm not found of the "in" part. tgid_nr_ns() is fine with me.

> 
> 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.

Sounds good.

Powered by blists - more mailing lists