| Message ID | 20231018122518.128049-6-wedsonaf@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust abstractions for VFS | expand |
On 10/18/23 14:25, Wedson Almeida Filho wrote: > + /// Returns the super-block that owns the inode. > + pub fn super_block(&self) -> &SuperBlock<T> { > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a > + // shared reference (&self) to it. > + unsafe { &*(*self.0.get()).i_sb.cast() } > + } This makes me a bit nervous. I had to look up whether this field was a pointer to a superblock, or just a superblock embedded directly in `struct inode`. It does look like it's correct as-is, but I'd feel more confident about it if it doesn't use a cast to completely ignore the type going in to the pointer cast. Could you define a `from_raw` on `SuperBlock` and change this to: unsafe { &*SuperBlock::from_raw((*self.0.get()).i_sb) } or perhaps add a type annotation like this: let i_sb: *mut super_block = unsafe { (*self.0.get()).i_sb }; i_sb.cast() Alice
Alice Ryhl <alice@ryhl.io> writes: > On 10/18/23 14:25, Wedson Almeida Filho wrote: >> + /// Returns the super-block that owns the inode. >> + pub fn super_block(&self) -> &SuperBlock<T> { >> + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a >> + // shared reference (&self) to it. >> + unsafe { &*(*self.0.get()).i_sb.cast() } >> + } > > This makes me a bit nervous. I had to look up whether this field was a pointer > to a superblock, or just a superblock embedded directly in `struct inode`. It > does look like it's correct as-is, but I'd feel more confident about it if it > doesn't use a cast to completely ignore the type going in to the pointer cast. > > Could you define a `from_raw` on `SuperBlock` and change this to: > > unsafe { &*SuperBlock::from_raw((*self.0.get()).i_sb) } > > or perhaps add a type annotation like this: > > let i_sb: *mut super_block = unsafe { (*self.0.get()).i_sb }; > i_sb.cast() I think it would also be nice to make the cast explicit: i_sb.cast::<SuperBlock<T>>() otherwise the cast is no different than `as _` with all the caveats that comes with. BR Andreas
Wedson Almeida Filho <wedsonaf@gmail.com> writes: > +/// The number of an inode. > +pub type Ino = u64; Would it be possible to use a descriptive name such as `INodeNumber`? > + /// Returns the super-block that owns the inode. > + pub fn super_block(&self) -> &SuperBlock<T> { > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a > + // shared reference (&self) to it. > + unsafe { &*(*self.0.get()).i_sb.cast() } > + } I think the safety comment should talk about the pointee rather than the pointer? "The pointee of `i_sb` is immutable, and ..." BR Andreas
On Wed, Oct 18, 2023 at 09:25:04AM -0300, Wedson Almeida Filho wrote: > From: Wedson Almeida Filho <walmeida@microsoft.com> > > Allow Rust file systems to handle typed and ref-counted inodes. > > This is in preparation for creating new inodes (for example, to create > the root inode of a new superblock), which comes in the next patch in > the series. > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> > --- > rust/helpers.c | 7 +++++++ > rust/kernel/fs.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 58 insertions(+), 2 deletions(-) > > diff --git a/rust/helpers.c b/rust/helpers.c > index 4c86fe4a7e05..fe45f8ddb31f 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -25,6 +25,7 @@ > #include <linux/build_bug.h> > #include <linux/err.h> > #include <linux/errname.h> > +#include <linux/fs.h> > #include <linux/mutex.h> > #include <linux/refcount.h> > #include <linux/sched/signal.h> > @@ -144,6 +145,12 @@ struct kunit *rust_helper_kunit_get_current_test(void) > } > EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test); > > +off_t rust_helper_i_size_read(const struct inode *inode) i_size_read returns a loff_t (aka __kernel_loff_t (aka long long)), but this returns an off_t (aka __kernel_off_t (aka long)). Won't that cause truncation issues for files larger than 4GB on some architectures? > +{ > + return i_size_read(inode); > +} > +EXPORT_SYMBOL_GPL(rust_helper_i_size_read); > + > /* > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can > * use it in contexts where Rust expects a `usize` like slice (array) indices. > diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs > index 31cf643aaded..30fa1f312f33 100644 > --- a/rust/kernel/fs.rs > +++ b/rust/kernel/fs.rs > @@ -7,9 +7,9 @@ > //! C headers: [`include/linux/fs.h`](../../include/linux/fs.h) > > use crate::error::{code::*, from_result, to_result, Error, Result}; > -use crate::types::Opaque; > +use crate::types::{AlwaysRefCounted, Opaque}; > use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule}; > -use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin}; > +use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin, ptr}; > use macros::{pin_data, pinned_drop}; > > /// Maximum size of an inode. > @@ -94,6 +94,55 @@ fn drop(self: Pin<&mut Self>) { > } > } > > +/// The number of an inode. > +pub type Ino = u64; > + > +/// A node in the file system index (inode). > +/// > +/// Wraps the kernel's `struct inode`. > +/// > +/// # Invariants > +/// > +/// Instances of this type are always ref-counted, that is, a call to `ihold` ensures that the > +/// allocation remains valid at least until the matching call to `iput`. > +#[repr(transparent)] > +pub struct INode<T: FileSystem + ?Sized>(Opaque<bindings::inode>, PhantomData<T>); > + > +impl<T: FileSystem + ?Sized> INode<T> { > + /// Returns the number of the inode. > + pub fn ino(&self) -> Ino { > + // SAFETY: `i_ino` is immutable, and `self` is guaranteed to be valid by the existence of a > + // shared reference (&self) to it. > + unsafe { (*self.0.get()).i_ino } Is "*self.0.get()" the means by which the Rust bindings get at the actual C object? (Forgive me, I've barely finished drying the primer coat on my rust-fu.) > + } > + > + /// Returns the super-block that owns the inode. > + pub fn super_block(&self) -> &SuperBlock<T> { > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a > + // shared reference (&self) to it. > + unsafe { &*(*self.0.get()).i_sb.cast() } > + } > + > + /// Returns the size of the inode contents. > + pub fn size(&self) -> i64 { I'm a little surprised I didn't see a pub type loff_t = i64 followed by this function returning a loff_t. Or maybe it would be better to define it as: struct loff_t(i64); So that dopey fs developers like me cannot so easily assign a file position (bytes) to a pgoff_t (page index) without either supplying an actual conversion operator or seeing complaints from the compiler. > + // SAFETY: `self` is guaranteed to be valid by the existence of a shared reference. > + unsafe { bindings::i_size_read(self.0.get()) } It's confusing that rust_i_size_read returns a long but on the rust side, INode::size returns an i64. --D > + } > +} > + > +// SAFETY: The type invariants guarantee that `INode` is always ref-counted. > +unsafe impl<T: FileSystem + ?Sized> AlwaysRefCounted for INode<T> { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference means that the refcount is nonzero. > + unsafe { bindings::ihold(self.0.get()) }; > + } > + > + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > + unsafe { bindings::iput(obj.cast().as_ptr()) } > + } > +} > + > /// A file system super block. > /// > /// Wraps the kernel's `struct super_block`. > -- > 2.34.1 > >
On Wed, Jan 03, 2024 at 01:54:34PM +0100, Andreas Hindborg (Samsung) wrote: > > Wedson Almeida Filho <wedsonaf@gmail.com> writes: > > > +/// The number of an inode. > > +pub type Ino = u64; > > Would it be possible to use a descriptive name such as `INodeNumber`? Filesystem programmers are lazy. Originally the term was "index node number", which was shortened to "inode number", shortened again to "inumber", and finally "ino". The Rust type name might as well mirror the C type. (There are probably greyerbeards than I who can quote even more arcane points of Unix filesystem history.) > > + /// Returns the super-block that owns the inode. > > + pub fn super_block(&self) -> &SuperBlock<T> { > > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a > > + // shared reference (&self) to it. > > + unsafe { &*(*self.0.get()).i_sb.cast() } > > + } > > I think the safety comment should talk about the pointee rather than the > pointer? "The pointee of `i_sb` is immutable, and ..." inode::i_sb (the pointer) shouldn't be reassigned to a different superblock during the lifetime of the inode; but the superblock object itself (the pointee) is very much mutable. --D > BR Andreas > >
"Darrick J. Wong" <djwong@kernel.org> writes: [...] >> > + /// Returns the super-block that owns the inode. >> > + pub fn super_block(&self) -> &SuperBlock<T> { >> > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a >> > + // shared reference (&self) to it. >> > + unsafe { &*(*self.0.get()).i_sb.cast() } >> > + } >> >> I think the safety comment should talk about the pointee rather than the >> pointer? "The pointee of `i_sb` is immutable, and ..." > > inode::i_sb (the pointer) shouldn't be reassigned to a different > superblock during the lifetime of the inode; but the superblock object > itself (the pointee) is very much mutable. Ah, I thought the comment was about why it is sound to create `&SuperBlock`, but it is referring to why it is sound to read `i_sb`. Perhaps the comment should state this? Perhaps it is also worth mentioning why it is OK to construct a shared reference from this pointer? Best regards Andreas
On 03.01.24 13:54, Andreas Hindborg (Samsung) wrote: > Wedson Almeida Filho <wedsonaf@gmail.com> writes: >> + /// Returns the super-block that owns the inode. >> + pub fn super_block(&self) -> &SuperBlock<T> { >> + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a >> + // shared reference (&self) to it. >> + unsafe { &*(*self.0.get()).i_sb.cast() } >> + } > > I think the safety comment should talk about the pointee rather than the > pointer? "The pointee of `i_sb` is immutable, and ..." I think in this case it would be a very good idea to just split the `unsafe` block into two parts. That would solve the issue of "what does this safety comment justify?".
On Thu, 4 Jan 2024 at 02:14, Darrick J. Wong <djwong@kernel.org> wrote: > > On Wed, Oct 18, 2023 at 09:25:04AM -0300, Wedson Almeida Filho wrote: > > From: Wedson Almeida Filho <walmeida@microsoft.com> > > > > Allow Rust file systems to handle typed and ref-counted inodes. > > > > This is in preparation for creating new inodes (for example, to create > > the root inode of a new superblock), which comes in the next patch in > > the series. > > > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> > > --- > > rust/helpers.c | 7 +++++++ > > rust/kernel/fs.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 58 insertions(+), 2 deletions(-) > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > index 4c86fe4a7e05..fe45f8ddb31f 100644 > > --- a/rust/helpers.c > > +++ b/rust/helpers.c > > @@ -25,6 +25,7 @@ > > #include <linux/build_bug.h> > > #include <linux/err.h> > > #include <linux/errname.h> > > +#include <linux/fs.h> > > #include <linux/mutex.h> > > #include <linux/refcount.h> > > #include <linux/sched/signal.h> > > @@ -144,6 +145,12 @@ struct kunit *rust_helper_kunit_get_current_test(void) > > } > > EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test); > > > > +off_t rust_helper_i_size_read(const struct inode *inode) > > i_size_read returns a loff_t (aka __kernel_loff_t (aka long long)), > but this returns an off_t (aka __kernel_off_t (aka long)). > > Won't that cause truncation issues for files larger than 4GB on some > architectures? This is indeed a bug, thanks for catching it! Fixed in v2. > > +{ > > + return i_size_read(inode); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_i_size_read); > > + > > /* > > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can > > * use it in contexts where Rust expects a `usize` like slice (array) indices. > > diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs > > index 31cf643aaded..30fa1f312f33 100644 > > --- a/rust/kernel/fs.rs > > +++ b/rust/kernel/fs.rs > > @@ -7,9 +7,9 @@ > > //! C headers: [`include/linux/fs.h`](../../include/linux/fs.h) > > > > use crate::error::{code::*, from_result, to_result, Error, Result}; > > -use crate::types::Opaque; > > +use crate::types::{AlwaysRefCounted, Opaque}; > > use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule}; > > -use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin}; > > +use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin, ptr}; > > use macros::{pin_data, pinned_drop}; > > > > /// Maximum size of an inode. > > @@ -94,6 +94,55 @@ fn drop(self: Pin<&mut Self>) { > > } > > } > > > > +/// The number of an inode. > > +pub type Ino = u64; > > + > > +/// A node in the file system index (inode). > > +/// > > +/// Wraps the kernel's `struct inode`. > > +/// > > +/// # Invariants > > +/// > > +/// Instances of this type are always ref-counted, that is, a call to `ihold` ensures that the > > +/// allocation remains valid at least until the matching call to `iput`. > > +#[repr(transparent)] > > +pub struct INode<T: FileSystem + ?Sized>(Opaque<bindings::inode>, PhantomData<T>); > > + > > +impl<T: FileSystem + ?Sized> INode<T> { > > + /// Returns the number of the inode. > > + pub fn ino(&self) -> Ino { > > + // SAFETY: `i_ino` is immutable, and `self` is guaranteed to be valid by the existence of a > > + // shared reference (&self) to it. > > + unsafe { (*self.0.get()).i_ino } > > Is "*self.0.get()" the means by which the Rust bindings get at the > actual C object? It gets a pointer to the C object. self's type is INode, which is a pair. `self.0` get the first element of the pair, which has type `Opaque<bindings::inode>`. It has a method called `get` that returns a pointer to the object it wraps. > (Forgive me, I've barely finished drying the primer coat on my rust-fu.) > > > + } > > + > > + /// Returns the super-block that owns the inode. > > + pub fn super_block(&self) -> &SuperBlock<T> { > > + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a > > + // shared reference (&self) to it. > > + unsafe { &*(*self.0.get()).i_sb.cast() } > > + } > > + > > + /// Returns the size of the inode contents. > > + pub fn size(&self) -> i64 { > > I'm a little surprised I didn't see a > > pub type loff_t = i64 > > followed by this function returning a loff_t. Or maybe it would be > better to define it as: This was suggested by Matthew Wilcox as well, which I did for v2, though I call it `Offset`. > struct loff_t(i64); > > So that dopey fs developers like me cannot so easily assign a file > position (bytes) to a pgoff_t (page index) without either supplying an > actual conversion operator or seeing complaints from the compiler. We may want to eventually do this, but for now I'm only doing the type alias. The disadvantage of doing a new type is that we lose all arithmetic operators as well, though we can redefine them by implementing the appropriate traits. Thanks, -Wedson
diff --git a/rust/helpers.c b/rust/helpers.c index 4c86fe4a7e05..fe45f8ddb31f 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -25,6 +25,7 @@ #include <linux/build_bug.h> #include <linux/err.h> #include <linux/errname.h> +#include <linux/fs.h> #include <linux/mutex.h> #include <linux/refcount.h> #include <linux/sched/signal.h> @@ -144,6 +145,12 @@ struct kunit *rust_helper_kunit_get_current_test(void) } EXPORT_SYMBOL_GPL(rust_helper_kunit_get_current_test); +off_t rust_helper_i_size_read(const struct inode *inode) +{ + return i_size_read(inode); +} +EXPORT_SYMBOL_GPL(rust_helper_i_size_read); + /* * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can * use it in contexts where Rust expects a `usize` like slice (array) indices. diff --git a/rust/kernel/fs.rs b/rust/kernel/fs.rs index 31cf643aaded..30fa1f312f33 100644 --- a/rust/kernel/fs.rs +++ b/rust/kernel/fs.rs @@ -7,9 +7,9 @@ //! C headers: [`include/linux/fs.h`](../../include/linux/fs.h) use crate::error::{code::*, from_result, to_result, Error, Result}; -use crate::types::Opaque; +use crate::types::{AlwaysRefCounted, Opaque}; use crate::{bindings, init::PinInit, str::CStr, try_pin_init, ThisModule}; -use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin}; +use core::{marker::PhantomData, marker::PhantomPinned, pin::Pin, ptr}; use macros::{pin_data, pinned_drop}; /// Maximum size of an inode. @@ -94,6 +94,55 @@ fn drop(self: Pin<&mut Self>) { } } +/// The number of an inode. +pub type Ino = u64; + +/// A node in the file system index (inode). +/// +/// Wraps the kernel's `struct inode`. +/// +/// # Invariants +/// +/// Instances of this type are always ref-counted, that is, a call to `ihold` ensures that the +/// allocation remains valid at least until the matching call to `iput`. +#[repr(transparent)] +pub struct INode<T: FileSystem + ?Sized>(Opaque<bindings::inode>, PhantomData<T>); + +impl<T: FileSystem + ?Sized> INode<T> { + /// Returns the number of the inode. + pub fn ino(&self) -> Ino { + // SAFETY: `i_ino` is immutable, and `self` is guaranteed to be valid by the existence of a + // shared reference (&self) to it. + unsafe { (*self.0.get()).i_ino } + } + + /// Returns the super-block that owns the inode. + pub fn super_block(&self) -> &SuperBlock<T> { + // SAFETY: `i_sb` is immutable, and `self` is guaranteed to be valid by the existence of a + // shared reference (&self) to it. + unsafe { &*(*self.0.get()).i_sb.cast() } + } + + /// Returns the size of the inode contents. + pub fn size(&self) -> i64 { + // SAFETY: `self` is guaranteed to be valid by the existence of a shared reference. + unsafe { bindings::i_size_read(self.0.get()) } + } +} + +// SAFETY: The type invariants guarantee that `INode` is always ref-counted. +unsafe impl<T: FileSystem + ?Sized> AlwaysRefCounted for INode<T> { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference means that the refcount is nonzero. + unsafe { bindings::ihold(self.0.get()) }; + } + + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { + // SAFETY: The safety requirements guarantee that the refcount is nonzero. + unsafe { bindings::iput(obj.cast().as_ptr()) } + } +} + /// A file system super block. /// /// Wraps the kernel's `struct super_block`.