Date:   Mon, 11 Dec 2023 17:04:52 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Kent Overstreet <kent.overstreet@...ux.dev>
Cc:     Boqun Feng <boqun.feng@...il.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <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>,
        Kees Cook <keescook@...omium.org>,
        Matthew Wilcox <willy@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Daniel Xu <dxu@...uu.xyz>, linux-kernel@...r.kernel.org,
        rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 5/7] rust: file: add `Kuid` wrapper

On 12/11/23 16:58, Kent Overstreet wrote:
> On Fri, Dec 08, 2023 at 08:43:19AM -0800, Boqun Feng wrote:
>> On Fri, Dec 08, 2023 at 04:40:09PM +0000, Benno Lossin wrote:
>>> On 12/6/23 12:59, Alice Ryhl wrote:
>>>> +    /// Returns the given task's pid in the current pid namespace.
>>>> +    pub fn pid_in_current_ns(&self) -> Pid {
>>>> +        // SAFETY: Calling `task_active_pid_ns` with the current task is always safe.
>>>> +        let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) };
>>>
>>> Why not create a safe wrapper for `bindings::get_current()`?
>>> This patch series has three occurrences of `get_current`, so I think it
>>> should be ok to add a wrapper.
>>> I would also prefer to move the call to `bindings::get_current()` out of
>>> the `unsafe` block.
>>
>> FWIW, we have a current!() macro, we should use it here.
> 
> Why does it need to be a macro?

This is a very interesting question. A `Task` is `AlwaysRefCounted`, so
if you have a `&'a Task`, someone above you owns a refcount on that
task. But the `current` task will never go away as long as you stay in
the same task. So you actually do not need to own a refcount as long as
there are no context switches.

We use this to our advantage and the `current!()` macro returns
something that acts like `&'a Task` but additionally is `!Send` (so it
cannot be sent over to a different task). This means that we do not need
to take a refcount on the current task to get a reference to it.

But just having a function that returns a `&'a Task` like thing that is
`!Send` is not enough, since there are no constraints on 'a. This is
because the function `Task::current` would take no arguments and there
is nothing the lifetime could even bind to.
Since there are no constraints, you could just choose 'a = 'static which
is obviously wrong, since there are tasks that end. For this reason the
`Task::current` function is `unsafe` and the macro `current!` ensures
that the lifetime 'a ends early enough. It is implemented like this:

    macro_rules! current {
        () => {
            // SAFETY: Deref + addr-of below create a temporary `TaskRef` that cannot outlive the
            // caller.
            unsafe { &*$crate::task::Task::current() }
        };
    }

Note the `&*`. This ensures that the thing returned by `Task::current`
is temporary and cannot outlive the current function thus preventing the
creation of static references to it.

If you want to read more, see here for Gary's original discovery of the
issue:
https://lore.kernel.org/rust-for-linux/20230331034701.0657d5f2.gary@garyguo.net/

-- 
Cheers,
Benno

Powered by blists - more mailing lists