Skip to content
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.

Sign up for GitHub

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

[closes #426] Apply AsBytes+FromBytes #466

Closed
wants to merge 2 commits into from
Closed

[closes #426] Apply AsBytes+FromBytes #466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9da0460
use AsBytes+FromBytes traits for safe byte conversions
coolofficials Mar 30, 2021
fe65de9
fmt
coolofficials Mar 30, 2021
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
40 changes: 40 additions & 0 deletions kernel-rs/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions kernel-rs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ array-macro = "2.0.0"
static_assertions = "1.1.0"
itertools = { version = "0.10.0", default-features = false }
pin-project = "1"
zerocopy = { version = "0.3.0", default-features = false }

# Compiler options for sysroot packages.
# Cargo currently warns following packages are not dependencies.
Expand Down
23 changes: 15 additions & 8 deletions kernel-rs/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use core::{cmp, mem};

use bitflags::bitflags;
use itertools::*;
use zerocopy::{AsBytes, FromBytes};

use crate::{
fs::Path,
Expand All @@ -22,10 +23,13 @@ const ELF_MAGIC: u32 = 0x464c457f;
const ELF_PROG_LOAD: u32 = 1;

/// File header
#[derive(Default, Clone)]
#[derive(Default, Clone, AsBytes, FromBytes)]
// It needs repr(C) because it's struct for in-disk representation
// which should follow C(=machine) representation
// https://github.com/kaist-cp/rv6/issues/52
// repr(C) is also required for AsBytes & FromBytes.
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.AsBytes.html
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.FromBytes.html
#[repr(C)]
struct ElfHdr {
/// must equal ELF_MAGIC
Expand All @@ -48,6 +52,10 @@ struct ElfHdr {

bitflags! {
/// Flag bits for ProgHdr flags
#[derive(AsBytes, FromBytes)]
// repr(C) is also required for AsBytes & FromBytes.
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.AsBytes.html
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.FromBytes.html
#[repr(C)]
struct ProgFlags: u32 {
const EXEC = 1;
Expand All @@ -63,10 +71,13 @@ impl Default for ProgFlags {
}

/// Program section header
#[derive(Default, Clone)]
#[derive(Default, Clone, AsBytes, FromBytes)]
// It needs repr(C) because it's struct for in-disk representation
// which should follow C(=machine) representation
// https://github.com/kaist-cp/rv6/issues/52
// repr(C) is also required for AsBytes & FromBytes.
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.AsBytes.html
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.FromBytes.html
#[repr(C)]
struct ProgHdr {
typ: u32,
Expand Down Expand Up @@ -113,9 +124,7 @@ impl Kernel {

// Check ELF header
let mut elf: ElfHdr = Default::default();
// SAFETY: ElfHdr can be safely transmuted to [u8; _], as it
// contains only integers, which do not have internal structures.
unsafe { ip.read_kernel(&mut elf, 0) }?;
ip.read_kernel(&mut elf, 0)?;
if !elf.is_valid() {
return Err(());
}
Expand All @@ -128,9 +137,7 @@ impl Kernel {
let off = elf.phoff + i * mem::size_of::<ProgHdr>();

let mut ph: ProgHdr = Default::default();
// SAFETY: ProgHdr can be safely transmuted to [u8; _], as it
// contains only integers, which do not have internal structures.
unsafe { ip.read_kernel(&mut ph, off as _) }?;
ip.read_kernel(&mut ph, off as _)?;
if ph.is_prog_load() {
if ph.memsz < ph.filesz || ph.vaddr % PGSIZE != 0 {
return Err(());
Expand Down
44 changes: 19 additions & 25 deletions kernel-rs/src/fs/inode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ use core::{

use array_macro::array;
use static_assertions::const_assert;
use zerocopy::{AsBytes, FromBytes};

use super::{FileName, IPB, MAXFILE, NDIRECT, NINDIRECT};
use crate::{
Expand Down Expand Up @@ -187,7 +188,12 @@ pub struct InodeGuard<'a> {
pub inode: &'a Inode,
}

#[derive(Default)]
#[derive(Default, AsBytes, FromBytes)]
// It needs repr(C) for deriving zerocopy::FromBytes trait.
// DIRSIZ should match conditions for AsBytes & FromBytes.
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.AsBytes.html
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.FromBytes.html
#[repr(C)]
pub struct Dirent {
pub inum: u16,
name: [u8; DIRSIZ],
Expand All @@ -196,9 +202,7 @@ pub struct Dirent {
impl Dirent {
fn new(ip: &mut InodeGuard<'_>, off: u32) -> Result<Dirent, ()> {
let mut dirent = Dirent::default();
// SAFETY: Dirent can be safely transmuted to [u8; _], as it
// contains only u16 and u8's, which do not have internal structures.
unsafe { ip.read_kernel(&mut dirent, off) }?;
ip.read_kernel(&mut dirent, off)?;
Ok(dirent)
}

Expand Down Expand Up @@ -409,16 +413,8 @@ impl InodeGuard<'_> {

/// Copy data into `dst` from the content of inode at offset `off`.
/// Return Ok(()) on success, Err(()) on failure.
///
/// # Safety
///
/// `T` can be safely `transmute`d to `[u8; size_of::<T>()]`.
pub unsafe fn read_kernel<T>(&mut self, dst: &mut T, off: u32) -> Result<(), ()> {
let bytes = self.read_bytes_kernel(
// SAFETY: the safety assumption of this method.
unsafe { core::slice::from_raw_parts_mut(dst as *mut _ as _, mem::size_of::<T>()) },
off,
);
pub fn read_kernel<T: AsBytes + FromBytes>(&mut self, dst: &mut T, off: u32) -> Result<(), ()> {
let bytes = self.read_bytes_kernel(dst.as_bytes_mut(), off);
if bytes == mem::size_of::<T>() {
Ok(())
} else {
Expand Down Expand Up @@ -496,14 +492,13 @@ impl InodeGuard<'_> {

/// Copy data from `src` into the inode at offset `off`.
/// Return Ok(()) on success, Err(()) on failure.
pub fn write_kernel<T>(&mut self, src: &T, off: u32, tx: &FsTransaction<'_>) -> Result<(), ()> {
let bytes = self.write_bytes_kernel(
// SAFETY: src is a valid reference to T and
// u8 does not have any internal structure.
unsafe { core::slice::from_raw_parts(src as *const _ as _, mem::size_of::<T>()) },
off,
tx,
)?;
pub fn write_kernel<T: AsBytes + FromBytes>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

FromBytes는 없어도 되지 않나요? (UserMemory::copy_out도 마찬가지)

&mut self,
src: &T,
off: u32,
tx: &FsTransaction<'_>,
) -> Result<(), ()> {
let bytes = self.write_bytes_kernel(src.as_bytes(), off, tx)?;
if bytes == mem::size_of::<T>() {
Ok(())
} else {
Expand Down Expand Up @@ -664,9 +659,8 @@ impl InodeGuard<'_> {
pub fn is_dir_empty(&mut self) -> bool {
let mut de: Dirent = Default::default();
for off in (2 * DIRENT_SIZE as u32..self.deref_inner().size).step_by(DIRENT_SIZE) {
// SAFETY: Dirent can be safely transmuted to [u8; _], as it
// contains only u16 and u8's, which do not have internal structures.
unsafe { self.read_kernel(&mut de, off) }.expect("is_dir_empty: read_kernel");
self.read_kernel(&mut de, off)
.expect("is_dir_empty: read_kernel");
if de.inum != 0 {
return false;
}
Expand Down
8 changes: 6 additions & 2 deletions kernel-rs/src/stat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
#[derive(Copy, Clone)]
#[repr(C)]
use zerocopy::{AsBytes, FromBytes};

#[derive(Copy, Clone, AsBytes, FromBytes)]
// repr(packed) is required for AsBytes.
// https://docs.rs/zerocopy/0.3.0/zerocopy/trait.AsBytes.html
#[repr(packed)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

repr(C) 이외의 memory layout을 사용하면 안 됩니다. repr(packed)를 사용한 것으로 인해 ls에서 파일 크기가 제대로 출력되지 않습니다.

$ ls
.              1 1 0
..             1 1 0
README         2 2 0
cat            2 3 0
echo           2 4 0
forktest       2 5 0
grep           2 6 0
init           2 7 0
kill           2 8 0
ln             2 9 0
ls             2 10 0
mkdir          2 11 0
rm             2 12 0
sh             2 13 0
stressfs       2 14 0
usertests      2 15 0
grind          2 16 0
wc             2 17 0
zombie         2 18 0
console        3 19 0

repr(C)를 사용하되 nlink 다음에 padding을 위한 4 바이트 크기의 필드를 넣어서 해결하거나, copy_out과 write_kernel은 trait bound를 없애고 구현에서 unsafe를 사용해야 할 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

추가로, struct에서 padding에 해당하는 부분을 읽는 것이 Rust에서 UB인지 unspecified behavior인지 확인해 보는 것이 좋을 것 같습니다.

In other words, the only cases in which reading uninitialized memory is permitted are inside unions and in "padding" (the gaps between the fields/elements of a type).

(https://doc.rust-lang.org/reference/behavior-considered-undefined.html)

The value of padding bytes when storing values in structures or unions.

(C18 Spec: Unspecified behavior)

Rust reference의 설명을 봐도 그렇고, C에서 unspecified인 것을 봐도 그렇고 아마 UB는 아닌 것 같습니다만, 정확히 아시는 분이 계시면 좋을 것 같네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

rust-lang/unsafe-code-guidelines#174
rust-lang/unsafe-code-guidelines#183

확실하지는 않지만 아마 괜찮을 것 같습니다

pub struct Stat {
/// File system's disk device
pub dev: i32,
Expand Down
3 changes: 1 addition & 2 deletions kernel-rs/src/syscall.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ pub fn fetchaddr(addr: UVAddr, proc: &mut CurrentProc<'_>) -> Result<usize, ()>
if addr.into_usize() >= proc.memory().size() || addr.into_usize() + sz > proc.memory().size() {
return Err(());
}
// SAFETY: usize does not have any internal structure.
unsafe { proc.memory_mut().copy_in(&mut ip, addr) }?;
proc.memory_mut().copy_in(&mut ip, addr)?;
Ok(ip)
}

Expand Down
26 changes: 10 additions & 16 deletions kernel-rs/src/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use core::{cmp, marker::PhantomData, mem, ops::Add, slice};

use zerocopy::{AsBytes, FromBytes};

use crate::{
fs::InodeGuard,
kernel::kernel_builder,
Expand Down Expand Up @@ -534,13 +536,8 @@ impl UserMemory {
/// Copy from kernel to user.
/// Copy from src to virtual address dstva in a given page table.
/// Return Ok(()) on success, Err(()) on error.
pub fn copy_out<T>(&mut self, dstva: UVAddr, src: &T) -> Result<(), ()> {
self.copy_out_bytes(
dstva,
// SAFETY: src is a valid reference to T and
// u8 does not have any internal structure.
unsafe { core::slice::from_raw_parts_mut(src as *const _ as _, mem::size_of::<T>()) },
)
pub fn copy_out<T: AsBytes + FromBytes>(&mut self, dstva: UVAddr, src: &T) -> Result<(), ()> {
self.copy_out_bytes(dstva, src.as_bytes())
}

/// Copy from user to kernel.
Expand All @@ -566,15 +563,12 @@ impl UserMemory {
/// Copy from user to kernel.
/// Copy to dst from virtual address srcva in a given page table.
/// Return Ok(()) on success, Err(()) on error.
///
/// # Safety
///
/// `T` can be safely `transmute`d to `[u8; size_of::<T>()]`.
pub unsafe fn copy_in<T>(&mut self, dst: &mut T, srcva: UVAddr) -> Result<(), ()> {
self.copy_in_bytes(
unsafe { core::slice::from_raw_parts_mut(dst as *mut _ as _, mem::size_of::<T>()) },
srcva,
)
pub fn copy_in<T: AsBytes + FromBytes>(
&mut self,
dst: &mut T,
srcva: UVAddr,
) -> Result<(), ()> {
self.copy_in_bytes(dst.as_bytes_mut(), srcva)
}

/// Copy a null-terminated string from user to kernel.
Expand Down