"Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:
> On Thu, Jan 9, 2025 at 9:42 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>>
>> "Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:
>>
>> > On Mon, Dec 16, 2024 at 3:51 PM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>> >>
>> >> "Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:
>> >> > + #[inline]
>> >> > + pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
>> >> > + // SAFETY: It is safe to call `task_active_pid_ns` without RCU protection when calling it
>> >> > + // on the current task.
>> >> > + let active_ns = unsafe { bindings::task_active_pid_ns(self.as_ptr()) };
>> >> > +
>> >> > + if active_ns.is_null() {
>> >> > + return None;
>> >> > + }
>> >> > +
>> >> > + // 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 `&CurrentTask` type should be used which ensures that the returned
>> >> > + // `PidNamespace` cannot outlive the current task context. The `CurrentTask::active_pid_ns`
>> >> > + // function 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.
>> >>
>> >> While this comment is a nice piece of documentation, I think we should
>> >> move it elsewhere, or restrict it to paragraphs pertaining to (1), since
>> >> that is the only case we consider here?
>> >
>> > Where would you move it?
>>
>> The info about (2) should probably be with the implementation for that
>> case, when it lands. Perhaps we can move it hen?
>
> The function already exists. It's called Task::get_pid_ns(). I think
> the comment makes sense here: get_pid_ns() is the normal case where
> you don't skip synchronization, and active_pid_ns() is the special
> case where you can skip RCU due to reasons. This comment explains that
> normally you cannot skip RCU, but in this special case you can.
Reading this again I think it should simply be cut down in size:
```
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.
