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.
