binder: Add support for transferring file descriptors. #371

wedsonaf · 2021-06-09T10:36:13Z

Signed-off-by: Wedson Almeida Filho [email protected]

ojeda

LGTM.

drivers/android/transaction.rs

TheSven73

I wish I understood Binder, so my review would be more useful than it is now. But I guess something is better than nothing, right?

TheSven73 · 2021-06-09T12:56:08Z

drivers/android/allocation.rs

@@ -19,6 +22,7 @@ pub(crate) struct Allocation<'a> {
    pub(crate) process: &'a Process,
    allocation_info: Option<AllocationInfo>,
    free_on_drop: bool,
+    pub(crate) file_list: List<Box<FileInfo>>,


Nit: when looking at the binder code, I often wonder why struct members are so often declared as pub(crate)? Would it be beneficial to replace with a method function that does what you plan to do with the struct member, without having to expose internal implementation details?

Most of these modules started off in another module with access to all fields. Eventually I pushed some of these out to their own modules and made most fields private; for the ones where this wasn't trivial I just made them pub(crate) because I had (and still do) bigger changes to worry about. I didn't want to have pure getters/setters as in Java.

For this particular change, I already have take_file_list (added later), so I may as well add a file_list push and make file_list private. Will do.

Got ya. It's a WIP.

TheSven73 · 2021-06-09T13:02:25Z

drivers/android/transaction.rs

 pub(crate) struct Transaction {
+    inner: SpinLock<TransactionInner>,


Do you have a real need for a SpinLock here, as opposed to a Mutex? Are you locking this from a non-sleep context?
(Not suggesting it should be a Mutex - I just genuinely don't know)

I need some synchronisation. Given that the hold times are small (just to swap a pointer), I chose a spinlock because it will just spin on contention as opposed to sleep. This is preferable because schedule() itself would take longer than the hold time of the lock, that is, sleeping is counterproductive on contention.

just to swap a pointer

Would AtomicPtr work?

just to swap a pointer

Would AtomicPtr work?

I considered it, but decided against it for two reasons: it doesn't work for pointers to ?Sized types, and I may want to add more fields to it inner eventually.

Makes sense. Could be one of the parameters to be verified using the ping benchmark somewhere in the future?

Yes, absolutely.

One "advantage" we have with binder is that we have the C implementation to benchmark against. As long as we're not too far off, we're good.

TheSven73 · 2021-06-09T13:08:15Z

drivers/android/transaction.rs

+        let mut_tr = Arc::get_mut(&mut tr).ok_or(Error::EINVAL)?;
+
+        // SAFETY: `inner` is pinned behind `Arc`.
+        kernel::spinlock_init!(Pin::new_unchecked(&mut_tr.inner), "Transaction::inner");


I'm still trying to get my head around Pinning. Is there a guarantee that Self will never move out of the Arc?
I'm guessing you either need an explicit guarantee (in the form of an # Invariant, might be hard to mentally enforce here) or return Pin<Arc<Self>> ?

It's not guaranteed. At the moment it's an invariant of the project: I don't move objects behind Arcs.

Some background: in the beginning I had a bunch of TODO: This needs to be pinned in a lot of places, and a bunch of Pins that resulted in a bunch of unsafe blocks (it's always unsafe to get a mutable reference to a pinned type because the compiler needs us to manually ensure that we don't move anything). I removed them until we get a better handle on how this will look like.

TheSven73 · 2021-06-09T13:30:44Z

drivers/android/transaction.rs

+
+        // When `drop` is called, we don't want the allocation to be freed because it is now the
+        // user's reponsibility to free it.
+        self.free_allocation.store(false, Ordering::Relaxed);


If using Ordering::Relaxed, is the following scenario possible?

a thread calls do_work() and free_allocation is set to false

another thread calls Drop, doesn't "see" that free_allocation is false yet, frees the allocation

user frees the allocation again?

It's not possible because Arc prevents that. In particular, all ref count decrements have release semantics, which are paired with an acquire right before drop. See https://doc.rust-lang.org/src/alloc/sync.rs.html#1540.

That definitely sounds legit :) It may be hard to know for most people looking at the code? Maybe document this in the code at some point in the future? Idk, I'm sure you've got more important things on your plate in Binder.

Fair point. I'll add a comment that describes this.

This got me thinking. Currently in kernel C, there are lots of variables that convey information from one thread to another - like free_allocation here. But many of them don't need to be protected by a synchronization primitive. That's because many kernel calls issue explicit fences. E.g. when you're waiting on a flag with wait_event_interruptible(), that function is guaranteed to have issued a fence when it returns, so the int or bool flag you're waiting for can never be out of date.

But in Rust, we can't make that optimization? We always need the non-zero cost synchronization primitive, even if we can logically deduce things will work across threads?

Anyways, this is too optimize-ey to worry about at this point IMHO.

load/store with Ordering::Relaxed doesn't have overhead though. There is a nice article about what's actually generated: https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. load/store with Ordering::Relaxed are basically just normal load/store with additional tear-free guarantee (even though normal load/store are de facto tear-free).

Interesting!! Is Relaxed equivalent to a normal access in a structure?

Interesting!! Is Relaxed equivalent to a normal access in a structure?

If you only access it once :)

