Re: [PATCH v11 8/8] task: rust: rework how current is accessed

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Jan 13, 2025 at 11:30:05AM +0100, Alice Ryhl wrote:
> On Tue, Dec 17, 2024 at 12:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Wed, Dec 11, 2024 at 10:37:12AM +0000, Alice Ryhl wrote:
> > > Introduce a new type called `CurrentTask` that lets you perform various
> > > operations that are only safe on the `current` task. Use the new type to
> > > provide a way to access the current mm without incrementing its
> > > refcount.
> > >
> > > With this change, you can write stuff such as
> > >
> > >       let vma = current!().mm().lock_vma_under_rcu(addr);
> > >
> > > without incrementing any refcounts.
> > >
> > > This replaces the existing abstractions for accessing the current pid
> > > namespace. With the old approach, every field access to current involves
> > > both a macro and a unsafe helper function. The new approach simplifies
> > > that to a single safe function on the `CurrentTask` type. This makes it
> > > less heavy-weight to add additional current accessors in the future.
> > >
> > > That said, creating a `CurrentTask` type like the one in this patch
> > > requires that we are careful to ensure that it cannot escape the current
> > > task or otherwise access things after they are freed. To do this, I
> > > declared that it cannot escape the current "task context" where I
> > > defined a "task context" as essentially the region in which `current`
> > > remains unchanged. So e.g., release_task() or begin_new_exec() would
> > > leave the task context.
> > >
> > > If a userspace thread returns to userspace and later makes another
> > > syscall, then I consider the two syscalls to be different task contexts.
> > > This allows values stored in that task to be modified between syscalls,
> > > even if they're guaranteed to be immutable during a syscall.
> > >
> > > Ensuring correctness of `CurrentTask` is slightly tricky if we also want
> > > the ability to have a safe `kthread_use_mm()` implementation in Rust. To
> > > support that safely, there are two patterns we need to ensure are safe:
> > >
> > >       // Case 1: current!() called inside the scope.
> > >       let mm;
> > >       kthread_use_mm(some_mm, || {
> > >           mm = current!().mm();
> > >       });
> > >       drop(some_mm);
> > >       mm.do_something(); // UAF
> > >
> > > and:
> > >
> > >       // Case 2: current!() called before the scope.
> > >       let mm;
> > >       let task = current!();
> > >       kthread_use_mm(some_mm, || {
> > >           mm = task.mm();
> > >       });
> > >       drop(some_mm);
> > >       mm.do_something(); // UAF
> > >
> > > The existing `current!()` abstraction already natively prevents the
> > > first case: The `&CurrentTask` would be tied to the inner scope, so the
> > > borrow-checker ensures that no reference derived from it can escape the
> > > scope.
> > >
> > > Fixing the second case is a bit more tricky. The solution is to
> > > essentially pretend that the contents of the scope execute on an
> > > different thread, which means that only thread-safe types can cross the
> > > boundary. Since `CurrentTask` is marked `NotThreadSafe`, attempts to
> > > move it to another thread will fail, and this includes our fake pretend
> > > thread boundary.
> > >
> > > This has the disadvantage that other types that aren't thread-safe for
> > > reasons unrelated to `current` also cannot be moved across the
> > > `kthread_use_mm()` boundary. I consider this an acceptable tradeoff.
> > >
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/mm.rs   |  22 ----
> > >  rust/kernel/task.rs | 284 ++++++++++++++++++++++++++++++----------------------
> > >  2 files changed, 167 insertions(+), 139 deletions(-)
> > >
> > > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > > index 50f4861ae4b9..f7d1079391ef 100644
> > > --- a/rust/kernel/mm.rs
> > > +++ b/rust/kernel/mm.rs
> > > @@ -142,28 +142,6 @@ fn deref(&self) -> &MmWithUser {
> > >
> > >  // These methods are safe to call even if `mm_users` is zero.
> > >  impl Mm {
> > > -    /// Call `mmgrab` on `current.mm`.
> > > -    #[inline]
> > > -    pub fn mmgrab_current() -> Option<ARef<Mm>> {
> > > -        // SAFETY: It's safe to get the `mm` field from current.
> > > -        let mm = unsafe {
> > > -            let current = bindings::get_current();
> > > -            (*current).mm
> > > -        };
> > > -
> > > -        if mm.is_null() {
> > > -            return None;
> > > -        }
> > > -
> > > -        // SAFETY: The value of `current->mm` is guaranteed to be null or a valid `mm_struct`. We
> > > -        // just checked that it's not null. Furthermore, the returned `&Mm` is valid only for the
> > > -        // duration of this function, and `current->mm` will stay valid for that long.
> > > -        let mm = unsafe { Mm::from_raw(mm) };
> > > -
> > > -        // This increments the refcount using `mmgrab`.
> > > -        Some(ARef::from(mm))
> > > -    }
> > > -
> >
> > This is removed because of no user? If so, maybe don't introduce this at
> > all in the earlier patch of this series? The rest looks good to me.
> 
> I guess I can drop the temporary introduction of this. It's here due
> to the history of this series where originally it only had
> mmgrab_current, and Binder would use that. But with this patch, you

As someone who usually dig a lot of history in git, I would like to see
the drop of the temporary introduction ;-) If you're going to rebase,
please see whether the drop can be done easily, thanks! Not a block
issue though.

With or without the change, feel free to add:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> can use CurrentTask::mm() + ARef::from() to do the same thing. For
> Binder, the difference doesn't matter, but the latter is more powerful
> as you can access the current task's mm_struct without incrementing
> refcounts.
> 
> Alice