Skip to content

Clean unsafe in unsafe fns warnings #348

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 3 commits into from
Jun 4, 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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ KBUILD_RUSTCFLAGS := --emit=dep-info,obj,metadata --edition=2018 \
-Cpanic=abort -Cembed-bitcode=n -Clto=n -Crpath=n \
-Cforce-unwind-tables=n -Ccodegen-units=1 \
-Zbinary_dep_depinfo=y -Zsymbol-mangling-version=v0 \
-W unsafe_op_in_unsafe_fn
-D unsafe_op_in_unsafe_fn
KBUILD_AFLAGS_KERNEL :=
KBUILD_CFLAGS_KERNEL :=
KBUILD_RUSTCFLAGS_KERNEL :=
Expand Down
14 changes: 8 additions & 6 deletions drivers/android/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,14 @@ impl NodeDeath {
cookie,
work_links: Links::new(),
death_links: Links::new(),
inner: SpinLock::new(NodeDeathInner {
dead: false,
cleared: false,
notification_done: false,
aborted: false,
}),
inner: unsafe {
SpinLock::new(NodeDeathInner {
dead: false,
cleared: false,
notification_done: false,
aborted: false,
})
},
}
}

Expand Down
6 changes: 4 additions & 2 deletions rust/kernel/allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ unsafe impl GlobalAlloc for KernelAllocator {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
// `krealloc()` is used instead of `kmalloc()` because the latter is
// an inline function and cannot be bound to as a result.
bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8
unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 }
}

unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
bindings::kfree(ptr as *const c_types::c_void);
unsafe {
bindings::kfree(ptr as *const c_types::c_void);
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion rust/kernel/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
non_camel_case_types,
non_upper_case_globals,
non_snake_case,
improper_ctypes
improper_ctypes,
unsafe_op_in_unsafe_fn
)]
mod bindings_raw {
use crate::c_types;
Expand Down
82 changes: 43 additions & 39 deletions rust/kernel/file_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ impl PollTable {

// SAFETY: `PollTable::ptr` is guaranteed to be valid by the type invariants and the null
// check above.
let table = &*self.ptr;
let table = unsafe { &*self.ptr };
if let Some(proc) = table._qproc {
// SAFETY: All pointers are known to be valid.
proc(file.ptr as _, cv.wait_list.get(), self.ptr)
unsafe { proc(file.ptr as _, cv.wait_list.get(), self.ptr) }
}
}
}
Expand All @@ -84,9 +84,9 @@ unsafe extern "C" fn open_callback<A: FileOpenAdapter, T: FileOpener<A::Arg>>(
file: *mut bindings::file,
) -> c_types::c_int {
from_kernel_result! {
let arg = A::convert(inode, file);
let ptr = T::open(&*arg)?.into_pointer();
(*file).private_data = ptr as *mut c_types::c_void;
let arg = unsafe { A::convert(inode, file) };
let ptr = T::open(unsafe { &*arg })?.into_pointer();
unsafe { (*file).private_data = ptr as *mut c_types::c_void };
Ok(0)
}
}
Expand All @@ -98,12 +98,12 @@ unsafe extern "C" fn read_callback<T: FileOperations>(
offset: *mut bindings::loff_t,
) -> c_types::c_ssize_t {
from_kernel_result! {
let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len).writer();
let f = &*((*file).private_data as *const T);
let mut data = unsafe { UserSlicePtr::new(buf as *mut c_types::c_void, len).writer() };
let f = unsafe { &*((*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(&FileRef::from_ptr(file), &mut data, (*offset).try_into()?)?;
(*offset) += bindings::loff_t::try_from(read).unwrap();
let read = f.read(unsafe { &FileRef::from_ptr(file) }, &mut data, unsafe { *offset }.try_into()?)?;
unsafe { (*offset) += bindings::loff_t::try_from(read).unwrap() };
Ok(read as _)
}
}
Expand All @@ -113,12 +113,12 @@ unsafe extern "C" fn read_iter_callback<T: FileOperations>(
raw_iter: *mut bindings::iov_iter,
) -> isize {
from_kernel_result! {
let mut iter = IovIter::from_ptr(raw_iter);
let file = (*iocb).ki_filp;
let offset = (*iocb).ki_pos;
let f = &*((*file).private_data as *const T);
let read = f.read(&FileRef::from_ptr(file), &mut iter, offset.try_into()?)?;
(*iocb).ki_pos += bindings::loff_t::try_from(read).unwrap();
let mut iter = unsafe { IovIter::from_ptr(raw_iter) };
let file = unsafe { (*iocb).ki_filp };
let offset = unsafe { (*iocb).ki_pos };
let f = unsafe { &*((*file).private_data as *const T) };
let read = f.read(unsafe { &FileRef::from_ptr(file) }, &mut iter, offset.try_into()?)?;
unsafe { (*iocb).ki_pos += bindings::loff_t::try_from(read).unwrap() };
Ok(read as _)
}
}
Expand All @@ -130,12 +130,12 @@ unsafe extern "C" fn write_callback<T: FileOperations>(
offset: *mut bindings::loff_t,
) -> c_types::c_ssize_t {
from_kernel_result! {
let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len).reader();
let f = &*((*file).private_data as *const T);
let mut data = unsafe { UserSlicePtr::new(buf as *mut c_types::c_void, len).reader() };
let f = unsafe { &*((*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(&FileRef::from_ptr(file), &mut data, (*offset).try_into()?)?;
(*offset) += bindings::loff_t::try_from(written).unwrap();
let written = f.write(unsafe { &FileRef::from_ptr(file) }, &mut data, unsafe { *offset }.try_into()?)?;
unsafe { (*offset) += bindings::loff_t::try_from(written).unwrap() };
Ok(written as _)
}
}
Expand All @@ -145,12 +145,12 @@ unsafe extern "C" fn write_iter_callback<T: FileOperations>(
raw_iter: *mut bindings::iov_iter,
) -> isize {
from_kernel_result! {
let mut iter = IovIter::from_ptr(raw_iter);
let file = (*iocb).ki_filp;
let offset = (*iocb).ki_pos;
let f = &*((*file).private_data as *const T);
let written = f.write(&FileRef::from_ptr(file), &mut iter, offset.try_into()?)?;
(*iocb).ki_pos += bindings::loff_t::try_from(written).unwrap();
let mut iter = unsafe { IovIter::from_ptr(raw_iter) };
let file = unsafe { (*iocb).ki_filp };
let offset = unsafe { (*iocb).ki_pos };
let f = unsafe { &*((*file).private_data as *const T) };
let written = f.write(unsafe { &FileRef::from_ptr(file) }, &mut iter, offset.try_into()?)?;
unsafe { (*iocb).ki_pos += bindings::loff_t::try_from(written).unwrap() };
Ok(written as _)
}
}
Expand All @@ -159,8 +159,10 @@ unsafe extern "C" fn release_callback<T: FileOperations>(
_inode: *mut bindings::inode,
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 _), &FileRef::from_ptr(file));
let ptr = mem::replace(unsafe { &mut (*file).private_data }, ptr::null_mut());
T::release(unsafe { T::Wrapper::from_pointer(ptr as _) }, unsafe {
&FileRef::from_ptr(file)
});
0
}

Expand All @@ -176,8 +178,8 @@ unsafe extern "C" fn llseek_callback<T: FileOperations>(
bindings::SEEK_END => SeekFrom::End(offset),
_ => return Err(Error::EINVAL),
};
let f = &*((*file).private_data as *const T);
let off = f.seek(&FileRef::from_ptr(file), off)?;
let f = unsafe { &*((*file).private_data as *const T) };
let off = f.seek(unsafe { &FileRef::from_ptr(file) }, off)?;
Ok(off as bindings::loff_t)
}
}
Expand All @@ -188,10 +190,10 @@ unsafe extern "C" fn unlocked_ioctl_callback<T: FileOperations>(
arg: c_types::c_ulong,
) -> c_types::c_long {
from_kernel_result! {
let f = &*((*file).private_data as *const T);
let f = unsafe { &*((*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(&FileRef::from_ptr(file), &mut cmd)?;
let ret = f.ioctl(unsafe { &FileRef::from_ptr(file) }, &mut cmd)?;
Ok(ret as _)
}
}
Expand All @@ -202,10 +204,10 @@ unsafe extern "C" fn compat_ioctl_callback<T: FileOperations>(
arg: c_types::c_ulong,
) -> c_types::c_long {
from_kernel_result! {
let f = &*((*file).private_data as *const T);
let f = unsafe { &*((*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(&FileRef::from_ptr(file), &mut cmd)?;
let ret = f.compat_ioctl(unsafe { &FileRef::from_ptr(file) }, &mut cmd)?;
Ok(ret as _)
}
}
Expand All @@ -215,8 +217,8 @@ unsafe extern "C" fn mmap_callback<T: FileOperations>(
vma: *mut bindings::vm_area_struct,
) -> c_types::c_int {
from_kernel_result! {
let f = &*((*file).private_data as *const T);
f.mmap(&FileRef::from_ptr(file), &mut *vma)?;
let f = unsafe { &*((*file).private_data as *const T) };
f.mmap(unsafe { &FileRef::from_ptr(file) }, unsafe { &mut *vma })?;
Ok(0)
}
}
Expand All @@ -231,8 +233,8 @@ unsafe extern "C" fn fsync_callback<T: FileOperations>(
let start = start.try_into()?;
let end = end.try_into()?;
let datasync = datasync != 0;
let f = &*((*file).private_data as *const T);
let res = f.fsync(&FileRef::from_ptr(file), start, end, datasync)?;
let f = unsafe { &*((*file).private_data as *const T) };
let res = f.fsync(unsafe { &FileRef::from_ptr(file) }, start, end, datasync)?;
Ok(res.try_into().unwrap())
}
}
Expand All @@ -241,8 +243,10 @@ unsafe extern "C" fn poll_callback<T: FileOperations>(
file: *mut bindings::file,
wait: *mut bindings::poll_table_struct,
) -> bindings::__poll_t {
let f = &*((*file).private_data as *const T);
match f.poll(&FileRef::from_ptr(file), &PollTable::from_ptr(wait)) {
let f = unsafe { &*((*file).private_data as *const T) };
match f.poll(unsafe { &FileRef::from_ptr(file) }, unsafe {
&PollTable::from_ptr(wait)
}) {
Ok(v) => v,
Err(_) => bindings::POLLERR,
}
Expand Down
4 changes: 2 additions & 2 deletions rust/kernel/iov_iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ impl IoBufferWriter for IovIter {
}

unsafe fn write_raw(&mut self, data: *const u8, len: usize) -> Result {
let res = rust_helper_copy_to_iter(data as _, len, self.ptr);
let res = unsafe { rust_helper_copy_to_iter(data as _, len, self.ptr) };
if res != len {
Err(Error::EFAULT)
} else {
Expand All @@ -85,7 +85,7 @@ impl IoBufferReader for IovIter {
}

unsafe fn read_raw(&mut self, out: *mut u8, len: usize) -> Result {
let res = rust_helper_copy_from_iter(out as _, len, self.ptr);
let res = unsafe { rust_helper_copy_from_iter(out as _, len, self.ptr) };
if res != len {
Err(Error::EFAULT)
} else {
Expand Down
2 changes: 1 addition & 1 deletion rust/kernel/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ macro_rules! offset_of {
macro_rules! container_of {
($ptr:expr, $type:ty, $($f:tt)*) => {{
let offset = $crate::offset_of!($type, $($f)*);
($ptr as *const _ as *const u8).offset(-offset) as *const $type
unsafe { ($ptr as *const _ as *const u8).offset(-offset) as *const $type }
}}
}

Expand Down
16 changes: 8 additions & 8 deletions rust/kernel/linked_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl<T: ?Sized> Wrapper<T> for Box<T> {
}

unsafe fn from_pointer(ptr: NonNull<T>) -> Self {
Box::from_raw(ptr.as_ptr())
unsafe { Box::from_raw(ptr.as_ptr()) }
}

fn as_ref(&self) -> &T {
Expand All @@ -47,7 +47,7 @@ impl<T: ?Sized> Wrapper<T> for Arc<T> {
}

unsafe fn from_pointer(ptr: NonNull<T>) -> Self {
Arc::from_raw(ptr.as_ptr())
unsafe { Arc::from_raw(ptr.as_ptr()) }
}

fn as_ref(&self) -> &T {
Expand All @@ -61,7 +61,7 @@ impl<T: ?Sized> Wrapper<T> for &T {
}

unsafe fn from_pointer(ptr: NonNull<T>) -> Self {
&*ptr.as_ptr()
unsafe { &*ptr.as_ptr() }
}

fn as_ref(&self) -> &T {
Expand Down Expand Up @@ -149,10 +149,10 @@ impl<G: GetLinksWrapped> List<G> {
/// Callers must ensure that `existing` points to a valid entry that is on the list.
pub unsafe fn insert_after(&mut self, existing: NonNull<G::EntryType>, data: G::Wrapped) {
let ptr = data.into_pointer();
let entry = &*existing.as_ptr();
if !self.list.insert_after(entry, ptr.as_ref()) {
let entry = unsafe { &*existing.as_ptr() };
if unsafe { !self.list.insert_after(entry, ptr.as_ref()) } {
// If insertion failed, rebuild object so that it can be freed.
G::Wrapped::from_pointer(ptr);
unsafe { G::Wrapped::from_pointer(ptr) };
}
}

Expand All @@ -164,8 +164,8 @@ impl<G: GetLinksWrapped> List<G> {
/// list leads to memory unsafety.
pub unsafe fn remove(&mut self, data: &G::Wrapped) -> Option<G::Wrapped> {
let entry_ref = Wrapper::as_ref(data);
if self.list.remove(entry_ref) {
Some(G::Wrapped::from_pointer(NonNull::from(entry_ref)))
if unsafe { self.list.remove(entry_ref) } {
Some(unsafe { G::Wrapped::from_pointer(NonNull::from(entry_ref)) })
} else {
None
}
Expand Down
5 changes: 4 additions & 1 deletion rust/kernel/miscdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,11 @@ impl<T: Sync> FileOpenAdapter for Registration<T> {
type Arg = T;

unsafe fn convert(_inode: *mut bindings::inode, file: *mut bindings::file) -> *const Self::Arg {
// TODO: `SAFETY` comment required here even if `unsafe` is not present,
// because `container_of!` hides it. Ideally we would not allow
// `unsafe` code as parameters to macros.
let reg = crate::container_of!((*file).private_data, Self, mdev);
Copy link
Member

Choose a reason for hiding this comment

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

The macro could introduce a temp outside of the unsafe block that stores the value casted to a raw ptr. Ideally no $foo:expr happen inside an unsafe block for macros.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1, opening an issue.

Copy link
Member Author

@ojeda ojeda Jun 4, 2021

Choose a reason for hiding this comment

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

I guess this could also be a lint with some logic like:

  • If inside an unsafe fn...
  • And there is a macro call outside an unsafe block...
  • And unsafe_op_in_unsafe_fn is enabled in this context...
  • And if the node/AST/tree of a parameter appears as-is...
  • Then check that node/AST/tree is not inside an introduced unsafe block.

Easier said than done, and I guess I am forgetting a few things, but... :^)

If you think it makes sense, I can open an issue too.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. I believe there has been discussion previously regarding "unsafe" hygiene, but I can't remember what came of it. I think it was on internals.rust-lang.org.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it does not hurt to open it to keep track.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

&(*reg).context
unsafe { &(*reg).context }
}
}

Expand Down
12 changes: 6 additions & 6 deletions rust/kernel/module_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,12 @@ pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
let arg = if val.is_null() {
None
} else {
Some(CStr::from_char_ptr(val).as_bytes())
Some(unsafe { CStr::from_char_ptr(val).as_bytes() })
};
match Self::try_from_param_arg(arg) {
Some(new_value) => {
let old_value = (*param).__bindgen_anon_1.arg as *mut Self;
let _ = core::ptr::replace(old_value, new_value);
let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut Self };
let _ = unsafe { core::ptr::replace(old_value, new_value) };
0
}
None => crate::error::Error::EINVAL.to_kernel_errno(),
Expand All @@ -95,9 +95,9 @@ pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
buf: *mut crate::c_types::c_char,
param: *const crate::bindings::kernel_param,
) -> crate::c_types::c_int {
let slice = core::slice::from_raw_parts_mut(buf as *mut u8, crate::PAGE_SIZE);
let slice = unsafe { core::slice::from_raw_parts_mut(buf as *mut u8, crate::PAGE_SIZE) };
let mut buf = crate::buffer::Buffer::new(slice);
match write!(buf, "{}\0", *((*param).__bindgen_anon_1.arg as *mut Self)) {
match unsafe { write!(buf, "{}\0", *((*param).__bindgen_anon_1.arg as *mut Self)) } {
Err(_) => crate::error::Error::EINVAL.to_kernel_errno(),
Ok(()) => buf.bytes_written() as crate::c_types::c_int,
}
Expand All @@ -111,7 +111,7 @@ pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
///
/// The `arg` field of `param` must be an instance of `Self`.
unsafe extern "C" fn free(arg: *mut crate::c_types::c_void) {
core::ptr::drop_in_place(arg as *mut Self);
unsafe { core::ptr::drop_in_place(arg as *mut Self) };
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere you may want to move the ; inside the unsafe block. While this doesn't change behavior as all cases are for () returning expressions I think, it does look nicer IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, now that you mention it, I agree.

Does anyone know if there a way to tell rustfmt to look for this? Or in Clippy perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

I can't find a clippy lint for this. Makes sense to have as a clippy lint though IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}

Expand Down
2 changes: 1 addition & 1 deletion rust/kernel/of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,6 @@ impl PointerWrapper for OfMatchTable {
}

unsafe fn from_pointer(p: *const c_types::c_void) -> Self {
Self(InnerTable::from_pointer(p))
Self(unsafe { InnerTable::from_pointer(p) })
}
}
4 changes: 2 additions & 2 deletions rust/kernel/pages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl<const ORDER: u32> Pages<ORDER> {
}

let mapping = self.kmap(0).ok_or(Error::EINVAL)?;
ptr::copy((mapping.ptr as *mut u8).add(offset), dest, len);
unsafe { ptr::copy((mapping.ptr as *mut u8).add(offset), dest, len) };
Ok(())
}

Expand All @@ -127,7 +127,7 @@ impl<const ORDER: u32> Pages<ORDER> {
}

let mapping = self.kmap(0).ok_or(Error::EINVAL)?;
ptr::copy(src, (mapping.ptr as *mut u8).add(offset), len);
unsafe { ptr::copy(src, (mapping.ptr as *mut u8).add(offset), len) };
Ok(())
}

Expand Down
Loading