| Message ID | 20231018122518.128049-12-wedsonaf@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust abstractions for VFS | expand |
On 23/10/18 09:25AM, Wedson Almeida Filho wrote: > From: Wedson Almeida Filho <walmeida@microsoft.com> > > Allow Rust file systems to expose xattrs associated with inodes. > `overlayfs` uses an xattr to indicate that a directory is opaque (i.e., > that lower layers should not be looked up). The planned file systems > need to support opaque directories, so they must be able to implement > this. > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/error.rs | 2 ++ > rust/kernel/fs.rs | 43 +++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 53a99ea512d1..fa754c5e85a2 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -15,6 +15,7 @@ > #include <linux/refcount.h> > #include <linux/wait.h> > #include <linux/sched.h> > +#include <linux/xattr.h> > > /* `bindgen` gets confused at certain things. */ > const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs > index 484fa7c11de1..6c167583b275 100644 > --- a/rust/kernel/error.rs > +++ b/rust/kernel/error.rs > @@ -81,6 +81,8 @@ macro_rules! declare_err { > declare_err!(EIOCBQUEUED, "iocb queued, will get completion event."); > declare_err!(ERECALLCONFLICT, "Conflict with recalled state."); > declare_err!(ENOGRACE, "NFS file lock reclaim refused."); > + declare_err!(ENODATA, "No data available."); > + declare_err!(EOPNOTSUPP, "Operation not supported on transport endpoint."); > } > > /// Generic integer kernel error. > diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs > index ee3dce87032b..adf9cbee16d2 100644 > --- a/rust/kernel/fs.rs > +++ b/rust/kernel/fs.rs > @@ -42,6 +42,14 @@ pub trait FileSystem { > > /// Reads the contents of the inode into the given folio. > fn read_folio(inode: &INode<Self>, folio: LockedFolio<'_>) -> Result; > + > + /// Reads an xattr. > + /// > + /// Returns the number of bytes written to `outbuf`. If it is too small, returns the number of > + /// bytes needs to hold the attribute. > + fn read_xattr(_inode: &INode<Self>, _name: &CStr, _outbuf: &mut [u8]) -> Result<usize> { > + Err(EOPNOTSUPP) > + } > } > > /// The types of directory entries reported by [`FileSystem::read_dir`]. > @@ -418,6 +426,7 @@ impl<T: FileSystem + ?Sized> Tables<T> { > > sb.0.s_magic = params.magic as _; > sb.0.s_op = &Tables::<T>::SUPER_BLOCK; > + sb.0.s_xattr = &Tables::<T>::XATTR_HANDLERS[0]; > sb.0.s_maxbytes = params.maxbytes; > sb.0.s_time_gran = params.time_gran; > sb.0.s_blocksize_bits = params.blocksize_bits; > @@ -487,6 +496,40 @@ impl<T: FileSystem + ?Sized> Tables<T> { > shutdown: None, > }; > > + const XATTR_HANDLERS: [*const bindings::xattr_handler; 2] = [&Self::XATTR_HANDLER, ptr::null()]; > + > + const XATTR_HANDLER: bindings::xattr_handler = bindings::xattr_handler { > + name: ptr::null(), > + prefix: crate::c_str!("").as_char_ptr(), > + flags: 0, > + list: None, > + get: Some(Self::xattr_get_callback), > + set: None, > + }; > + > + unsafe extern "C" fn xattr_get_callback( > + _handler: *const bindings::xattr_handler, > + _dentry: *mut bindings::dentry, > + inode_ptr: *mut bindings::inode, > + name: *const core::ffi::c_char, > + buffer: *mut core::ffi::c_void, > + size: usize, > + ) -> core::ffi::c_int { > + from_result(|| { > + // SAFETY: The C API guarantees that `inode_ptr` is a valid inode. > + let inode = unsafe { &*inode_ptr.cast::<INode<T>>() }; > + > + // SAFETY: The c API guarantees that `name` is a valid null-terminated string. It > + // also guarantees that it's valid for the duration of the callback. > + let name = unsafe { CStr::from_char_ptr(name) }; > + > + // SAFETY: The C API guarantees that `buffer` is at least `size` bytes in length. > + let buf = unsafe { core::slice::from_raw_parts_mut(buffer.cast(), size) }; I think this is not safe. from_raw_parts_mut's documentation says: ``` `data` must be non-null and aligned even for zero-length slices. One reason for this is that enum layout optimizations may rely on references (including slices of any length) being aligned and non-null to distinguish them from other data. You can obtain a pointer that is usable as `data` for zero-length slices using [`NonNull::dangling()`]. ``` `vfs_getxattr_alloc` explicitly calls the `get` handler with `buffer` set to NULL and `size` set to 0, in order to determine the required size for the extended attributes: ``` error = handler->get(handler, dentry, inode, name, NULL, 0); if (error < 0) return error; ``` So `buffer` is definitely NULL in the first call to the handler. When `buffer` is NULL, the first argument to `from_raw_parts_mut` should be `NonNull::dangling()`. > + let len = T::read_xattr(inode, name, buf)?; > + Ok(len.try_into()?) > + }) > + } > + > const DIR_FILE_OPERATIONS: bindings::file_operations = bindings::file_operations { > owner: ptr::null_mut(), > llseek: Some(bindings::generic_file_llseek), > -- > 2.34.1 >
On Wed, 18 Oct 2023 at 10:06, Ariel Miculas (amiculas) <amiculas@cisco.com> wrote: > I think this is not safe. from_raw_parts_mut's documentation says: > ``` > `data` must be non-null and aligned even for zero-length slices. One > reason for this is that enum layout optimizations may rely on references > (including slices of any length) being aligned and non-null to distinguish > them from other data. You can obtain a pointer that is usable as `data` > for zero-length slices using [`NonNull::dangling()`]. > ``` > > `vfs_getxattr_alloc` explicitly calls the `get` handler with `buffer` set > to NULL and `size` set to 0, in order to determine the required size for > the extended attributes: > ``` > error = handler->get(handler, dentry, inode, name, NULL, 0); > if (error < 0) > return error; > ``` > > So `buffer` is definitely NULL in the first call to the handler. > > When `buffer` is NULL, the first argument to `from_raw_parts_mut` should > be `NonNull::dangling()`. Good catch, thanks! I'll fix this for v2.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 53a99ea512d1..fa754c5e85a2 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -15,6 +15,7 @@ #include <linux/refcount.h> #include <linux/wait.h> #include <linux/sched.h> +#include <linux/xattr.h> /* `bindgen` gets confused at certain things. */ const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN; diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 484fa7c11de1..6c167583b275 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -81,6 +81,8 @@ macro_rules! declare_err { declare_err!(EIOCBQUEUED, "iocb queued, will get completion event."); declare_err!(ERECALLCONFLICT, "Conflict with recalled state."); declare_err!(ENOGRACE, "NFS file lock reclaim refused."); + declare_err!(ENODATA, "No data available."); + declare_err!(EOPNOTSUPP, "Operation not supported on transport endpoint."); } /// Generic integer kernel error. diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs index ee3dce87032b..adf9cbee16d2 100644 --- a/rust/kernel/fs.rs +++ b/rust/kernel/fs.rs @@ -42,6 +42,14 @@ pub trait FileSystem { /// Reads the contents of the inode into the given folio. fn read_folio(inode: &INode<Self>, folio: LockedFolio<'_>) -> Result; + + /// Reads an xattr. + /// + /// Returns the number of bytes written to `outbuf`. If it is too small, returns the number of + /// bytes needs to hold the attribute. + fn read_xattr(_inode: &INode<Self>, _name: &CStr, _outbuf: &mut [u8]) -> Result<usize> { + Err(EOPNOTSUPP) + } } /// The types of directory entries reported by [`FileSystem::read_dir`]. @@ -418,6 +426,7 @@ impl<T: FileSystem + ?Sized> Tables<T> { sb.0.s_magic = params.magic as _; sb.0.s_op = &Tables::<T>::SUPER_BLOCK; + sb.0.s_xattr = &Tables::<T>::XATTR_HANDLERS[0]; sb.0.s_maxbytes = params.maxbytes; sb.0.s_time_gran = params.time_gran; sb.0.s_blocksize_bits = params.blocksize_bits; @@ -487,6 +496,40 @@ impl<T: FileSystem + ?Sized> Tables<T> { shutdown: None, }; + const XATTR_HANDLERS: [*const bindings::xattr_handler; 2] = [&Self::XATTR_HANDLER, ptr::null()]; + + const XATTR_HANDLER: bindings::xattr_handler = bindings::xattr_handler { + name: ptr::null(), + prefix: crate::c_str!("").as_char_ptr(), + flags: 0, + list: None, + get: Some(Self::xattr_get_callback), + set: None, + }; + + unsafe extern "C" fn xattr_get_callback( + _handler: *const bindings::xattr_handler, + _dentry: *mut bindings::dentry, + inode_ptr: *mut bindings::inode, + name: *const core::ffi::c_char, + buffer: *mut core::ffi::c_void, + size: usize, + ) -> core::ffi::c_int { + from_result(|| { + // SAFETY: The C API guarantees that `inode_ptr` is a valid inode. + let inode = unsafe { &*inode_ptr.cast::<INode<T>>() }; + + // SAFETY: The c API guarantees that `name` is a valid null-terminated string. It + // also guarantees that it's valid for the duration of the callback. + let name = unsafe { CStr::from_char_ptr(name) }; + + // SAFETY: The C API guarantees that `buffer` is at least `size` bytes in length. + let buf = unsafe { core::slice::from_raw_parts_mut(buffer.cast(), size) }; + let len = T::read_xattr(inode, name, buf)?; + Ok(len.try_into()?) + }) + } + const DIR_FILE_OPERATIONS: bindings::file_operations = bindings::file_operations { owner: ptr::null_mut(), llseek: Some(bindings::generic_file_llseek),