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

[Christian Brauner]

On Thu, Nov 30, 2023 at 12:10:12PM +0000, Alice Ryhl wrote:
> Christian Brauner <brauner@xxxxxxxxxx> 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.