linux-kernel - Re: [PATCH 1/7] rust: file: add Rust abstraction for `struct file`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231130-zweig-mitleid-2ba3ef78145e@brauner>
Date:   Thu, 30 Nov 2023 13:36:06 +0100
From:   Christian Brauner <brauner@...nel.org>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     a.hindborg@...sung.com, alex.gaynor@...il.com, arve@...roid.com,
        benno.lossin@...ton.me, bjorn3_gh@...tonmail.com,
        boqun.feng@...il.com, cmllamas@...gle.com,
        dan.j.williams@...el.com, dxu@...uu.xyz, gary@...yguo.net,
        gregkh@...uxfoundation.org, joel@...lfernandes.org,
        keescook@...omium.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, maco@...roid.com, ojeda@...nel.org,
        peterz@...radead.org, rust-for-linux@...r.kernel.org,
        surenb@...gle.com, tglx@...utronix.de, tkjos@...roid.com,
        viro@...iv.linux.org.uk, wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH 1/7] rust: file: add Rust abstraction for `struct file`

On Thu, Nov 30, 2023 at 12:10:12PM +0000, Alice Ryhl wrote:
> Christian Brauner <brauner@...nel.org> writes:
> >> This is the backdoor. You use it when *you* know that the file is okay
> > 
> > And a huge one.
> > 
> >> to access, but Rust doesn't. It's unsafe because it's not checked by
> >> Rust.
> >> 
> >> For example you could do this:
> >> 
> >> 	let ptr = unsafe { bindings::fdget(fd) };
> >> 
> >> 	// SAFETY: We just called `fdget`.
> >> 	let file = unsafe { File::from_ptr(ptr) };
> >> 	use_file(file);
> >> 
> >> 	// SAFETY: We're not using `file` after this call.
> >> 	unsafe { bindings::fdput(ptr) };
> >> 
> >> It's used in Binder here:
> >> https://github.com/Darksonn/linux/blob/dca45e6c7848e024709b165a306cdbe88e5b086a/drivers/android/rust_binder.rs#L331-L332
> >> 
> >> Basically, I use it to say "C code has called fdget for us so it's okay
> >> to access the file", whenever userspace uses a syscall to call into the
> >> driver.
> > 
> > Yeah, ok, because the fd you're operating on may be coming from fdget(). Iirc,
> > binder is almost by default used multi-threaded with a shared file descriptor
> > table? But while that means fdget() will usually bump the reference count you
> > can't be sure. Hmkay.
> 
> Even if the syscall used `fget` instead of `fdget`, I would still be
> using `from_ptr` here. The `ARef` type only really makes sense when *we*
> have ownership of the ref-count, but in this case we don't own it. We're
> just given a promise that the caller is keeping it alive for us using
> some mechanism or another.
> 
> >>>> +// SAFETY: It's OK to access `File` through shared references from other threads because we're
> >>>> +// either accessing properties that don't change or that are properly synchronised by C code.
> >>> 
> >>> Uhm, what guarantees are you talking about specifically, please?
> >>> Examples would help.
> >>> 
> >>>> +unsafe impl Sync for File {}
> >> 
> >> The Sync trait defines whether a value may be accessed from several
> >> threads in parallel (using shared/immutable references). In our case,
> > 
> > So let me put this into my own words and you correct me, please:
> > 
> > So, this really just means that if I have two processes both with their own
> > fdtable and they happen to hold fds that refer to the same @file:
> > 
> > P1				P2
> > struct fd fd1 = fdget(1234);
> >                                  struct fd fd2 = fdget(5678);
> > if (!fd1.file)                   if (!fd2.file)
> > 	return -EBADF;                 return -EBADF;
> > 
> > // fd1.file == fd2.file
> > 
> > the only if the Sync trait is implemented both P1 and P2 can in parallel call
> > file->f_op->poll(@file)?
> > 
> > So if the Sync trait isn't implemented then the compiler will prohibit that P1
> > and P2 at the same time call file->f_op->poll(@file)? And that's all that's
> > meant by a shared reference? It's really about sharing the pointer.
> 
> Yeah, what you're saying sounds correct. For a type that is not Sync,
> you would need a lock around the call to `poll` before the compiler
> would accept the call.
> 
> (Or some other mechanism to convince the compiler that no other thread
> is looking at the file at the same time. Of course, a lock is just one
> way to do that.)
> 
> > The thing is that "shared reference" gets a bit in our way here:
> > 
> > (1) If you have SCM_RIGHTs in the mix then P1 can open fd1 to @file and then
> >     send that @file to P2 which now has fd2 refering to @file as well. The
> >     @file->f_count is bumped in that process. So @file->f_count is now 2.
> > 
> >     Now both P1 and P2 call fdget(). Since they don't have a shared fdtable
> >     none of them take an additional reference to @file. IOW, @file->f_count
> >     may remain 2 all throughout the @file->f_op->*() operation.
> > 
> >     So they share a reference to that file and elide both the
> >     atomic_inc_not_zero() and the atomic_dec_not_zero().
> > 
> > (2) io_uring has fixed files whose reference count always stays at 1.
> >     So all io_uring operations on such fixed files share a single reference.
> > 
> > So that's why this is a bit confusing at first to read "shared reference".
> > 
> > Please add a comment on top of unsafe impl Sync for File {}
> > explaining/clarifying this a little that it's about calling methods on the same
> > file.
> 
> Yeah, I agree, the terminology gets a bit mixed up here because we both
> use the word "reference" for different things.

> 
> How about this comment?

Sounds good.