diff --git a/kernel-rs/src/file.rs b/kernel-rs/src/file.rs index 3af16f1c3..005b33d38 100644 --- a/kernel-rs/src/file.rs +++ b/kernel-rs/src/file.rs @@ -176,7 +176,7 @@ impl ArenaObject for File { A::reacquire_after(guard, || { let typ = mem::replace(&mut self.typ, FileType::None); match typ { - FileType::Pipe { mut pipe } => unsafe { pipe.close(self.writable) }, + FileType::Pipe { pipe } => pipe.close(self.writable), FileType::Inode { ip, .. } | FileType::Device { ip, .. } => { // TODO(https://github.com/kaist-cp/rv6/issues/290) // The inode ip will be dropped by drop(ip). Deallocation diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index e1b9f0f4a..8b5c5c65a 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -7,7 +7,7 @@ use crate::{ spinlock::Spinlock, vm::UVAddr, }; -use core::{mem, ops::Deref, ptr}; +use core::{mem, ops::Deref, ptr::NonNull}; const PIPESIZE: usize = 512; @@ -40,8 +40,8 @@ pub struct Pipe { impl Pipe { /// PipeInner::try_read() tries to read as much as possible. /// Pipe::read() executes try_read() until all bytes in pipe are read. - //TODO(https://github.com/kaist-cp/rv6/issues/366) : `n` should be u32 - pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result { + //TODO(https://github.com/kaist-cp/rv6/issues/366) : `n` should be u32. + pub fn read(&self, addr: UVAddr, n: usize) -> Result { let mut inner = self.inner.lock(); loop { match inner.try_read(addr, n) { @@ -61,7 +61,7 @@ impl Pipe { /// PipeInner::try_write() tries to write as much as possible. /// Pipe::write() executes try_write() until `n` bytes are written. - pub unsafe fn write(&self, addr: UVAddr, n: usize) -> Result { + pub fn write(&self, addr: UVAddr, n: usize) -> Result { let mut written = 0; let mut inner = self.inner.lock(); loop { @@ -84,7 +84,7 @@ impl Pipe { } } - unsafe fn close(&self, writable: bool) -> bool { + fn close(&self, writable: bool) -> bool { let mut inner = self.inner.lock(); if writable { @@ -95,73 +95,73 @@ impl Pipe { self.write_waitchannel.wakeup(); } - // Return whether pipe would be freed or not + // Return whether pipe should be freed or not. !inner.readopen && !inner.writeopen } } +/// # Safety +/// +/// `ptr` always refers to a `Pipe`. pub struct AllocatedPipe { - ptr: *mut Pipe, + ptr: NonNull, } impl Deref for AllocatedPipe { type Target = Pipe; fn deref(&self) -> &Self::Target { - unsafe { &*self.ptr } + // Safe since `ptr` always refers to a `Pipe`. + unsafe { self.ptr.as_ref() } } } impl AllocatedPipe { pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> { let page = kernel().alloc().ok_or(())?; - let ptr = page.into_usize() as *mut Pipe; + let mut ptr = NonNull::new(page.into_usize() as *mut Pipe).expect("AllocatedPipe alloc"); - // `Pipe` must be align with `Page` + // `Pipe` must be aligned with `Page`. const_assert!(mem::size_of::() <= PGSIZE); - //TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr` - // It is safe because unique access to page is guaranteed since page is just allocated, - // and the pipe size and alignment are compatible with the page. + //TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`. unsafe { - ptr::write( - ptr, - Pipe { - inner: Spinlock::new( - "pipe", - PipeInner { - data: [0; PIPESIZE], - nwrite: 0, - nread: 0, - readopen: true, - writeopen: true, - }, - ), - read_waitchannel: WaitChannel::new(), - write_waitchannel: WaitChannel::new(), - }, - ); - }; + // Safe since `ptr` holds a valid, unique page allocated from `kernel().alloc()`, + // and the pipe size and alignment are compatible with the page. + *ptr.as_mut() = Pipe { + inner: Spinlock::new( + "pipe", + PipeInner { + data: [0; PIPESIZE], + nwrite: 0, + nread: 0, + readopen: true, + writeopen: true, + }, + ), + read_waitchannel: WaitChannel::new(), + write_waitchannel: WaitChannel::new(), + }; + } let f0 = kernel() .ftable .alloc_file(FileType::Pipe { pipe: Self { ptr } }, true, false) - // It is safe because ptr is an address of page, which obtained by alloc() - .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?; + // Safe since ptr is an address of a page obtained by alloc(). + .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr.as_ptr() as _) }))?; let f1 = kernel() .ftable .alloc_file(FileType::Pipe { pipe: Self { ptr } }, false, true) - // It is safe because ptr is an address of page, which obtained by alloc() - .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?; + // Safe since ptr is an address of a page obtained by alloc(). + .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr.as_ptr() as _) }))?; Ok((f0, f1)) } - // TODO(https://github.com/kaist-cp/rv6/issues/365): use `Drop` instead of `close` - // TODO(https://github.com/kaist-cp/rv6/issues/365): use `self` instead of `&mut self` - // `&mut self` is used because `Drop` of `File` uses AllocatedPipe inside File. - // https://github.com/kaist-cp/rv6/pull/211#discussion_r491671723 - pub unsafe fn close(&mut self, writable: bool) { - if (*self.ptr).close(writable) { - kernel().free(Page::from_usize(self.ptr as *mut Pipe as _)); + pub fn close(self, writable: bool) { + unsafe { + // Safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`. + if self.ptr.as_ref().close(writable) { + kernel().free(Page::from_usize(self.ptr.as_ptr() as _)); + } } } } @@ -173,13 +173,22 @@ pub enum PipeError { } impl PipeInner { - unsafe fn try_write(&mut self, addr: UVAddr, n: usize) -> Result { + fn try_write(&mut self, addr: UVAddr, n: usize) -> Result { let mut ch = [0 as u8]; - let proc = myproc(); - if !self.readopen || (*proc).killed() { + let proc = unsafe { + // TODO(https://github.com/kaist-cp/rv6/issues/354) + // Remove this unsafe part after resolving #354. + &*myproc() + }; + if !self.readopen || proc.killed() { return Err(PipeError::InvalidStatus); } - let data = &mut *(*proc).data.get(); + + let data = unsafe { + // TODO(https://github.com/kaist-cp/rv6/issues/354) + // Remove this unsafe part after resolving #354. + &mut *proc.data.get() + }; for i in 0..n { if self.nwrite == self.nread.wrapping_add(PIPESIZE as u32) { //DOC: pipewrite-full @@ -194,18 +203,25 @@ impl PipeInner { Ok(n) } - unsafe fn try_read(&mut self, addr: UVAddr, n: usize) -> Result { - let proc = myproc(); - let data = &mut *(*proc).data.get(); - + fn try_read(&mut self, addr: UVAddr, n: usize) -> Result { + let proc = unsafe { + // TODO(https://github.com/kaist-cp/rv6/issues/354) + // Remove this unsafe part after resolving #354. + &*myproc() + }; //DOC: pipe-empty if self.nread == self.nwrite && self.writeopen { - if (*proc).killed() { + if proc.killed() { return Err(PipeError::InvalidStatus); } return Err(PipeError::WaitForIO); } + let data = unsafe { + // TODO(https://github.com/kaist-cp/rv6/issues/354) + // Remove this unsafe part after resolving #354. + &mut *proc.data.get() + }; //DOC: piperead-copy for i in 0..n { if self.nread == self.nwrite {