Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Fri, 1 Dec 2023 11:27:33 +0100
From:   Christian Brauner <brauner@kernel.org>
To:     Alice Ryhl <aliceryhl@google.com>
Cc:     benno.lossin@proton.me, a.hindborg@samsung.com,
        alex.gaynor@gmail.com, arve@android.com, 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
Subject: Re: [PATCH 2/7] rust: cred: add Rust abstraction for `struct cred`
Message-ID: <20231201-zacken-gewachsen-73fe323b067b@brauner>
References: <W6StBLpVsvvGchAT5ZEvH9JJyzu401dMqR3yN73NZPjPeZRoaKAuoYe40QWErmPwrnJVTH7BbLKtWXDOMYny5xjwd3CSLyz5IYYReB6-450=@proton.me>
 <20231201090636.2179663-1-aliceryhl@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231201090636.2179663-1-aliceryhl@google.com>
Precedence: bulk

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.