From ac412dacd9bd41418df8e8868988a9c2c3e07b6a Mon Sep 17 00:00:00 2001
From: Xiaohe Yang <yangxiaohe2@huawei.com>
Date: Thu, 24 Dec 2020 11:12:09 +0800
Subject: [PATCH 1/3] address_space: fix the bug that other user-space process
 cannot use shared memory of stratovirt process

The reason is that created File's lifecycle ends when exiting `create_host_mmaps` function,
and the file descriptor which is saved in `HostMemMapping` is invalid after
exiting `create_host_mmaps` function.

This commit fix this bug by saving `Arc<File>` in `HostMemMapping`,
so we can get the file descriptor of this file in the whole lifecycle of
`HostMemMapping` (i.e. the whole lifecycle of stratovirt process).

Signed-off-by: Xiaohe Yang <yangxiaohe2@huawei.com>
---
 address_space/src/address_space.rs   |  6 +--
 address_space/src/host_mmap.rs       | 56 +++++++++++-----------------
 address_space/src/lib.rs             |  2 +-
 address_space/src/listener.rs        |  2 +-
 address_space/src/region.rs          | 13 +++----
 boot_loader/src/x86_64/bootparam.rs  |  2 +-
 boot_loader/src/x86_64/mod.rs        |  2 +-
 device_model/src/mmio/virtio_mmio.rs |  2 +-
 device_model/src/virtio/queue.rs     |  2 +-
 9 files changed, 36 insertions(+), 51 deletions(-)

diff --git a/address_space/src/address_space.rs b/address_space/src/address_space.rs
index 4bab75b..21f25bd 100644
--- a/address_space/src/address_space.rs
+++ b/address_space/src/address_space.rs
@@ -658,9 +658,9 @@ mod test {
         };
 
         let ram1 =
-            Arc::new(HostMemMapping::new(GuestAddress(0), 1000, -1, 0, false, false).unwrap());
+            Arc::new(HostMemMapping::new(GuestAddress(0), 1000, None, false, false).unwrap());
         let ram2 =
-            Arc::new(HostMemMapping::new(GuestAddress(2000), 1000, -1, 0, false, false).unwrap());
+            Arc::new(HostMemMapping::new(GuestAddress(2000), 1000, None, false, false).unwrap());
         let region_a = Region::init_ram_region(ram1.clone());
         let region_b = Region::init_ram_region(ram2.clone());
         root.add_subregion(region_a, ram1.start_address().raw_value())
@@ -724,7 +724,7 @@ mod test {
         let root = Region::init_container_region(8000);
         let space = AddressSpace::new(root.clone()).unwrap();
         let ram1 =
-            Arc::new(HostMemMapping::new(GuestAddress(0), 1000, -1, 0, false, false).unwrap());
+            Arc::new(HostMemMapping::new(GuestAddress(0), 1000, None, false, false).unwrap());
         let region_a = Region::init_ram_region(ram1.clone());
         root.add_subregion(region_a, ram1.start_address().raw_value())
             .unwrap();
diff --git a/address_space/src/host_mmap.rs b/address_space/src/host_mmap.rs
index 9760f61..1701036 100644
--- a/address_space/src/host_mmap.rs
+++ b/address_space/src/host_mmap.rs
@@ -20,10 +20,11 @@ use crate::errors::{ErrorKind, Result, ResultExt};
 use crate::{AddressRange, GuestAddress};
 
 /// FileBackend represents backend-file of `HostMemMapping`.
+#[derive(Clone)]
 pub struct FileBackend {
     /// File we used to map memory.
-    pub file: File,
-    /// Offset from where the file begins.
+    pub file: Arc<File>,
+    /// Represents HostMmapping's offset in this file.
     pub offset: u64,
 }
 
@@ -72,7 +73,7 @@ impl FileBackend {
         }
 
         Ok(FileBackend {
-            file,
+            file: Arc::new(file),
             offset: 0_u64,
         })
     }
