On Fri, Dec 01, 2023 at 09:06:35AM +0000, Alice Ryhl wrote:
> Benno Lossin <benno.lossin@xxxxxxxxx> 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.
