ptfs: use BorrowedFd instead of RawFd when possible

jiangliu · eryugey · commit cc0a191bd19f · 2023-11-14T11:37:21.000+08:00
Use BorrowedFd instead of RawFd when possible. By using BorrowedFd
we can explicitly manage the lifetime of the underlying RawFd,
to avoid misusing after file close.

Signed-off-by: Jiang Liu &lt;gerry@linux.alibaba.com&gt;
diff --git a/src/passthrough/file_handle.rs b/src/passthrough/file_handle.rs
@@ -8,6 +8,7 @@ use std::ffi::CStr;
 use std::fmt::{Debug, Formatter};
 use std::fs::File;
 use std::io;
+use std::os::fd::AsFd;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use std::sync::Arc;
 
@@ -283,7 +284,7 @@ impl Debug for OpenableFileHandle {
         write!(
             f,
             "Openable file handle: mountfd {}, type {}, len {}",
-            self.mount_fd.as_raw_fd(),
+            self.mount_fd.as_fd().as_raw_fd(),
             fh.handle_type,
             fh.handle_bytes
         )
@@ -295,7 +296,7 @@ impl OpenableFileHandle {
     pub fn open(&self, flags: libc::c_int) -> io::Result<File> {
         let ret = unsafe {
             open_by_handle_at(
-                self.mount_fd.as_raw_fd(),
+                self.mount_fd.as_fd().as_raw_fd(),
                 self.handle.handle.wrapper.as_fam_struct_ptr(),
                 flags,
             )
diff --git a/src/passthrough/mod.rs b/src/passthrough/mod.rs
@@ -18,6 +18,7 @@ use std::fs::File;
 use std::io;
 use std::marker::PhantomData;
 use std::ops::{Deref, DerefMut};
+use std::os::fd::{AsFd, BorrowedFd};
 use std::os::unix::ffi::OsStringExt;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::path::PathBuf;
@@ -85,6 +86,15 @@ impl AsRawFd for InodeFile<'_> {
     }
 }
 
+impl AsFd for InodeFile<'_> {
+    fn as_fd(&self) -> BorrowedFd<'_> {
+        match self {
+            Self::Owned(file) => file.as_fd(),
+            Self::Ref(file_ref) => file_ref.as_fd(),
+        }
+    }
+}
+
 #[derive(Debug)]
 enum InodeHandle {
     File(File),
@@ -268,12 +278,8 @@ impl HandleData {
         (self.lock.lock().unwrap(), &self.file)
     }
 
-    // When making use of the underlying RawFd, the caller must ensure that the Arc<HandleData>
-    // object is within scope. Otherwise it may cause race window to access wrong target fd.
-    // By introducing this method, we could explicitly audit all callers making use of the
-    // underlying RawFd.
-    fn get_handle_raw_fd(&self) -> RawFd {
-        self.file.as_raw_fd()
+    fn borrow_fd(&self) -> BorrowedFd {
+        self.file.as_fd()
     }
 
     fn get_flags(&self) -> u32 {
@@ -977,10 +983,8 @@ mod tests {
     use crate::api::{Vfs, VfsOptions};
     use caps::{CapSet, Capability};
     use log;
-    use std::fs::File;
     use std::io::Read;
     use std::ops::Deref;
-    use std::os::unix::io::FromRawFd;
     use std::os::unix::prelude::MetadataExt;
     use vmm_sys_util::{tempdir::TempDir, tempfile::TempFile};
 
@@ -1231,7 +1235,7 @@ mod tests {
         };
         let (entry, handle, _, _) = fs.create(&ctx, ROOT_ID, &fname, args).unwrap();
         let handle_data = fs.handle_map.get(handle.unwrap(), entry.inode).unwrap();
-        let mut f = unsafe { File::from_raw_fd(handle_data.get_handle_raw_fd()) };
+        let (_guard, mut f) = handle_data.get_file_mut();
         let mut buf = [0; 4];
         // Buggy code return EBADF on read
         let n = f.read(&mut buf).unwrap();
@@ -1244,7 +1248,7 @@ mod tests {
             .open(&ctx, entry.inode, libc::O_WRONLY as u32, 0)
             .unwrap();
         let handle_data = fs.handle_map.get(handle.unwrap(), entry.inode).unwrap();
-        let mut f = unsafe { File::from_raw_fd(handle_data.get_handle_raw_fd()) };
+        let (_guard, mut f) = handle_data.get_file_mut();
         let mut buf = [0; 4];
         let n = f.read(&mut buf).unwrap();
         assert_eq!(n, 0);
diff --git a/src/passthrough/mount_fd.rs b/src/passthrough/mount_fd.rs
@@ -5,6 +5,7 @@ use std::collections::{HashMap, HashSet};
 use std::ffi::CString;
 use std::fs::File;
 use std::io::{self, Read, Seek};
+use std::os::fd::{AsFd, BorrowedFd};
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::sync::{Arc, Mutex, RwLock, Weak};
 
@@ -22,9 +23,9 @@ pub struct MountFd {
     map: Weak<RwLock<HashMap<MountId, Weak<MountFd>>>>,
 }
 
-impl AsRawFd for MountFd {
-    fn as_raw_fd(&self) -> RawFd {
-        self.file.as_raw_fd()
+impl AsFd for MountFd {
+    fn as_fd(&self) -> BorrowedFd {
+        self.file.as_fd()
     }
 }
 
@@ -464,7 +465,7 @@ mod tests {
         assert_eq!(mount_fds.map.read().unwrap().len(), 1);
 
         // Ensure fd1 and fd2 are the same object.
-        assert_eq!(fd1.as_raw_fd(), fd2.as_raw_fd());
+        assert_eq!(fd1.as_fd().as_raw_fd(), fd2.as_fd().as_raw_fd());
 
         drop(fd1);
         assert_eq!(Arc::strong_count(&fd2), 1);
diff --git a/src/passthrough/sync_io.rs b/src/passthrough/sync_io.rs
@@ -152,7 +152,7 @@ impl<S: BitmapSlice + Send + Sync> PassthroughFs<S> {
                         type_: u32::from(dirent64.d_ty),
                         name,
                     },
-                    data.get_handle_raw_fd(),
+                    data.borrow_fd().as_raw_fd(),
                 )
             };
 
@@ -663,7 +663,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         // Manually implement File::try_clone() by borrowing fd of data.file instead of dup().
         // It's safe because the `data` variable's lifetime spans the whole function,
         // so data.file won't be closed.
-        let f = unsafe { File::from_raw_fd(data.get_handle_raw_fd()) };
+        let f = unsafe { File::from_raw_fd(data.borrow_fd().as_raw_fd()) };
 
         self.check_fd_flags(data, f.as_raw_fd(), flags)?;
 
@@ -690,7 +690,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         // Manually implement File::try_clone() by borrowing fd of data.file instead of dup().
         // It's safe because the `data` variable's lifetime spans the whole function,
         // so data.file won't be closed.
-        let f = unsafe { File::from_raw_fd(data.get_handle_raw_fd()) };
+        let f = unsafe { File::from_raw_fd(data.borrow_fd().as_raw_fd()) };
 
         self.check_fd_flags(data, f.as_raw_fd(), flags)?;
 
@@ -732,7 +732,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         let inode_data = self.inode_map.get(inode)?;
 
         enum Data {
-            Handle(Arc<HandleData>, RawFd),
+            Handle(Arc<HandleData>),
             ProcPath(CString),
         }
 
@@ -745,8 +745,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
             // If we have a handle then use it otherwise get a new fd from the inode.
             if let Some(handle) = handle {
                 let hd = self.handle_map.get(handle, inode)?;
-                let fd = hd.get_handle_raw_fd();
-                Data::Handle(hd, fd)
+                Data::Handle(hd)
             } else {
                 let pathname = CString::new(format!("{}", file.as_raw_fd()))
                     .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
@@ -762,7 +761,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
             // Safe because this doesn't modify any memory and we check the return value.
             let res = unsafe {
                 match data {
-                    Data::Handle(_, fd) => libc::fchmod(fd, attr.st_mode),
+                    Data::Handle(ref h) => libc::fchmod(h.borrow_fd().as_raw_fd(), attr.st_mode),
                     Data::ProcPath(ref p) => {
                         libc::fchmodat(self.proc_self_fd.as_raw_fd(), p.as_ptr(), attr.st_mode, 0)
                     }
@@ -817,7 +816,9 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
 
             // Safe because this doesn't modify any memory and we check the return value.
             let res = match data {
-                Data::Handle(_, fd) => unsafe { libc::ftruncate(fd, attr.st_size) },
+                Data::Handle(ref h) => unsafe {
+                    libc::ftruncate(h.borrow_fd().as_raw_fd(), attr.st_size)
+                },
                 _ => {
                     // There is no `ftruncateat` so we need to get a new fd and truncate it.
                     let f = self.open_inode(inode, libc::O_NONBLOCK | libc::O_RDWR)?;
@@ -857,7 +858,9 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
 
             // Safe because this doesn't modify any memory and we check the return value.
             let res = match data {
-                Data::Handle(_, fd) => unsafe { libc::futimens(fd, tvs.as_ptr()) },
+                Data::Handle(ref h) => unsafe {
+                    libc::futimens(h.borrow_fd().as_raw_fd(), tvs.as_ptr())
+                },
                 Data::ProcPath(ref p) => unsafe {
                     libc::utimensat(self.proc_self_fd.as_raw_fd(), p.as_ptr(), tvs.as_ptr(), 0)
                 },
@@ -1043,7 +1046,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         // behavior by doing the same thing (dup-ing the fd and then immediately closing it). Safe
         // because this doesn't modify any memory and we check the return values.
         unsafe {
-            let newfd = libc::dup(data.get_handle_raw_fd());
+            let newfd = libc::dup(data.borrow_fd().as_raw_fd());
             if newfd < 0 {
                 return Err(io::Error::last_os_error());
             }
@@ -1064,14 +1067,14 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         handle: Handle,
     ) -> io::Result<()> {
         let data = self.get_data(handle, inode, libc::O_RDONLY)?;
-        let fd = data.get_handle_raw_fd();
+        let fd = data.borrow_fd();
 
         // Safe because this doesn't modify any memory and we check the return value.
         let res = unsafe {
             if datasync {
-                libc::fdatasync(fd)
+                libc::fdatasync(fd.as_raw_fd())
             } else {
-                libc::fsync(fd)
+                libc::fsync(fd.as_raw_fd())
             }
         };
         if res == 0 {
@@ -1276,7 +1279,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
     ) -> io::Result<()> {
         // Let the Arc<HandleData> in scope, otherwise fd may get invalid.
         let data = self.get_data(handle, inode, libc::O_RDWR)?;
-        let fd = data.get_handle_raw_fd();
+        let fd = data.borrow_fd();
 
         if self.seal_size.load(Ordering::Relaxed) {
             let st = stat_fd(&fd, None)?;
@@ -1292,7 +1295,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
         // Safe because this doesn't modify any memory and we check the return value.
         let res = unsafe {
             libc::fallocate64(
-                fd,
+                fd.as_raw_fd(),
                 mode as libc::c_int,
                 offset as libc::off64_t,
                 length as libc::off64_t,