If you access an atomic integer multiple times with Relaxed ordering the compiler will need to generate multiple load instructions (since its value may change), while for a non-atomic integer Rust compiler is free to assume that value wouldn't change (even if there is a function-call in between two accesses).

That's extremely interesting, thank you Gary !

load/store with Ordering::Relaxed doesn't have overhead though.

While this is generally true, it isn't always true. Even on architectures that offer non-tearing word-sized ops, if the type is bigger than the word size, we may need more expensive instructions. For example, on 32-bit x86, if you try to atomically read/write a 64-bit integer, it won't be a regular load/store.

But to Sven's original point, I tend to agree that Rust imposes restrictions that may make the code less efficient than C. We need to look into those carefully as I think a lot of them can be exposed safely in Rust, however, some of them are inherently unsafe (i.e., they rely on the developer following rules that cannot be enforced by the compiler) -- for these I think we'll have to do as discussed in another bug/issue, provide a combination of: safe alternative with the extra cost as the price for safety, and/or an unsafe optimal alternative. If we offer both, we let the developer choose according to their needs.

drivers/android/allocation.rs

bjorn3 · 2021-06-09T13:54:39Z

drivers/android/thread.rs

+
+                let obj = view.read::<bindings::binder_fd_object>(offset)?;
+                // SAFETY: The type is `BINDER_TYPE_FD`, so `fd` is populated.
+                let fd = unsafe { obj.__bindgen_anon_1.fd };


To nitpick: While this is indeed safe, I don't think the reason is correct. The program using binder doesn't have to populate fd. It is not UB to read it however, as from the perspective of the kernel all userspace data is defined.

Hmm... I am confused here.

From a quick look at the reference, it says Rust unions do not have an active member (unlike C++), i.e. bits are always re-interpreted. It also says reading uninitialized memory from unions is OK, so that is not a concern either. Thus, for u32 etc. it would seem it is always safe to read from it since there are no invalid values possible.

So two questions:

Why does Rust not simply mark the operation as safe? Is it because they did not want to make the operation conditionally-safe depending on the type?

If it is always safe, what do you mean when you say the reason is "as from the perspective of the kernel all userspace data is defined"?

Why does Rust not simply mark the operation as safe? Is it because they did not want to make the operation conditionally-safe depending on the type?

Imagine the following union

union TransmuteToBox { num: usize, the_box: Box<u8>, }

If reading an union variant was safe TransmuteToBox { num: 0 }.the_box would give a dangling box, which is UB.

What the reference is saying is that if the byte representation of the union is valid for an union variant, then reading it as such union variant is safe. This is unlike C where writing as one variant and then reading it as another variant is UB even if both variants have the same set of allowed byte representations. For example the following would be UB in C:

