-
Notifications
You must be signed in to change notification settings - Fork 456
binder: Add support for transferring file descriptors. #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,20 @@ | ||
// SPDX-License-Identifier: GPL-2.0 | ||
|
||
use alloc::sync::Arc; | ||
use core::sync::atomic::{AtomicBool, Ordering}; | ||
use alloc::{boxed::Box, sync::Arc}; | ||
use core::{ | ||
pin::Pin, | ||
sync::atomic::{AtomicBool, Ordering}, | ||
}; | ||
use kernel::{ | ||
io_buffer::IoBufferWriter, linked_list::Links, prelude::*, sync::Ref, | ||
user_ptr::UserSlicePtrWriter, ScopeGuard, | ||
bindings, | ||
file::{File, FileDescriptorReservation}, | ||
io_buffer::IoBufferWriter, | ||
linked_list::List, | ||
linked_list::{GetLinks, Links}, | ||
prelude::*, | ||
sync::{Ref, SpinLock}, | ||
user_ptr::UserSlicePtrWriter, | ||
Error, ScopeGuard, | ||
}; | ||
|
||
use crate::{ | ||
|
@@ -16,7 +26,12 @@ use crate::{ | |
DeliverToRead, Either, | ||
}; | ||
|
||
struct TransactionInner { | ||
file_list: List<Box<FileInfo>>, | ||
} | ||
|
||
pub(crate) struct Transaction { | ||
inner: SpinLock<TransactionInner>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a real need for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I considered it, but decided against it for two reasons: it doesn't work for pointers to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Could be one of the parameters to be verified using the ping benchmark somewhere in the future? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// TODO: Node should be released when the buffer is released. | ||
node_ref: Option<NodeRef>, | ||
stack_next: Option<Arc<Transaction>>, | ||
|
@@ -37,13 +52,16 @@ impl Transaction { | |
stack_next: Option<Arc<Transaction>>, | ||
from: &Arc<Thread>, | ||
tr: &BinderTransactionData, | ||
) -> BinderResult<Self> { | ||
) -> BinderResult<Arc<Self>> { | ||
let allow_fds = node_ref.node.flags & FLAT_BINDER_FLAG_ACCEPTS_FDS != 0; | ||
let to = node_ref.node.owner.clone(); | ||
let alloc = from.copy_transaction_data(&to, tr, allow_fds)?; | ||
let mut alloc = from.copy_transaction_data(&to, tr, allow_fds)?; | ||
let data_address = alloc.ptr; | ||
let file_list = alloc.take_file_list(); | ||
alloc.keep_alive(); | ||
Ok(Self { | ||
let mut tr = Arc::try_new(Self { | ||
// SAFETY: `spinlock_init` is called below. | ||
inner: unsafe { SpinLock::new(TransactionInner { file_list }) }, | ||
node_ref: Some(node_ref), | ||
stack_next, | ||
from: from.clone(), | ||
|
@@ -55,19 +73,28 @@ impl Transaction { | |
offsets_size: tr.offsets_size as _, | ||
links: Links::new(), | ||
free_allocation: AtomicBool::new(true), | ||
}) | ||
})?; | ||
|
||
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"); | ||
Ok(tr) | ||
} | ||
|
||
pub(crate) fn new_reply( | ||
from: &Arc<Thread>, | ||
to: Ref<Process>, | ||
tr: &BinderTransactionData, | ||
allow_fds: bool, | ||
) -> BinderResult<Self> { | ||
let alloc = from.copy_transaction_data(&to, tr, allow_fds)?; | ||
) -> BinderResult<Arc<Self>> { | ||
let mut alloc = from.copy_transaction_data(&to, tr, allow_fds)?; | ||
let data_address = alloc.ptr; | ||
let file_list = alloc.take_file_list(); | ||
alloc.keep_alive(); | ||
Ok(Self { | ||
let mut tr = Arc::try_new(Self { | ||
// SAFETY: `spinlock_init` is called below. | ||
inner: unsafe { SpinLock::new(TransactionInner { file_list }) }, | ||
node_ref: None, | ||
stack_next: None, | ||
from: from.clone(), | ||
|
@@ -79,7 +106,13 @@ impl Transaction { | |
offsets_size: tr.offsets_size as _, | ||
links: Links::new(), | ||
free_allocation: AtomicBool::new(true), | ||
}) | ||
})?; | ||
|
||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still trying to get my head around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not guaranteed. At the moment it's an invariant of the project: I don't move objects behind Some background: in the beginning I had a bunch of |
||
Ok(tr) | ||
} | ||
|
||
/// Determines if the transaction is stacked on top of the given transaction. | ||
|
@@ -136,6 +169,33 @@ impl Transaction { | |
process.push_work(self) | ||
} | ||
} | ||
|
||
/// Prepares the file list for delivery to the caller. | ||
fn prepare_file_list(&self) -> Result<List<Box<FileInfo>>> { | ||
// Get list of files that are being transferred as part of the transaction. | ||
let mut file_list = core::mem::replace(&mut self.inner.lock().file_list, List::new()); | ||
|
||
// If the list is non-empty, prepare the buffer. | ||
if !file_list.is_empty() { | ||
let alloc = self.to.buffer_get(self.data_address).ok_or(Error::ESRCH)?; | ||
let cleanup = ScopeGuard::new(|| { | ||
self.free_allocation.store(false, Ordering::Relaxed); | ||
}); | ||
|
||
let mut it = file_list.cursor_front_mut(); | ||
while let Some(file_info) = it.current() { | ||
let reservation = FileDescriptorReservation::new(bindings::O_CLOEXEC)?; | ||
alloc.write(file_info.buffer_offset, &reservation.reserved_fd())?; | ||
file_info.reservation = Some(reservation); | ||
it.move_next(); | ||
} | ||
|
||
alloc.keep_alive(); | ||
cleanup.dismiss(); | ||
} | ||
|
||
Ok(file_list) | ||
} | ||
} | ||
|
||
impl DeliverToRead for Transaction { | ||
|
@@ -145,9 +205,19 @@ impl DeliverToRead for Transaction { | |
pub sender_euid: uid_t, | ||
*/ | ||
let send_failed_reply = ScopeGuard::new(|| { | ||
let reply = Either::Right(BR_FAILED_REPLY); | ||
self.from.deliver_reply(reply, &self); | ||
if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { | ||
let reply = Either::Right(BR_FAILED_REPLY); | ||
self.from.deliver_reply(reply, &self); | ||
} | ||
}); | ||
let mut file_list = if let Ok(list) = self.prepare_file_list() { | ||
list | ||
} else { | ||
// On failure to process the list, we send a reply back to the sender and ignore the | ||
// transaction on the recipient. | ||
return Ok(true); | ||
}; | ||
|
||
let mut tr = BinderTransactionData::default(); | ||
|
||
if let Some(nref) = &self.node_ref { | ||
|
@@ -165,10 +235,6 @@ impl DeliverToRead for Transaction { | |
tr.data.ptr.offsets = (self.data_address + ptr_align(self.data_size)) as _; | ||
} | ||
|
||
// 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); | ||
|
||
let code = if self.node_ref.is_none() { | ||
BR_REPLY | ||
} else { | ||
|
@@ -183,6 +249,27 @@ impl DeliverToRead for Transaction { | |
// here on out. | ||
send_failed_reply.dismiss(); | ||
|
||
// Commit all files. | ||
{ | ||
let mut it = file_list.cursor_front_mut(); | ||
while let Some(file_info) = it.current() { | ||
if let Some(reservation) = file_info.reservation.take() { | ||
if let Some(file) = file_info.file.take() { | ||
reservation.commit(file); | ||
} | ||
} | ||
|
||
it.move_next(); | ||
} | ||
} | ||
|
||
// When `drop` is called, we don't want the allocation to be freed because it is now the | ||
// user's reponsibility to free it. | ||
// | ||
// `drop` is guaranteed to see this relaxed store because `Arc` guarantess that everything | ||
// that happens when an object is referenced happens-before the eventual `drop`. | ||
self.free_allocation.store(false, Ordering::Relaxed); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not possible because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. I'll add a comment that describes this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This got me thinking. Currently in kernel C, there are lots of variables that convey information from one thread to another - like 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting!! Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you only access it once :) If you access an atomic integer multiple times with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's extremely interesting, thank you Gary ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
// When this is not a reply and not an async transaction, update `current_transaction`. If | ||
// it's a reply, `current_transaction` has already been updated appropriately. | ||
if self.node_ref.is_some() && tr.flags & TF_ONE_WAY == 0 { | ||
|
@@ -209,3 +296,35 @@ impl Drop for Transaction { | |
} | ||
} | ||
} | ||
|
||
pub(crate) struct FileInfo { | ||
links: Links<FileInfo>, | ||
|
||
/// The file for which a descriptor will be created in the recipient process. | ||
file: Option<File>, | ||
|
||
/// The file descriptor reservation on the recipient process. | ||
reservation: Option<FileDescriptorReservation>, | ||
|
||
/// The offset in the buffer where the file descriptor is stored. | ||
buffer_offset: usize, | ||
} | ||
|
||
impl FileInfo { | ||
pub(crate) fn new(file: File, buffer_offset: usize) -> Self { | ||
Self { | ||
file: Some(file), | ||
reservation: None, | ||
buffer_offset, | ||
links: Links::new(), | ||
} | ||
} | ||
} | ||
|
||
impl GetLinks for FileInfo { | ||
type EntryType = Self; | ||
|
||
fn get_links(data: &Self::EntryType) -> &Links<Self::EntryType> { | ||
&data.links | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I am confused here.
From a quick look at the reference, it says Rust
union
s do not have an active member (unlike C++), i.e. bits are always re-interpreted. It also says reading uninitialized memory fromunion
s is OK, so that is not a concern either. Thus, foru32
etc. it would seem it is always safe to read from it since there are no invalid values possible.So two questions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine the following union
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:
despite the fact that all 0s is a valid byte representation for
[u8; 4]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Would something like this be reasonable to you all?
// SAFETY: `fd` is a `u32`; any bit pattern is a valid representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view.read
already copies it :) it's just that this is copied directly in kernel space rather than from userspace.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.