Remove cid parameter from VirtualMachineService
Cid parameter has been passed just to identify the guest VMs. This makes
the server identify itself, with incoming vsock address.
Bug: 199259751
Test: atest MicrodroidHostTestCases ComposHostTestCases
Change-Id: I8dacfec80574fe765b96139cdecb6f3192728577
diff --git a/compos/src/compsvc_main.rs b/compos/src/compsvc_main.rs
index 6887947..fc00039 100644
--- a/compos/src/compsvc_main.rs
+++ b/compos/src/compsvc_main.rs
@@ -36,9 +36,6 @@
use binder_common::rpc_server::run_rpc_server;
use compos_common::COMPOS_VSOCK_PORT;
use log::{debug, error};
-use nix::ioctl_read_bad;
-use std::fs::OpenOptions;
-use std::os::unix::io::AsRawFd;
/// The CID representing the host VM
const VMADDR_CID_HOST: u32 = 2;
@@ -64,12 +61,11 @@
let service = compsvc::new_binder()?.as_binder();
let vm_service = get_vm_service()?;
- let local_cid = get_local_cid()?;
debug!("compsvc is starting as a rpc service.");
let retval = run_rpc_server(service, COMPOS_VSOCK_PORT, || {
- if let Err(e) = vm_service.notifyPayloadReady(local_cid as i32) {
+ if let Err(e) = vm_service.notifyPayloadReady() {
error!("Unable to notify ready: {}", e);
}
});
@@ -94,26 +90,3 @@
FromIBinder::try_from(ibinder).context("Connecting to IVirtualMachineService")
}
-
-// TODO(b/199259751): remove this after VS can check the peer addresses of binder clients
-fn get_local_cid() -> Result<u32> {
- let f = OpenOptions::new()
- .read(true)
- .write(false)
- .open("/dev/vsock")
- .context("Failed to open /dev/vsock")?;
- let mut cid = 0;
- // SAFETY: the kernel only modifies the given u32 integer.
- unsafe { vm_sockets_get_local_cid(f.as_raw_fd(), &mut cid) }
- .context("Failed to get local CID")?;
- Ok(cid)
-}
-
-// TODO(b/199259751): remove this after VS can check the peer addresses of binder clients
-const IOCTL_VM_SOCKETS_GET_LOCAL_CID: usize = 0x7b9;
-ioctl_read_bad!(
- /// Gets local cid from /dev/vsock
- vm_sockets_get_local_cid,
- IOCTL_VM_SOCKETS_GET_LOCAL_CID,
- u32
-);
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index 177a0db..ac62e58 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -27,12 +27,11 @@
use log::{error, info, warn};
use microdroid_metadata::{write_metadata, Metadata};
use microdroid_payload_config::{Task, TaskType, VmPayloadConfig};
-use nix::ioctl_read_bad;
use payload::{get_apex_data_from_payload, load_metadata, to_metadata};
use rustutils::system_properties;
use rustutils::system_properties::PropertyWatcher;
use std::fs::{self, File, OpenOptions};
-use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
+use std::os::unix::io::{FromRawFd, IntoRawFd};
use std::path::Path;
use std::process::{Command, Stdio};
use std::str;
@@ -67,27 +66,6 @@
}
}
-const IOCTL_VM_SOCKETS_GET_LOCAL_CID: usize = 0x7b9;
-ioctl_read_bad!(
- /// Gets local cid from /dev/vsock
- vm_sockets_get_local_cid,
- IOCTL_VM_SOCKETS_GET_LOCAL_CID,
- u32
-);
-
-// TODO: remove this after VS can check the peer addresses of binder clients
-fn get_local_cid() -> Result<u32> {
- let f = OpenOptions::new()
- .read(true)
- .write(false)
- .open("/dev/vsock")
- .context("failed to open /dev/vsock")?;
- let mut ret = 0;
- // SAFETY: the kernel only modifies the given u32 integer.
- unsafe { vm_sockets_get_local_cid(f.as_raw_fd(), &mut ret) }?;
- Ok(ret)
-}
-
fn main() {
if let Err(e) = try_main() {
error!("failed with {:?}", e);
@@ -242,14 +220,13 @@
info!("executing main task {:?}...", task);
let mut command = build_command(task)?;
- let local_cid = get_local_cid()?;
info!("notifying payload started");
- service.notifyPayloadStarted(local_cid as i32)?;
+ service.notifyPayloadStarted()?;
let exit_status = command.spawn()?.wait()?;
if let Some(code) = exit_status.code() {
info!("notifying payload finished");
- service.notifyPayloadFinished(local_cid as i32, code)?;
+ service.notifyPayloadFinished(code)?;
if code == 0 {
info!("task successfully finished");
diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp
index 2903a08..f56b261 100644
--- a/tests/testapk/src/native/testbinary.cpp
+++ b/tests/testapk/src/native/testbinary.cpp
@@ -198,21 +198,6 @@
return result;
}
-Result<unsigned> get_local_cid() {
- // TODO: remove this after VS can check the peer addresses of binder clients
- unique_fd fd(open("/dev/vsock", O_RDONLY));
- if (fd.get() == -1) {
- return ErrnoError() << "failed to open /dev/vsock";
- }
-
- unsigned cid;
- if (ioctl(fd.get(), IOCTL_VM_SOCKETS_GET_LOCAL_CID, &cid) == -1) {
- return ErrnoError() << "failed to IOCTL_VM_SOCKETS_GET_LOCAL_CID";
- }
-
- return cid;
-}
-
Result<void> start_test_service() {
class TestService : public aidl::com::android::microdroid::testservice::BnTestService {
ndk::ScopedAStatus addInteger(int32_t a, int32_t b, int32_t* out) override {
@@ -232,9 +217,7 @@
std::cerr << "failed to connect VirtualMachineService";
return;
}
- if (auto res = get_local_cid(); !res.ok()) {
- std::cerr << "failed to get local cid: " << res.error();
- } else if (!virtualMachineService->notifyPayloadReady(res.value()).isOk()) {
+ if (!virtualMachineService->notifyPayloadReady().isOk()) {
std::cerr << "failed to notify payload ready to virtualizationservice";
}
};
diff --git a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
index fba83c8..8611898 100644
--- a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
+++ b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
@@ -31,19 +31,16 @@
/**
* Notifies that the payload has started.
- * TODO(b/191845268): remove cid parameter
*/
- void notifyPayloadStarted(int cid);
+ void notifyPayloadStarted();
/**
* Notifies that the payload is ready to serve.
- * TODO(b/191845268): remove cid parameter
*/
- void notifyPayloadReady(int cid);
+ void notifyPayloadReady();
/**
* Notifies that the payload has finished.
- * TODO(b/191845268): remove cid parameter
*/
- void notifyPayloadFinished(int cid, int exitCode);
+ void notifyPayloadFinished(int exitCode);
}
diff --git a/virtualizationservice/src/aidl.rs b/virtualizationservice/src/aidl.rs
index fa6ee40..2f901b4 100644
--- a/virtualizationservice/src/aidl.rs
+++ b/virtualizationservice/src/aidl.rs
@@ -55,8 +55,10 @@
use std::fs::{create_dir, File, OpenOptions};
use std::io::{Error, ErrorKind, Write};
use std::num::NonZeroU32;
+use std::os::raw;
use std::os::unix::io::{FromRawFd, IntoRawFd};
use std::path::{Path, PathBuf};
+use std::ptr::null_mut;
use std::sync::{Arc, Mutex, Weak};
use vmconfig::VmConfig;
use vsock::{SockAddr, VsockListener, VsockStream};
@@ -345,15 +347,18 @@
});
// binder server for vm
- let state = service.state.clone(); // reference to state (not the state itself) is copied
+ let mut state = service.state.clone(); // reference to state (not the state itself) is copied
std::thread::spawn(move || {
- let mut service = VirtualMachineService::new_binder(state).as_binder();
+ let state_ptr = &mut state as *mut _ as *mut raw::c_void;
+
debug!("virtual machine service is starting as an RPC service.");
- // SAFETY: Service ownership is transferring to the server and won't be valid afterward.
- // Plus the binder objects are threadsafe.
+ // SAFETY: factory function is only ever called by RunRpcServerWithFactory, within the
+ // lifetime of the state, with context taking the pointer value above (so a properly
+ // aligned non-null pointer to an initialized instance).
let retval = unsafe {
- binder_rpc_unstable_bindgen::RunRpcServer(
- service.as_native_mut() as *mut binder_rpc_unstable_bindgen::AIBinder,
+ binder_rpc_unstable_bindgen::RunRpcServerWithFactory(
+ Some(VirtualMachineService::factory),
+ state_ptr,
VM_BINDER_SERVICE_PORT as u32,
)
};
@@ -846,13 +851,14 @@
#[derive(Debug, Default)]
struct VirtualMachineService {
state: Arc<Mutex<State>>,
+ cid: Cid,
}
impl Interface for VirtualMachineService {}
impl IVirtualMachineService for VirtualMachineService {
- fn notifyPayloadStarted(&self, cid: i32) -> binder::Result<()> {
- let cid = cid as Cid;
+ fn notifyPayloadStarted(&self) -> binder::Result<()> {
+ let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM having CID {} started payload", cid);
vm.update_payload_state(PayloadState::Started)
@@ -869,8 +875,8 @@
}
}
- fn notifyPayloadReady(&self, cid: i32) -> binder::Result<()> {
- let cid = cid as Cid;
+ fn notifyPayloadReady(&self) -> binder::Result<()> {
+ let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM having CID {} payload is ready", cid);
vm.update_payload_state(PayloadState::Ready)
@@ -886,8 +892,8 @@
}
}
- fn notifyPayloadFinished(&self, cid: i32, exit_code: i32) -> binder::Result<()> {
- let cid = cid as Cid;
+ fn notifyPayloadFinished(&self, exit_code: i32) -> binder::Result<()> {
+ let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM having CID {} finished payload", cid);
vm.update_payload_state(PayloadState::Finished)
@@ -905,9 +911,26 @@
}
impl VirtualMachineService {
- fn new_binder(state: Arc<Mutex<State>>) -> Strong<dyn IVirtualMachineService> {
+ // SAFETY: Service ownership is held by state, and the binder objects are threadsafe.
+ pub unsafe extern "C" fn factory(
+ cid: Cid,
+ context: *mut raw::c_void,
+ ) -> *mut binder_rpc_unstable_bindgen::AIBinder {
+ let state_ptr = context as *mut Arc<Mutex<State>>;
+ let state = state_ptr.as_ref().unwrap();
+ if let Some(vm) = state.lock().unwrap().get_vm(cid) {
+ let mut vm_service = vm.vm_service.lock().unwrap();
+ let service = vm_service.get_or_insert_with(|| Self::new_binder(state.clone(), cid));
+ service.as_binder().as_native_mut() as *mut binder_rpc_unstable_bindgen::AIBinder
+ } else {
+ error!("connection from cid={} is not from a guest VM", cid);
+ null_mut()
+ }
+ }
+
+ fn new_binder(state: Arc<Mutex<State>>, cid: Cid) -> Strong<dyn IVirtualMachineService> {
BnVirtualMachineService::new_binder(
- VirtualMachineService { state },
+ VirtualMachineService { state, cid },
BinderFeatures::default(),
)
}
diff --git a/virtualizationservice/src/crosvm.rs b/virtualizationservice/src/crosvm.rs
index 4844657..8a5a7dd 100644
--- a/virtualizationservice/src/crosvm.rs
+++ b/virtualizationservice/src/crosvm.rs
@@ -29,6 +29,8 @@
use std::sync::{Arc, Mutex};
use std::thread;
use vsock::VsockStream;
+use android_system_virtualmachineservice::binder::Strong;
+use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::IVirtualMachineService;
const CROSVM_PATH: &str = "/apex/com.android.virt/bin/crosvm";
@@ -132,6 +134,8 @@
pub callbacks: VirtualMachineCallbacks,
/// Input/output stream of the payload run in the VM.
pub stream: Mutex<Option<VsockStream>>,
+ /// VirtualMachineService binder object for the VM.
+ pub vm_service: Mutex<Option<Strong<dyn IVirtualMachineService>>>,
/// The latest lifecycle state which the payload reported itself to be in.
payload_state: Mutex<PayloadState>,
}
@@ -158,6 +162,7 @@
requester_debug_pid,
callbacks: Default::default(),
stream: Mutex::new(None),
+ vm_service: Mutex::new(None),
payload_state: Mutex::new(PayloadState::Starting),
})
}