union TransmuteU32ToFourU8 { int: u32, array: [u8; 4], } TransmuteU32ToFourU8 { int: 0 }.array

despite the fact that all 0s is a valid byte representation for [u8; 4].

If it is always safe, what do you mean when you say the reason is "as from the perspective of the kernel all userspace data is defined"?

Even if the data is undefined inside the program that called the ioctl, the compiler of the kernel has no way to exploit this fact as it doesn't know.

I was under the impression that Rust had an active member. But given that isn't the case, it seems like the justifications should just be about the representation of the field being used. From the reference:

It is the programmer's responsibility to make sure that the data is valid at the field's type. Failing to do so results in undefined behavior. For example, reading the value 3 through of a field of the boolean type is undefined behavior.

Would something like this be reasonable to you all?

// SAFETY: `fd` is a `u32`; any bit pattern is a valid representation.

copy_from_userspace is written in assembly AFAICT, which is opaque, so a C or Rust compiler has to assume that it initializes the full copy target. If the compiler was able to see that the userspace leaves it unitialized, that would make it impossible to write a kernel that can't be made to invoke UB by a malicious program.

I think in this case it's shared memory rather than copying from userspace, but still there is no possible way to let the "uninitialized" property across process or kernel/userspace boundary.

If this is shared writable memory, you will have to use atomics, as otherwise you could have a data race, which is UB. The compiler could reload the value and expect it to still be the same if atomics are not used.

view.read already copies it :) it's just that this is copied directly in kernel space rather than from userspace.

The memory is indeed shared with userspace, but it (userspace) can only read from it. It is populated by copying from another userspace process to the kernel mapping (which is writable); while it is being copied, the receiving end (the one that has it mapped readonly) treats it as unallocated memory, so they don't touch it.

bjorn3 · 2021-06-09T13:56:41Z

drivers/android/transaction.rs

+    fn prepare_file_list(&self) -> Result<List<Box<FileInfo>>> {
+        // Get list of files that are being transfered as part of the transaction.
+        let mut file_list = List::new();
+        core::mem::swap(&mut file_list, &mut self.inner.lock().file_list);


You can use mem::replace here too. Something like let mut file_list = core::mem::replace(&mut ..., List::new());.

Will change this too.

Signed-off-by: Wedson Almeida Filho <[email protected]>

ksquirrel · 2021-06-09T15:49:45Z

Review of 42230f159d07:

✔️ Commit 42230f1: Looks fine!

wedsonaf · 2021-06-09T16:01:39Z

I wish I understood Binder, so my review would be more useful than it is now. But I guess something is better than nothing, right?

Indeed, thanks for reviewing! And please keep at it :)

This comment has been minimized.

Sign in to view

ojeda reviewed Jun 9, 2021

View reviewed changes

drivers/android/transaction.rs Outdated Show resolved Hide resolved

TheSven73 reviewed Jun 9, 2021

View reviewed changes

bjorn3 reviewed Jun 9, 2021

View reviewed changes

drivers/android/allocation.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 9, 2021

View reviewed changes

binder: Add support for transferring file descriptors.

42230f1

Signed-off-by: Wedson Almeida Filho <[email protected]>

wedsonaf force-pushed the file-descriptor branch from 29fba8a to 42230f1 Compare June 9, 2021 15:49

TheSven73 approved these changes Jun 10, 2021

View reviewed changes

wedsonaf merged commit d8c6b9c into Rust-for-Linux:rust Jun 11, 2021

wedsonaf deleted the file-descriptor branch June 11, 2021 00:02

		pub(crate) struct Transaction {
		inner: SpinLock<TransactionInner>,

binder: Add support for transferring file descriptors. #371

binder: Add support for transferring file descriptors. #371

Uh oh!

Conversation

wedsonaf commented Jun 9, 2021

Uh oh!

This comment has been minimized.

ojeda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheSven73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojeda Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ojeda Jun 9, 2021 •

edited

Loading

bjorn3 Jun 9, 2021 •

edited

Loading