Merge pull request #308 from wedsonaf/file-object

Automatically handle reference count on `File` instances.

Author: ojeda

rust/kernel/bindings_helper.h

@@ -13,6 +13,7 @@
 #include <linux/miscdevice.h>
 #include <linux/poll.h>
 #include <linux/mm.h>
+#include <linux/file.h>
 #include <uapi/linux/android/binder.h>
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>

rust/kernel/error.rs

@@ -53,6 +53,9 @@ impl Error {
     /// Interrupted system call.
     pub const EINTR: Self = Error(-(bindings::EINTR as i32));
 
+    /// Bad file number.
+    pub const EBADF: Self = Error(-(bindings::EBADF as i32));
+
     /// Creates an [`Error`] from a kernel error code.
     pub fn from_kernel_errno(errno: c_types::c_int) -> Error {
         Error(errno)

rust/kernel/file.rs

@@ -5,26 +5,32 @@
 //! C headers: [`include/linux/fs.h`](../../../../include/linux/fs.h) and
 //! [`include/linux/file.h`](../../../../include/linux/file.h)
 
-use crate::bindings;
+use crate::{bindings, error::Error, Result};
+use core::{mem::ManuallyDrop, ops::Deref};
 
 /// Wraps the kernel's `struct file`.
 ///
 /// # Invariants
 ///
-/// The pointer [`File::ptr`] is non-null and valid.
+/// The pointer [`File::ptr`] is non-null and valid. Its reference count is also non-zero.
 pub struct File {
-    pub(crate) ptr: *const bindings::file,
+    pub(crate) ptr: *mut bindings::file,
 }
 
 impl File {
-    /// Constructs a new [`struct file`] wrapper.
+    /// Constructs a new [`struct file`] wrapper from a file descriptor.
     ///
-    /// # Safety
-    ///
-    /// The pointer `ptr` must be non-null and valid for the lifetime of the object.
-    pub(crate) unsafe fn from_ptr(ptr: *const bindings::file) -> File {
-        // INVARIANTS: the safety contract ensures the type invariant will hold.
-        File { ptr }
+    /// The file descriptor belongs to the current process.
+    pub fn from_fd(fd: u32) -> Result<Self> {
+        // SAFETY: FFI call, there are no requirements on `fd`.
+        let ptr = unsafe { bindings::fget(fd) };
+        if ptr.is_null() {
+            return Err(Error::EBADF);
+        }
+
+        // INVARIANTS: We checked that `ptr` is non-null, so it is valid. `fget` increments the ref
+        // count before returning.
+        Ok(Self { ptr })
     }
 
     /// Returns the current seek/cursor/pointer position (`struct file::f_pos`).
@@ -39,3 +45,40 @@ impl File {
         unsafe { (*self.ptr).f_flags & bindings::O_NONBLOCK == 0 }
     }
 }
+
+impl Drop for File {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `File::ptr` has a non-zero reference count.
+        unsafe { bindings::fput(self.ptr) };
+    }
+}
+
+/// A wrapper for [`File`] that doesn't automatically decrement that refcount when dropped.
+///
+/// We need the wrapper because [`ManuallyDrop`] alone would allow callers to call
+/// [`ManuallyDrop::into_inner`]. This would allow an unsafe sequence to be triggered without
+/// `unsafe` blocks because it would trigger an unbalanced call to `fput`.
+///
+/// # Invariants
+///
+/// The wrapped [`File`] remains valid for the lifetime of the object.
+pub(crate) struct FileRef(ManuallyDrop<File>);
+
+impl FileRef {
+    /// Constructs a new [`struct file`] wrapper that doesn't change its reference count.
+    ///
+    /// # Safety
+    ///
+    /// The pointer `ptr` must be non-null and valid for the lifetime of the object.
+    pub(crate) unsafe fn from_ptr(ptr: *mut bindings::file) -> Self {
+        Self(ManuallyDrop::new(File { ptr }))
+    }
+}
+
+impl Deref for FileRef {
+    type Target = File;
+
+    fn deref(&self) -> &Self::Target {
+        self.0.deref()
+    }
+}

rust/kernel/file_operations.rs

@@ -12,7 +12,7 @@ use alloc::boxed::Box;
 use crate::{
     bindings, c_types,
     error::{Error, Result},
-    file::File,
+    file::{File, FileRef},
     from_kernel_result,
     io_buffer::{IoBufferReader, IoBufferWriter},
     iov_iter::IovIter,
@@ -102,7 +102,7 @@ unsafe extern "C" fn read_callback<T: FileOperations>(
         let f = &*((*file).private_data as *const T);
         // No `FMODE_UNSIGNED_OFFSET` support, so `offset` must be in [0, 2^63).
         // See discussion in https://github.com/fishinabarrel/linux-kernel-module-rust/pull/113
-        let read = f.read(&File::from_ptr(file), &mut data, (*offset).try_into()?)?;
+        let read = f.read(&FileRef::from_ptr(file), &mut data, (*offset).try_into()?)?;
         (*offset) += bindings::loff_t::try_from(read).unwrap();
         Ok(read as _)
     }
@@ -117,7 +117,7 @@ unsafe extern "C" fn read_iter_callback<T: FileOperations>(
         let file = (*iocb).ki_filp;
         let offset = (*iocb).ki_pos;
         let f = &*((*file).private_data as *const T);
-        let read = f.read(&File::from_ptr(file), &mut iter, offset.try_into()?)?;
+        let read = f.read(&FileRef::from_ptr(file), &mut iter, offset.try_into()?)?;
         (*iocb).ki_pos += bindings::loff_t::try_from(read).unwrap();
         Ok(read as _)
     }
@@ -134,7 +134,7 @@ unsafe extern "C" fn write_callback<T: FileOperations>(
         let f = &*((*file).private_data as *const T);
         // No `FMODE_UNSIGNED_OFFSET` support, so `offset` must be in [0, 2^63).
         // See discussion in https://github.com/fishinabarrel/linux-kernel-module-rust/pull/113
-        let written = f.write(&File::from_ptr(file), &mut data, (*offset).try_into()?)?;
+        let written = f.write(&FileRef::from_ptr(file), &mut data, (*offset).try_into()?)?;
         (*offset) += bindings::loff_t::try_from(written).unwrap();
         Ok(written as _)
     }
@@ -149,7 +149,7 @@ unsafe extern "C" fn write_iter_callback<T: FileOperations>(
         let file = (*iocb).ki_filp;
         let offset = (*iocb).ki_pos;
         let f = &*((*file).private_data as *const T);
-        let written = f.write(&File::from_ptr(file), &mut iter, offset.try_into()?)?;
+        let written = f.write(&FileRef::from_ptr(file), &mut iter, offset.try_into()?)?;
         (*iocb).ki_pos += bindings::loff_t::try_from(written).unwrap();
         Ok(written as _)
     }
@@ -160,7 +160,7 @@ unsafe extern "C" fn release_callback<T: FileOperations>(
     file: *mut bindings::file,
 ) -> c_types::c_int {
     let ptr = mem::replace(&mut (*file).private_data, ptr::null_mut());
-    T::release(T::Wrapper::from_pointer(ptr as _), &File::from_ptr(file));
+    T::release(T::Wrapper::from_pointer(ptr as _), &FileRef::from_ptr(file));
     0
 }
 
@@ -177,7 +177,7 @@ unsafe extern "C" fn llseek_callback<T: FileOperations>(
             _ => return Err(Error::EINVAL),
         };
         let f = &*((*file).private_data as *const T);
-        let off = f.seek(&File::from_ptr(file), off)?;
+        let off = f.seek(&FileRef::from_ptr(file), off)?;
         Ok(off as bindings::loff_t)
     }
 }
@@ -191,7 +191,7 @@ unsafe extern "C" fn unlocked_ioctl_callback<T: FileOperations>(
         let f = &*((*file).private_data as *const T);
         // SAFETY: This function is called by the kernel, so it must set `fs` appropriately.
         let mut cmd = IoctlCommand::new(cmd as _, arg as _);
-        let ret = f.ioctl(&File::from_ptr(file), &mut cmd)?;
+        let ret = f.ioctl(&FileRef::from_ptr(file), &mut cmd)?;
         Ok(ret as _)
     }
 }
