Skip to content

rust: kernel: add missing safety comments #382

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

Merged
merged 1 commit into from
Jul 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/kernel/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ pub struct FileDescriptorReservation {
impl FileDescriptorReservation {
/// Creates a new file descriptor reservation.
pub fn new(flags: u32) -> Result<Self> {
// SAFETY: FFI call, there are no safety requirements on `flags`.
let fd = unsafe { bindings::get_unused_fd_flags(flags) };
if fd < 0 {
return Err(Error::from_kernel_errno(fd));
Expand Down
19 changes: 18 additions & 1 deletion rust/kernel/file_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,29 @@ pub enum SeekFrom {
Current(i64),
}

/// Called by the VFS when an inode should be opened.
///
/// Calls `T::open` on the returned value of `A::convert`.
///
/// # Safety
///
/// The returned value of `A::convert` must be a valid non-null pointer and
/// `T:open` must return a valid non-null pointer on an `Ok` result.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was going to add comments and docs to all the helpers, but wasn't sure if this was something needed/wanted. If this is helpful, I can update with the rest documented.

unsafe extern "C" fn open_callback<A: FileOpenAdapter, T: FileOpener<A::Arg>>(
inode: *mut bindings::inode,
file: *mut bindings::file,
) -> c_types::c_int {
from_kernel_result! {
// SAFETY: `A::convert` must return a valid non-null pointer that
// should point to data in the inode or file that lives longer
// than the following use of `T::open`.
let arg = unsafe { A::convert(inode, file) };
// SAFETY: `arg` was previously returned by `A::convert` and must
// be a valid non-null pointer.
let ptr = T::open(unsafe { &*arg })?.into_pointer();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the unsafe { &*arg } block needs the // SAFETY comment. To arrive at a correct comment here, ask yourself how you can prove that it is always safe to convert the raw pointer arg to a shared reference, which lives long enough to satisfy the requirements of FileOpener::open().

(note that I do not know the answer to that question myself, this is intended to show what questions to ask and answer)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I wrote it this way because I'm assuming the pointer returned by convert is going to point to something in the inode or file which will definitely live beyond open callback and will live beyond T::open. I'll add that to the comment.

// SAFETY: `ptr` was previously returned by `T::open`. The returned
// value should be a boxed value and should live the length of the
// given file.
unsafe { (*file).private_data = ptr as *mut c_types::c_void };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To arrive at a correct comment here, ask yourself how you can prove that it is always safe to dereference the raw pointer called file. Is that pointer guaranteed to be valid and non-null? If so, why?

(note that I do not know the answer to that question myself, this is intended to show what questions to ask and answer)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this one a bit. I whiffed on the first draft.

Ok(0)
}
Expand Down Expand Up @@ -500,7 +516,8 @@ pub trait FileOpenAdapter {
/// # Safety
///
/// This function must be called only when [`struct file_operations::open`] is being called for
/// a file that was registered by the implementer.
/// a file that was registered by the implementer. The returned pointer must be valid and
/// not-null.
unsafe fn convert(_inode: *mut bindings::inode, _file: *mut bindings::file)
-> *const Self::Arg;
}
Expand Down