From e24ee555953de55a11eb4aa4877306712581e298 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Wed, 27 Jan 2021 12:43:29 +0000 Subject: [PATCH 1/6] Fix Pipe API. --- kernel-rs/src/file.rs | 2 +- kernel-rs/src/pipe.rs | 118 ++++++++++++++++++++++++------------------ 2 files changed, 68 insertions(+), 52 deletions(-) diff --git a/kernel-rs/src/file.rs b/kernel-rs/src/file.rs index f1079885c..79ceae8f8 100644 --- a/kernel-rs/src/file.rs +++ b/kernel-rs/src/file.rs @@ -174,7 +174,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 7dd49e0a4..3f42d9a5c 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}; use static_assertions::const_assert; const PIPESIZE: usize = 512; @@ -41,8 +41,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 unsafe { inner.try_read(addr, n) } { @@ -62,7 +62,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 { @@ -85,7 +85,7 @@ impl Pipe { } } - unsafe fn close(&self, writable: bool) -> bool { + fn close(&self, writable: bool) -> bool { let mut inner = self.inner.lock(); if writable { @@ -96,73 +96,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 unsafe { (*self.ptr).close(writable) } { - kernel().free(unsafe { 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 _)); + } } } } @@ -174,13 +174,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 = [0u8]; - let proc = unsafe { myproc() }; - if !self.readopen || unsafe { (*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 = unsafe { &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 @@ -195,18 +204,25 @@ impl PipeInner { Ok(n) } - unsafe fn try_read(&mut self, addr: UVAddr, n: usize) -> Result { - let proc = unsafe { myproc() }; - let data = unsafe { &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 unsafe { (*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 { From 3403d1b352cd1a86bbda6374e2d101bc547b9790 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 28 Jan 2021 05:19:37 +0000 Subject: [PATCH 2/6] Add comments about safety. --- kernel-rs/src/pipe.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 3f42d9a5c..c3a2ae1ee 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -104,6 +104,10 @@ impl Pipe { /// # Safety /// /// `ptr` always refers to a `Pipe`. +/// Also, for a single `Pipe`, we have a single read-only `AllocatedPipe` and a single write-only `AllocatedPipe`. +/// The `PipeInner`'s readopen/writeopen field denotes whether the read-only/write-only `AllocatedPipe` is still open, +/// and hence, we can safely free the `Pipe` only after both the readopen/writeopen field is false, since this means +/// all `AllocatedPipe`s were closed. pub struct AllocatedPipe { ptr: NonNull, } @@ -119,7 +123,10 @@ impl Deref for AllocatedPipe { impl AllocatedPipe { pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> { let page = kernel().alloc().ok_or(())?; - let mut ptr = NonNull::new(page.into_usize() as *mut Pipe).expect("AllocatedPipe alloc"); + let mut ptr = unsafe { + // Safe since by the invariant of `Page`, `page` is always non-null. + NonNull::new_unchecked(page.into_usize() as *mut Pipe) + }; // `Pipe` must be aligned with `Page`. const_assert!(mem::size_of::() <= PGSIZE); @@ -161,6 +168,8 @@ impl AllocatedPipe { unsafe { // Safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`. if self.ptr.as_ref().close(writable) { + // If `Pipe::close()` returned true, this means all `AllocatedPipe`s were closed. + // Hence, we can free the `Pipe`. kernel().free(Page::from_usize(self.ptr.as_ptr() as _)); } } From 93f3e811b498b7935fdcf8e5c4b24e399b08025a Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 28 Jan 2021 06:56:55 +0000 Subject: [PATCH 3/6] Add documentation comments. --- kernel-rs/src/pipe.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index c3a2ae1ee..0a0dc2362 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -39,9 +39,11 @@ 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. + /// Tries to read up to `n` bytes using `Pipe::try_read()`. + /// If successfully read i > 0 bytes, wakeups the `write_waitchannel` and returns `Ok(i: usize)`. + /// If the pipe was empty, sleeps at `read_waitchannel` and tries again after wakeup. + /// If an error happened, returns `Err(())`. + // 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 { @@ -60,8 +62,12 @@ impl Pipe { } } - /// PipeInner::try_write() tries to write as much as possible. - /// Pipe::write() executes try_write() until `n` bytes are written. + /// Tries to write up to `n` bytes by repeatedly calling `Pipe::try_write()`. + /// Wakeups `read_waitchannel` for every successful `Pipe::try_write()`. + /// After successfully writing i >= 0 bytes, returns `Ok(i)`. + /// Note that we may have i < `n` if an copy-in error happened. + /// If the pipe was full, sleeps at `write_waitchannel` and tries again after wakeup. + /// If an error happened, returns `Err(())`. pub fn write(&self, addr: UVAddr, n: usize) -> Result { let mut written = 0; let mut inner = self.inner.lock(); @@ -183,6 +189,10 @@ pub enum PipeError { } impl PipeInner { + /// Tries to write up to `n` bytes. + /// If the process was killed, returns `Err(InvalidStatus)`. + /// If an copy-in error happened after successfully writing i >= 0 bytes, returns `Err(InvalidCopyIn(i))`. + /// Otherwise, returns `Ok(i)` after successfully writing i >= 0 bytes. fn try_write(&mut self, addr: UVAddr, n: usize) -> Result { let mut ch = [0u8]; let proc = unsafe { @@ -213,6 +223,10 @@ impl PipeInner { Ok(n) } + /// Tries to read up to `n` bytes. + /// If successful read i > 0 bytes, returns `Ok(i: usize)`. + /// If the pipe was empty, returns `Err(WaitForIO)`. + /// If the process was killed, returns `Err(InvalidStatus)`. fn try_read(&mut self, addr: UVAddr, n: usize) -> Result { let proc = unsafe { // TODO(https://github.com/kaist-cp/rv6/issues/354) From 027eea7789a6422c6b006ae0f3c78d5e163fc998 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 28 Jan 2021 07:29:44 +0000 Subject: [PATCH 4/6] Remove TODO. --- kernel-rs/src/pipe.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 0a0dc2362..9727ef443 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -43,7 +43,6 @@ impl Pipe { /// If successfully read i > 0 bytes, wakeups the `write_waitchannel` and returns `Ok(i: usize)`. /// If the pipe was empty, sleeps at `read_waitchannel` and tries again after wakeup. /// If an error happened, returns `Err(())`. - // 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 { From 220a53d33e7a68f07c6d258a2ffaba15b0d30e53 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 28 Jan 2021 07:32:08 +0000 Subject: [PATCH 5/6] Small fix. --- kernel-rs/src/file.rs | 4 ++-- kernel-rs/src/pipe.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel-rs/src/file.rs b/kernel-rs/src/file.rs index 79ceae8f8..4fa021635 100644 --- a/kernel-rs/src/file.rs +++ b/kernel-rs/src/file.rs @@ -99,7 +99,7 @@ impl File { } match &self.typ { - FileType::Pipe { pipe } => unsafe { pipe.read(addr, usize::try_from(n).unwrap_or(0)) }, + FileType::Pipe { pipe } => pipe.read(addr, usize::try_from(n).unwrap_or(0)), FileType::Inode { ip, off } => { let mut ip = ip.deref().lock(); let curr_off = unsafe { *off.get() }; @@ -126,7 +126,7 @@ impl File { } match &self.typ { - FileType::Pipe { pipe } => unsafe { pipe.write(addr, usize::try_from(n).unwrap_or(0)) }, + FileType::Pipe { pipe } => pipe.write(addr, usize::try_from(n).unwrap_or(0)), FileType::Inode { ip, off } => { // write a few blocks at a time to avoid exceeding // the maximum log transaction size, including diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 9727ef443..295d118c3 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -46,7 +46,7 @@ impl Pipe { pub fn read(&self, addr: UVAddr, n: usize) -> Result { let mut inner = self.inner.lock(); loop { - match unsafe { inner.try_read(addr, n) } { + match inner.try_read(addr, n) { Ok(r) => { //DOC: piperead-wakeup self.write_waitchannel.wakeup(); @@ -71,7 +71,7 @@ impl Pipe { let mut written = 0; let mut inner = self.inner.lock(); loop { - match unsafe { inner.try_write(addr + written, n - written) } { + match inner.try_write(addr + written, n - written) { Ok(r) => { written += r; self.read_waitchannel.wakeup(); From 9918c32c45e6bcc6746028583d9d8cb343322412 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 28 Jan 2021 07:47:56 +0000 Subject: [PATCH 6/6] Narrow down unsafe in AllocatedPipe::close(). --- kernel-rs/src/pipe.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 295d118c3..19bdb7554 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -170,13 +170,11 @@ impl AllocatedPipe { } 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) { - // If `Pipe::close()` returned true, this means all `AllocatedPipe`s were closed. - // Hence, we can free the `Pipe`. - kernel().free(Page::from_usize(self.ptr.as_ptr() as _)); - } + if self.deref().close(writable) { + // If `Pipe::close()` returned true, this means all `AllocatedPipe`s were closed. + // Hence, we can free the `Pipe`. + // Also, the following is safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`. + kernel().free(unsafe { Page::from_usize(self.ptr.as_ptr() as _) }); } } }