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

[Alice Ryhl <aliceryhl@google.com>]

On Wed, Nov 27, 2024 at 4:52 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Nov 27, 2024 at 1:36 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > On Tue, Nov 26, 2024 at 6:15 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Fri, Nov 22, 2024 at 4:41 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > +impl CurrentTask {
> > > > +    /// Access the address space of this task.
> > > > +    ///
> > > > +    /// To increment the refcount of the referenced `mm`, you can use `ARef::from`.
> > > > +    #[inline]
> > > > +    pub fn mm(&self) -> Option<&MmWithUser> {
> > > > +        let mm = unsafe { (*self.as_ptr()).mm };
> > > > +
> > > > +        if mm.is_null() {
> > > > +            None
> > > > +        } else {
> > > > +            // SAFETY: If `current->mm` is non-null, then it references a valid mm with a non-zero
> > > > +            // value of `mm_users`. The returned `&MmWithUser` borrows from `CurrentTask`, so the
> > > > +            // `&MmWithUser` cannot escape the current task, meaning `mm_users` can't reach zero
> > > > +            // while the reference is still live.
> > > > +            Some(unsafe { MmWithUser::from_raw(mm) })
> > >
> > > Maybe also add safety comments for these nitpicky details:
> > >
> > > kthreads can use kthread_use_mm()/kthread_unuse_mm() to change
> > > current->mm (which allows kthreads to access arbitrary userspace
> > > address spaces with copy_from_user/copy_to_user), but as long as you
> > > can't call into kthread_use_mm()/kthread_unuse_mm() from Rust code,
> > > this should be correct. If you do want to allow calls into
> > > kthread_use_mm()/kthread_unuse_mm() later on, you might have to gate
> > > this on a check for PF_KTHREAD, or something like that.
> >
> > Huh ... is it possible to use kthread_use_mm() to create a situation
> > where current->mm has mm_users equal to zero? If not, then I don't
> > think it's a problem.
>
> Ah, no, I don't think so. I think the only problematic scenario would
> be if rust code created a borrow of current->mm, then called
> kthread_unuse_mm() and dropped the reference that was held on the MM,
> and then accessed the borrowed old mm_struct. Which isn't possible
> unless a Rust binding is created for
> kthread_use_mm()/kthread_unuse_mm().

Ah, ok.

The way that the current abstraction works is that it enforces that
the current pointer cannot escape the scope you were in when you
obtained it. If we enforce that kthread_use_mm() and
kthread_unuse_mm() involve a scope, then that should solve that.

Alice