@@ -205,7 +205,7 @@ unsafe extern "C" fn compat_ioctl_callback<T: FileOperations>(
         let f = &*((*file).private_data as *const T);
         // SAFETY: This function is called by the kernel, so it must set `fs` appropriately.
         let mut cmd = IoctlCommand::new(cmd as _, arg as _);
-        let ret = f.compat_ioctl(&File::from_ptr(file), &mut cmd)?;
+        let ret = f.compat_ioctl(&FileRef::from_ptr(file), &mut cmd)?;
         Ok(ret as _)
     }
 }
@@ -216,7 +216,7 @@ unsafe extern "C" fn mmap_callback<T: FileOperations>(
 ) -> c_types::c_int {
     from_kernel_result! {
         let f = &*((*file).private_data as *const T);
-        f.mmap(&File::from_ptr(file), &mut *vma)?;
+        f.mmap(&FileRef::from_ptr(file), &mut *vma)?;
         Ok(0)
     }
 }
@@ -232,7 +232,7 @@ unsafe extern "C" fn fsync_callback<T: FileOperations>(
         let end = end.try_into()?;
         let datasync = datasync != 0;
         let f = &*((*file).private_data as *const T);
-        let res = f.fsync(&File::from_ptr(file), start, end, datasync)?;
+        let res = f.fsync(&FileRef::from_ptr(file), start, end, datasync)?;
         Ok(res.try_into().unwrap())
     }
 }
@@ -242,7 +242,7 @@ unsafe extern "C" fn poll_callback<T: FileOperations>(
     wait: *mut bindings::poll_table_struct,
 ) -> bindings::__poll_t {
     let f = &*((*file).private_data as *const T);
-    match f.poll(&File::from_ptr(file), &PollTable::from_ptr(wait)) {
+    match f.poll(&FileRef::from_ptr(file), &PollTable::from_ptr(wait)) {
         Ok(v) => v,
         Err(_) => bindings::POLLERR,
     }