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?
/// All methods defined on `File` that take `&self` are safe to call even if
/// other threads are concurrently accessing the same `struct file`, because
/// those methods either access immutable properties or have proper
/// synchronization to ensure that such accesses are safe.
Note: Here, I say "take &self" to refer to methods with &self in the
signature. This signature means that you pass a &File to the method when
you call it.
Alice
