Re: [RFC PATCH 06/19] rust: fs: introduce `FileSystem::init_root`

[Wedson Almeida Filho <wedsonaf@gmail.com>]

On Wed, Jan 03, 2024 at 02:29:33PM +0100, Andreas Hindborg (Samsung) wrote:
> 
> Wedson Almeida Filho <wedsonaf@gmail.com> writes:
> 
> [...]
> 
> >  
> > +/// An inode that is locked and hasn't been initialised yet.
> > +#[repr(transparent)]
> > +pub struct NewINode<T: FileSystem + ?Sized>(ARef<INode<T>>);
> > +
> > +impl<T: FileSystem + ?Sized> NewINode<T> {
> > +    /// Initialises the new inode with the given parameters.
> > +    pub fn init(self, params: INodeParams) -> Result<ARef<INode<T>>> {
> > +        // SAFETY: This is a new inode, so it's safe to manipulate it mutably.
> > +        let inode = unsafe { &mut *self.0 .0.get() };
> 
> Perhaps it would make sense with a `UniqueARef` that guarantees
> uniqueness, in line with `alloc::UniqueRc`?

We do have something like that in the kernel crate for Rust-allocated
ref-counted memory, namely, UniqueArc.

But in this case, this is slightly different: the ref-count may be >1, it's just
that the other holders of pointers will refrain from accessing the object (for
some unspecified reason). We do have another case like this for folios. Perhaps
it does make sense to generalise the concept with a type; I'll look into this.

> 
> [...]
> 
> >  
> > +impl<T: FileSystem + ?Sized> SuperBlock<T> {
> > +    /// Tries to get an existing inode or create a new one if it doesn't exist yet.
> > +    pub fn get_or_create_inode(&self, ino: Ino) -> Result<Either<ARef<INode<T>>, NewINode<T>>> {
> > +        // SAFETY: The only initialisation missing from the superblock is the root, and this
> > +        // function is needed to create the root, so it's safe to call it.
> > +        let inode =
> > +            ptr::NonNull::new(unsafe { bindings::iget_locked(self.0.get(), ino) }).ok_or(ENOMEM)?;
> 
> I can't parse this safety comment properly.

Fixed in v2.

> > +
> > +        // SAFETY: `inode` is valid for read, but there could be concurrent writers (e.g., if it's
> > +        // an already-initialised inode), so we use `read_volatile` to read its current state.
> > +        let state = unsafe { ptr::read_volatile(ptr::addr_of!((*inode.as_ptr()).i_state)) };
> > +        if state & u64::from(bindings::I_NEW) == 0 {
> > +            // The inode is cached. Just return it.
> > +            //
> > +            // SAFETY: `inode` had its refcount incremented by `iget_locked`; this increment is now
> > +            // owned by `ARef`.
> > +            Ok(Either::Left(unsafe { ARef::from_raw(inode.cast()) }))
> > +        } else {
> > +            // SAFETY: The new inode is valid but not fully initialised yet, so it's ok to create a
> > +            // `NewINode`.
> > +            Ok(Either::Right(NewINode(unsafe {
> > +                ARef::from_raw(inode.cast())
> 
> I would suggest making the destination type explicit for the cast.

Done in v2.

> 
> > +            })))
> > +        }
> > +    }
> > +}
> > +
> >  /// Required superblock parameters.
> >  ///
> >  /// This is returned by implementations of [`FileSystem::super_params`].
> > @@ -215,41 +345,28 @@ impl<T: FileSystem + ?Sized> Tables<T> {
> >              sb.0.s_blocksize = 1 << sb.0.s_blocksize_bits;
> >              sb.0.s_flags |= bindings::SB_RDONLY;
> >  
> > -            // The following is scaffolding code that will be removed in a subsequent patch. It is
> > -            // needed to build a root dentry, otherwise core code will BUG().
> > -            // SAFETY: `sb` is the superblock being initialised, it is valid for read and write.
> > -            let inode = unsafe { bindings::new_inode(&mut sb.0) };
> > -            if inode.is_null() {
> > -                return Err(ENOMEM);
> > -            }
> > -
> > -            // SAFETY: `inode` is valid for write.
> > -            unsafe { bindings::set_nlink(inode, 2) };
> > -
> > -            {
> > -                // SAFETY: This is a newly-created inode. No other references to it exist, so it is
> > -                // safe to mutably dereference it.
> > -                let inode = unsafe { &mut *inode };
> > -                inode.i_ino = 1;
> > -                inode.i_mode = (bindings::S_IFDIR | 0o755) as _;
> > -
> > -                // SAFETY: `simple_dir_operations` never changes, it's safe to reference it.
> > -                inode.__bindgen_anon_3.i_fop = unsafe { &bindings::simple_dir_operations };
> > +            // SAFETY: The callback contract guarantees that `sb_ptr` is a unique pointer to a
> > +            // newly-created (and initialised above) superblock.
> > +            let sb = unsafe { &mut *sb_ptr.cast() };
> 
> Again, I would suggest an explicit destination type for the cast.

Done in v2.

> 
> > +            let root = T::init_root(sb)?;
> >  
> > -                // SAFETY: `simple_dir_inode_operations` never changes, it's safe to reference it.
> > -                inode.i_op = unsafe { &bindings::simple_dir_inode_operations };
> > +            // Reject root inode if it belongs to a different superblock.
> 
> I am curious how this would happen?

If a user mounts two instances of a file system and the implementation allocates
root inodes and swap them before returning. The types will match because they
are the same file system, but they'll have the wrong super-block.

Thanks,
-Wedson