Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Mon,  4 Dec 2023 15:42:59 +0000
In-Reply-To: <20231201-zacken-gewachsen-73fe323b067b@brauner>
Mime-Version: 1.0
References: <20231201-zacken-gewachsen-73fe323b067b@brauner>
Message-ID: <20231204154259.79529-1-aliceryhl@google.com>
Subject: Re: [PATCH 2/7] rust: cred: add Rust abstraction for `struct cred`
From:   Alice Ryhl <aliceryhl@google.com>
To:     brauner@kernel.org
Cc:     a.hindborg@samsung.com, alex.gaynor@gmail.com,
        aliceryhl@google.com, arve@android.com, benno.lossin@proton.me,
        bjorn3_gh@protonmail.com, boqun.feng@gmail.com,
        cmllamas@google.com, dan.j.williams@intel.com, dxu@dxuuu.xyz,
        gary@garyguo.net, gregkh@linuxfoundation.org,
        joel@joelfernandes.org, keescook@chromium.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        maco@android.com, ojeda@kernel.org, peterz@infradead.org,
        rust-for-linux@vger.kernel.org, surenb@google.com,
        tglx@linutronix.de, tkjos@android.com, viro@zeniv.linux.org.uk,
        wedsonaf@gmail.com, willy@infradead.org
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

Christian Brauner <brauner@kernel.org> writes:
> On Fri, Dec 01, 2023 at 09:06:35AM +0000, Alice Ryhl wrote:
> > Benno Lossin <benno.lossin@proton.me> writes:
> > > On 11/29/23 13:51, Alice Ryhl wrote:
> > >> +    /// Returns the credentials of the task that originally opened the file.
> > >> +    pub fn cred(&self) -> &Credential {
> > >> +        // This `read_volatile` is intended to correspond to a READ_ONCE call.
> > >> +        //
> > >> +        // SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
> > >> +        //
> > >> +        // TODO: Replace with `read_once` when available on the Rust side.
> > >> +        let ptr = unsafe { core::ptr::addr_of!((*self.0.get()).f_cred).read_volatile() };
> > >> +
> > >> +        // SAFETY: The signature of this function ensures that the caller will only access the
> > >> +        // returned credential while the file is still valid, and the credential must stay valid
> > >> +        // while the file is valid.
> > > 
> > > About the last part of this safety comment, is this a guarantee from the
> > > C side? If yes, then I would phrase it that way:
> > > 
> > >     ... while the file is still valid, and the C side ensures that the
> > >     credentials stay valid while the file is valid.
> > 
> > Yes, that's my intention with this code.
> > 
> > But I guess this is a good question for Christian Brauner to confirm:
> > 
> > If I read the credential from the `f_cred` field, is it guaranteed that
> > the pointer remains valid for at least as long as the file?
> > 
> > Or should I do some dance along the lines of "lock file, increment
> > refcount on credential, unlock file"?
> 
> The lifetime of the f_cred reference is at least as long as the lifetime
> of the file:
> 
> // file not yet visible anywhere
> some_file = alloc_file*()
> -> init_file()
>    {
>            file->f_cred = get_cred(cred /* usually current_cred() */)
>    }
> 
> 
> // install into fd_table -> irreversible, thing visible, possibly shared
> fd_install(1234, some_file)
> 
> // last fput
> fput()
> // atomic_dec_and_test() dance:
> -> file_free() // either "delayed" through task work, workqueue, or
> 	       // sometimes freed right away if file hasn't been opened,
> 	       // i.e., if fd_install() wasn't called
>    -> put_cred(file->f_cred)
> 
> In order to access anything you must hold a reference to the file or
> files->file_lock. IOW, no poking around in f->f_cred or any field for
> that matter just under rcu_read_lock() for example. Because files are
> SLAB_TYPESAFE_BY_RCU. You might be poking in someone else's creds then.

Okay, we aren't dealing with the rcu case in this patchset, so we know
that it wont be freed while we're accessing it.

I guess this means that the `f_cred` field is immutable, which means
that I don't need READ_ONCE to read it? I'll use an ordinary load in the
next version.

Alice