From 001a2cb3b916bbe3a1b351d4e2e8a940103578e8 Mon Sep 17 00:00:00 2001 From: "eryu.gey" Date: Mon, 7 Sep 2020 13:46:53 +0800 Subject: [PATCH 1/7] server: trim extra '\0's in bytes_to_cstr() Currently bytes_to_cstr() expects one and only one '\0' and the end of the buffer, but we've seen lookup request with name buffer having multiple '\0's at the end and caused failures. To make it more robust, trim extra zeros in buffer, this also follows what virtiofsd does. Signed-off-by: Eryu Guan --- src/api/server.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/api/server.rs b/src/api/server.rs index c690597c0..a9b320e5c 100644 --- a/src/api/server.rs +++ b/src/api/server.rs @@ -1395,9 +1395,21 @@ fn reply_error(err: io::Error, unique: u64, mut w: Writer) -> Result { } fn bytes_to_cstr(buf: &[u8]) -> Result<&CStr> { - // Convert to a `CStr` first so that we can drop the '\0' byte at the end - // and make sure there are no interior '\0' bytes. - CStr::from_bytes_with_nul(buf).map_err(Error::InvalidCString) + // There might be multiple 0s at the end of buf, find & use the first one and trim other zeros. + let len = buf.len(); + let mut n: usize = 0; + for c in buf.iter() { + if c == &0 { + break; + } + n += 1; + } + let nul_pos = if n + 1 >= len { len } else { n + 1 }; + let newbuf = &buf[0..nul_pos]; + + // Convert to a `CStr` so that we can drop the '\0' byte at the end and make sure there are no + // interior '\0' bytes. + CStr::from_bytes_with_nul(newbuf).map_err(Error::InvalidCString) } fn add_dirent( From db715a435701ced7418ed2d801de201c0b31c505 Mon Sep 17 00:00:00 2001 From: "chuanlang.gcw" Date: Thu, 10 Sep 2020 01:27:45 +0800 Subject: [PATCH 2/7] fusedev: commit() no longer returns IoError Writing fuse replies back to kernel should always return `EncodeMessage` with errors. So writing fuse replies will return unique error code hinting an error happens when replies. Signed-off-by: Changwei Ge --- src/api/server.rs | 4 ++-- src/lib.rs | 3 --- src/transport/fusedev/mod.rs | 4 +++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/api/server.rs b/src/api/server.rs index a9b320e5c..a9ee89820 100644 --- a/src/api/server.rs +++ b/src/api/server.rs @@ -571,7 +571,7 @@ impl Server { }; w.write_all(out.as_slice()).map_err(Error::EncodeMessage)?; - w.commit(&[&data_writer.0]).map_err(Error::IoError)?; + w.commit(&[&data_writer.0]).map_err(Error::EncodeMessage)?; Ok(out.len as usize) } Err(e) => reply_error(e, in_header.unique, w), @@ -976,7 +976,7 @@ impl Server { }; w.write_all(out.as_slice()).map_err(Error::EncodeMessage)?; - w.commit(&[&cursor]).map_err(Error::IoError)?; + w.commit(&[&cursor]).map_err(Error::EncodeMessage)?; Ok(out.len as usize) } } diff --git a/src/lib.rs b/src/lib.rs index 96dedc124..23780517d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,8 +75,6 @@ pub enum Error { /// The `size` field of the `SetxattrIn` message does not match the length /// of the decoded value. InvalidXattrSize((u32, usize)), - /// An IO related error has happened. - IoError(io::Error), } impl error::Error for Error {} @@ -96,7 +94,6 @@ impl fmt::Display for Error { decoded value: size = {}, value.len() = {}", size, len ), - IoError(err) => write!(f, "fail to handle request: {}", err), } } } diff --git a/src/transport/fusedev/mod.rs b/src/transport/fusedev/mod.rs index c7f616ae8..bdf1f49ec 100644 --- a/src/transport/fusedev/mod.rs +++ b/src/transport/fusedev/mod.rs @@ -163,7 +163,9 @@ impl<'a> Writer<'a> { if !buf.is_empty() { return writev(self.fd, buf.as_slice()).map_err(|e| { error! {"fail to write to fuse device on commit: {}", e}; - io::Error::new(io::ErrorKind::Other, format!("{}", e)) + e.as_errno() + .and_then(|r| Some(io::Error::from_raw_os_error(r as i32))) + .unwrap_or_else(|| io::Error::new(io::ErrorKind::Other, format!("{}", e))) }); } Ok(0) From 38a0d87c456dd346d5c628bf0a213a2ac4674b39 Mon Sep 17 00:00:00 2001 From: "eryu.gey" Date: Tue, 15 Sep 2020 11:07:35 +0800 Subject: [PATCH 3/7] passthroughfs: support "none" as cache policy virtiofsd uses "none" as one of the cache policies, but passthroughfs uses "never". Support "none" as well to be compatible with virtiofsd's configuration. Signed-off-by: Eryu Guan --- src/passthrough/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passthrough/fs.rs b/src/passthrough/fs.rs index 877a5f146..fed59b2f2 100644 --- a/src/passthrough/fs.rs +++ b/src/passthrough/fs.rs @@ -199,7 +199,7 @@ impl FromStr for CachePolicy { fn from_str(s: &str) -> Result { match s { - "never" | "Never" | "NEVER" => Ok(CachePolicy::Never), + "never" | "Never" | "NEVER" | "none" | "None" | "NONE" => Ok(CachePolicy::Never), "auto" | "Auto" | "AUTO" => Ok(CachePolicy::Auto), "always" | "Always" | "ALWAYS" => Ok(CachePolicy::Always), _ => Err("invalid cache policy"), From bb93d85f85eded19e4e77630a8567dd2c629bfef Mon Sep 17 00:00:00 2001 From: Liu Bo Date: Thu, 10 Sep 2020 17:47:46 -0700 Subject: [PATCH 4/7] Fix readdirplus to send available Dirent on error If there's an error, we can only signal it if we haven't stored any entries yet - otherwise we'd end up with wrong lookup counts for the entries that are already in the buffer. So we return what we've collected until that point. Signed-off-by: Liu Bo --- src/passthrough/fs.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/passthrough/fs.rs b/src/passthrough/fs.rs index fed59b2f2..bc8916caf 100644 --- a/src/passthrough/fs.rs +++ b/src/passthrough/fs.rs @@ -561,6 +561,7 @@ impl PassthroughFs { } let mut rem = &buf[..]; + let orig_rem_len = rem.len(); while !rem.is_empty() { // We only use debug asserts here because these values are coming from the kernel and we // trust them implicitly. @@ -599,7 +600,12 @@ impl PassthroughFs { match res { Ok(0) => break, Ok(_) => rem = &rem[dirent64.d_reclen as usize..], - Err(e) => return Err(e), + // If there's an error, we can only signal it if we haven't + // stored any entries yet - otherwise we'd end up with wrong + // lookup counts for the entries that are already in the + // buffer. So we return what we've collected until that point. + Err(e) if rem.len() == orig_rem_len => return Err(e), + Err(_) => return Ok(()), } } From 817d6dfc497425e75d2703ffb33186eb351783e0 Mon Sep 17 00:00:00 2001 From: Liu Bo Date: Wed, 21 Oct 2020 08:21:50 +0800 Subject: [PATCH 5/7] passthroughfs: avoid extra lookup after readdirplus The Sys_getdents64 in kernel will pad the name with '\0' bytes up to 8-byte alignment, so @name of LinuxDirent64 may contain a few null terminators. This causes an extra lookup from fuse kernel when called by readdirplus, because kernel path walking only takes name without null terminators, the dentry with more than 1 null terminators added by readdirplus doesn't satisfy the path walking so that kernel has to issue another lookup to get a valid dentry. Signed-off-by: Liu Bo --- src/api/server.rs | 3 ++- src/passthrough/fs.rs | 30 +++++++++++++++++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/api/server.rs b/src/api/server.rs index a9ee89820..daff41b4b 100644 --- a/src/api/server.rs +++ b/src/api/server.rs @@ -1394,7 +1394,8 @@ fn reply_error(err: io::Error, unique: u64, mut w: Writer) -> Result { .map_err(Error::EncodeMessage) } -fn bytes_to_cstr(buf: &[u8]) -> Result<&CStr> { +/// trim all trailing nul terminators. +pub fn bytes_to_cstr(buf: &[u8]) -> Result<&CStr> { // There might be multiple 0s at the end of buf, find & use the first one and trim other zeros. let len = buf.len(); let mut n: usize = 0; diff --git a/src/passthrough/fs.rs b/src/passthrough/fs.rs index bc8916caf..997353a5f 100644 --- a/src/passthrough/fs.rs +++ b/src/passthrough/fs.rs @@ -28,6 +28,7 @@ use crate::api::filesystem::{ Context, DirEntry, Entry, FileSystem, FsOptions, GetxattrReply, ListxattrReply, OpenOptions, SetattrValid, ZeroCopyReader, ZeroCopyWriter, }; +use crate::api::server; use crate::api::{BackendFileSystem, VFS_MAX_INO}; #[cfg(feature = "vhost-user-fs")] use crate::transport::FsCacheReqHandler; @@ -584,6 +585,20 @@ impl PassthroughFs { // break the loop so return `Ok` with a non-zero value instead. Ok(1) } else { + // The Sys_getdents64 in kernel will pad the name with '\0' + // bytes up to 8-byte alignment, so @name may contain a few null + // terminators. This causes an extra lookup from fuse when + // called by readdirplus, because kernel path walking only takes + // name without null terminators, the dentry with more than 1 + // null terminators added by readdirplus doesn't satisfy the + // path walking. + let name = server::bytes_to_cstr(name) + .map_err(|e| { + error!("do_readdir: {:?}", e); + io::Error::from_raw_os_error(libc::EINVAL) + })? + .to_bytes(); + add_entry(DirEntry { ino: dirent64.d_ino, offset: dirent64.d_off as u64, @@ -908,13 +923,14 @@ impl FileSystem for PassthroughFs { add_entry: &mut dyn FnMut(DirEntry, Entry) -> io::Result, ) -> io::Result<()> { self.do_readdir(inode, handle, size, offset, &mut |dir_entry| { - // Safe because the kernel guarantees that the buffer is nul-terminated. Additionally, - // the kernel will pad the name with '\0' bytes up to 8-byte alignment and there's no - // way for us to know exactly how many padding bytes there are. This would cause - // `CStr::from_bytes_with_nul` to return an error because it would think there are - // interior '\0' bytes. We trust the kernel to provide us with properly formatted data - // so we'll just skip the checks here. - let name = unsafe { CStr::from_bytes_with_nul_unchecked(dir_entry.name) }; + // Safe because do_readdir() has ensured dir_entry.name is a + // valid [u8] generated by CStr::to_bytes(). + let name = unsafe { + CStr::from_bytes_with_nul_unchecked(std::slice::from_raw_parts( + &dir_entry.name[0], + dir_entry.name.len() + 1, + )) + }; let entry = self.do_lookup(inode, name)?; let ino = entry.inode; From 126cf6b377fbed05a027eed3c05c6d1160436573 Mon Sep 17 00:00:00 2001 From: Changwei Ge Date: Mon, 21 Sep 2020 15:33:13 +0800 Subject: [PATCH 6/7] build: silent cllipy complaints. cargo clippy --features=fusedev --target-dir target-fusedev -- -Dclippy::all Checking fuse-rs v0.1.0 (/root/git_repo/fuse-backend-rs) error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)` --> /root/git_repo/fuse-backend-rs/src/transport/fusedev/mod.rs:166:17 | 166 | / e.as_errno() 167 | | .and_then(|r| Some(io::Error::from_raw_os_error(r as i32))) | |_______________________________________________________________________________^ help: try this: `e.as_errno().map(|r| io::Error::from_raw_os_error(r as i32))` | = note: `-D clippy::bind-instead-of-map` implied by `-D clippy::all` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map Signed-off-by: Changwei Ge --- src/transport/fusedev/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/fusedev/mod.rs b/src/transport/fusedev/mod.rs index bdf1f49ec..f99a4d420 100644 --- a/src/transport/fusedev/mod.rs +++ b/src/transport/fusedev/mod.rs @@ -164,7 +164,7 @@ impl<'a> Writer<'a> { return writev(self.fd, buf.as_slice()).map_err(|e| { error! {"fail to write to fuse device on commit: {}", e}; e.as_errno() - .and_then(|r| Some(io::Error::from_raw_os_error(r as i32))) + .map(|r| io::Error::from_raw_os_error(r as i32)) .unwrap_or_else(|| io::Error::new(io::ErrorKind::Other, format!("{}", e))) }); } From 013b48ffd8a3db124eddcda07637f65da4af110e Mon Sep 17 00:00:00 2001 From: Liu Bo Date: Wed, 21 Oct 2020 16:15:22 -0700 Subject: [PATCH 7/7] fix clippy errors Remove some unnecessary map() calls. Signed-off-by: Liu Bo --- src/api/vfs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api/vfs.rs b/src/api/vfs.rs index 1c616e5ed..b59ed6ec5 100644 --- a/src/api/vfs.rs +++ b/src/api/vfs.rs @@ -431,7 +431,7 @@ impl FileSystem for Vfs { ) -> Result<(libc::stat64, Duration)> { match self.get_real_rootfs(inode)? { (Left(fs), idata) => fs.getattr(ctx, idata.ino, handle), - (Right(fs), idata) => fs.getattr(ctx, idata.ino, handle.map(|h| h)), + (Right(fs), idata) => fs.getattr(ctx, idata.ino, handle), } } @@ -445,7 +445,7 @@ impl FileSystem for Vfs { ) -> Result<(libc::stat64, Duration)> { match self.get_real_rootfs(inode)? { (Left(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle, valid), - (Right(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle.map(|h| h), valid), + (Right(fs), idata) => fs.setattr(ctx, idata.ino, attr, handle, valid), } } @@ -587,7 +587,7 @@ impl FileSystem for Vfs { fs.create(ctx, idata.ino, name, mode, flags, umask) .map(|(mut a, b, c)| { a.inode = self.hash_inode(idata.super_index, a.inode)?; - Ok((a, b.map(|h| h), c)) + Ok((a, b, c)) })? } }