@@ -103,23 +104,17 @@ pub fn create_host_mmaps(
 
         anon_file.set_len(file_len)?;
         f_back = Some(FileBackend {
-            file: anon_file,
+            file: Arc::new(anon_file),
             offset: 0,
         });
     }
 
     let mut mappings = Vec::new();
     for range in ranges.iter() {
-        let (fd, offset) = if let Some(fb) = f_back.as_ref() {
-            (fb.file.as_raw_fd(), fb.offset)
-        } else {
-            (-1, 0)
-        };
         mappings.push(Arc::new(HostMemMapping::new(
             GuestAddress(range.0),
             range.1,
-            fd,
-            offset,
+            f_back.clone(),
             mem_config.dump_guest_core,
             mem_config.mem_share,
         )?));
@@ -138,12 +133,8 @@ pub struct HostMemMapping {
     address_range: AddressRange,
     /// The start address of mapped memory.
     host_addr: *mut u8,
-    /// The raw file descriptor that backs this mapping.
-    /// If anonymous mapping, this field is -1.
-    fd: RawFd,
-    /// Offset in file that backs this mapping.
-    /// If anonymous mapping, this field is 0.
-    file_offset: u64,
+    /// Represents file and offset-in-file that backs this mapping.
+    file_back: Option<FileBackend>,
 }
 
 // Send and Sync is not auto-implemented for raw pointer type
@@ -159,8 +150,7 @@ impl HostMemMapping {
     ///
     /// * `guest_addr` - The start address im memory.
     /// * `size` - Size of memory that will be mapped.
-    /// * `file_back` - The file's raw fd that backs memory,
-    /// * `file_offset` - Offset in the file that backs memory.
+    /// * `file_back` - Information of file and offset-in-file that backs memory.
     /// * `dump_guest_core` - Include guest memory in core file or not.
     /// * `is_share` - This mapping is sharable or not.
     ///
@@ -170,13 +160,12 @@ impl HostMemMapping {
     pub fn new(
         guest_addr: GuestAddress,
         size: u64,
-        file_back: RawFd,
-        file_offset: u64,
+        file_back: Option<FileBackend>,
         dump_guest_core: bool,
         is_share: bool,
     ) -> Result<HostMemMapping> {
         let mut flags = libc::MAP_NORESERVE;
-        if file_back == -1 {
+        if file_back.is_none() {
             flags |= libc::MAP_ANONYMOUS;
         }
         if is_share {
@@ -191,8 +180,11 @@ impl HostMemMapping {
                 size as libc::size_t,
                 libc::PROT_READ | libc::PROT_WRITE,
                 flags,
-                file_back,
-                file_offset as i64,
+                file_back
+                    .clone()
+                    .map(|fb| fb.file.as_raw_fd())
+                    .unwrap_or(-1),
+                file_back.clone().map(|fb| fb.offset).unwrap_or(0) as i64,
             );
             if hva == libc::MAP_FAILED {
                 return Err(ErrorKind::Mmap.into());
@@ -219,8 +211,7 @@ impl HostMemMapping {
                 size,
             },
             host_addr: host_addr as *mut u8,
-            fd: file_back,
-            file_offset,
+            file_back,
         })
     }
 
@@ -242,13 +233,8 @@ impl HostMemMapping {
 
     /// Get File backend information if this mapping is backed be host-memory.
     /// return None if this mapping is an anonymous mapping.
-    ///
-    /// # Returns
-    ///
-    /// * The file descriptor of file that backs this mapping.
-    /// * The offset in file that backs this mapping.
-    pub fn file_backend(&self) -> (RawFd, u64) {
-        (self.fd, self.file_offset)
+    pub fn file_backend(&self) -> Option<FileBackend> {
+        self.file_back.clone()
     }
 }
 
@@ -275,8 +261,8 @@ mod test {
 
     #[test]
     fn test_ramblock_creation() {
-        let ram1 = HostMemMapping::new(GuestAddress(0), 100u64, -1, 0, false, false).unwrap();
-        let ram2 = HostMemMapping::new(GuestAddress(0), 100u64, -1, 0, false, false).unwrap();
+        let ram1 = HostMemMapping::new(GuestAddress(0), 100u64, None, false, false).unwrap();
+        let ram2 = HostMemMapping::new(GuestAddress(0), 100u64, None, false, false).unwrap();
         identify(ram1, 0, 100);
         identify(ram2, 0, 100);
     }
diff --git a/address_space/src/lib.rs b/address_space/src/lib.rs
index 0732618..5125444 100644
--- a/address_space/src/lib.rs
+++ b/address_space/src/lib.rs
@@ -37,7 +37,7 @@
 //!     let space = AddressSpace::new(Region::init_container_region(u64::max_value())).unwrap();
 //!
 //!     // 2. create an Ram-type Region, and set it's priority
-//!     let mem_mapping = Arc::new(HostMemMapping::new(GuestAddress(0), 0x1000, -1, 0, false, false).unwrap());
+//!     let mem_mapping = Arc::new(HostMemMapping::new(GuestAddress(0), 0x1000, None, false, false).unwrap());
 //!     let ram_region = Region::init_ram_region(mem_mapping.clone());
 //!     ram_region.set_priority(10);
 //!
diff --git a/address_space/src/listener.rs b/address_space/src/listener.rs
index 75027dd..5a5c660 100644
--- a/address_space/src/listener.rs
+++ b/address_space/src/listener.rs
@@ -555,7 +555,7 @@ mod test {
 
     fn create_ram_range(addr: u64, size: u64, offset_in_region: u64) -> FlatRange {
         let mem_mapping =
-            Arc::new(HostMemMapping::new(GuestAddress(addr), size, -1, 0, false, false).unwrap());
+            Arc::new(HostMemMapping::new(GuestAddress(addr), size, None, false, false).unwrap());
         FlatRange {
             addr_range: AddressRange::new(
                 mem_mapping.start_address().unchecked_add(offset_in_region),
diff --git a/address_space/src/region.rs b/address_space/src/region.rs
index 67016d6..01c3dbc 100644
--- a/address_space/src/region.rs
+++ b/address_space/src/region.rs
@@ -10,13 +10,12 @@
 // NON-INFRINGEMENT, MERCHANTABILITY OR FIT FOR A PARTICULAR PURPOSE.
 // See the Mulan PSL v2 for more details.
 
-use std::os::unix::io::RawFd;
 use std::sync::atomic::{AtomicI32, AtomicU64, Ordering};
 use std::sync::{Arc, Mutex, RwLock, Weak};
 
 use crate::address_space::FlatView;
 use crate::errors::{ErrorKind, Result};
-use crate::{AddressRange, AddressSpace, GuestAddress, HostMemMapping, RegionOps};
+use crate::{AddressRange, AddressSpace, FileBackend, GuestAddress, HostMemMapping, RegionOps};
 
 /// Types of Region.
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
@@ -231,10 +230,10 @@ impl Region {
         self.mem_mapping.as_ref().map(|r| r.host_address())
     }
 
-    /// Get the file information if this region is backed by host-memory,
+    /// Get the file information if this region is backed by host-memory.
     /// Return `None` if it is not a Ram-type region.
-    pub fn get_file_backend(&self) -> Option<(RawFd, u64)> {
-        self.mem_mapping.as_ref().map(|r| r.file_backend())
+    pub fn get_file_backend(&self) -> Option<FileBackend> {
+        self.mem_mapping.as_ref().and_then(|r| r.file_backend())
     }
 
     /// Return all sub-regions of this Region, the returned vector is not empty,
@@ -669,7 +668,7 @@ mod test {
     #[test]
     fn test_ram_region() {
         let mem_mapping =
-            Arc::new(HostMemMapping::new(GuestAddress(0), 1024u64, -1, 0, false, false).unwrap());
+            Arc::new(HostMemMapping::new(GuestAddress(0), 1024u64, None, false, false).unwrap());
         let ram_region = Region::init_ram_region(mem_mapping.clone());
         let data: [u8; 10] = [10; 10];
         let mut res_data: [u8; 10] = [0; 10];
@@ -703,7 +702,7 @@ mod test {
     fn test_ram_region_access() {
         // the target guest address is 0~1024 (1024 not included)
         let rgn_start = GuestAddress(0);
-        let host_mmap = HostMemMapping::new(GuestAddress(0), 1024u64, -1, 0, false, false).unwrap();
+        let host_mmap = HostMemMapping::new(GuestAddress(0), 1024u64, None, false, false).unwrap();
         let ram_region = Region::init_ram_region(Arc::new(host_mmap));
         let mut file = std::fs::File::create("/tmp/test_read_write_buffer.tmp").unwrap();
         let mut file_read = std::fs::File::open("/tmp/test_read_write_buffer.tmp").unwrap();
diff --git a/boot_loader/src/x86_64/bootparam.rs b/boot_loader/src/x86_64/bootparam.rs
index 4be939e..47d122b 100644
--- a/boot_loader/src/x86_64/bootparam.rs
+++ b/boot_loader/src/x86_64/bootparam.rs
@@ -180,7 +180,7 @@ mod test {
         let root = Region::init_container_region(0x2000_0000);
         let space = AddressSpace::new(root.clone()).unwrap();
         let ram1 = Arc::new(
-            HostMemMapping::new(GuestAddress(0), 0x1000_0000, -1, 0, false, false).unwrap(),
+            HostMemMapping::new(GuestAddress(0), 0x1000_0000, None, false, false).unwrap(),
         );
         let region_a = Region::init_ram_region(ram1.clone());
         root.add_subregion(region_a, ram1.start_address().raw_value())
diff --git a/boot_loader/src/x86_64/mod.rs b/boot_loader/src/x86_64/mod.rs
index cfabf62..ffd6119 100644
--- a/boot_loader/src/x86_64/mod.rs
+++ b/boot_loader/src/x86_64/mod.rs
@@ -504,7 +504,7 @@ mod test {
         let root = Region::init_container_region(0x2000_0000);
         let space = AddressSpace::new(root.clone()).unwrap();
         let ram1 = Arc::new(
-            HostMemMapping::new(GuestAddress(0), 0x1000_0000, -1, 0, false, false).unwrap(),
+            HostMemMapping::new(GuestAddress(0), 0x1000_0000, None, false, false).unwrap(),
         );
         let region_a = Region::init_ram_region(ram1.clone());
         root.add_subregion(region_a, ram1.start_address().raw_value())
diff --git a/device_model/src/mmio/virtio_mmio.rs b/device_model/src/mmio/virtio_mmio.rs
index 8de2ba1..662fc2c 100644
--- a/device_model/src/mmio/virtio_mmio.rs
+++ b/device_model/src/mmio/virtio_mmio.rs
@@ -530,7 +530,7 @@ mod tests {
         let root = Region::init_container_region(1 << 36);
         let sys_space = AddressSpace::new(root).unwrap();
         let host_mmap = Arc::new(
-            HostMemMapping::new(GuestAddress(0), SYSTEM_SPACE_SIZE, -1, 0, false, false).unwrap(),
+            HostMemMapping::new(GuestAddress(0), SYSTEM_SPACE_SIZE, None, false, false).unwrap(),
         );
         sys_space
             .root()
diff --git a/device_model/src/virtio/queue.rs b/device_model/src/virtio/queue.rs
index 136986e..f49755e 100644
--- a/device_model/src/virtio/queue.rs
+++ b/device_model/src/virtio/queue.rs
@@ -746,7 +746,7 @@ mod tests {
         let root = Region::init_container_region(1 << 36);
         let sys_space = AddressSpace::new(root).unwrap();
         let host_mmap = Arc::new(
-            HostMemMapping::new(GuestAddress(0), SYSTEM_SPACE_SIZE, -1, 0, false, false).unwrap(),
+            HostMemMapping::new(GuestAddress(0), SYSTEM_SPACE_SIZE, None, false, false).unwrap(),
         );
         sys_space
             .root()
-- 
2.27.0

