Message-ID: <20240916170801.GO2825852@ZenIV>
Date: Mon, 16 Sep 2024 18:08:01 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Yiyang Wu <toolmanp@...p.cc>
Cc: linux-erofs@...ts.ozlabs.org, rust-for-linux@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 19/24] erofs: introduce namei alternative to C

On Mon, Sep 16, 2024 at 09:56:29PM +0800, Yiyang Wu wrote:
> +/// Lookup function for dentry-inode lookup replacement.
> +#[no_mangle]
> +pub unsafe extern "C" fn erofs_lookup_rust(
> +    k_inode: NonNull<inode>,
> +    dentry: NonNull<dentry>,
> +    _flags: c_uint,
> +) -> *mut c_void {
> +    // SAFETY: We are sure that the inode is a Kernel Inode since alloc_inode is called
> +    let erofs_inode = unsafe { &*container_of!(k_inode.as_ptr(), KernelInode, k_inode) };

	Ummm...  A wrapper would be highly useful.  And the reason why
it's safe is different - your function is called only via ->i_op->lookup,
the is only one instance of inode_operations that has that ->lookup
method, and the only place where an inode gets ->i_op set to that
is erofs_fill_inode().  Which is always passed erofs_inode::vfs_inode.

> +    // SAFETY: The super_block is initialized when the erofs_alloc_sbi_rust is called.
> +    let sbi = erofs_sbi(unsafe { NonNull::new(k_inode.as_ref().i_sb).unwrap() });

	Again, that calls for a wrapper - this time not erofs-specific;
inode->i_sb is *always* non-NULL, is assign-once and always points
to live struct super_block instance at least until the call of
destroy_inode().

> +    // SAFETY: this is backed by qstr which is c representation of a valid slice.

	What is that sentence supposed to mean?  Nevermind "why is it correct"...

> +    let name = unsafe {
> +        core::str::from_utf8_unchecked(core::slice::from_raw_parts(
> +            dentry.as_ref().d_name.name,
> +            dentry.as_ref().d_name.__bindgen_anon_1.__bindgen_anon_1.len as usize,

	Is that supposed to be an example of idiomatic Rust?  I'm not
trying to be snide, but my interest here is mostly about safety of
access to VFS data structures.	And ->d_name is _very_ unpleasant in
that respect; the locking rules required for its stability are subtle
and hard to verify on manual code audit.

	Current erofs_lookup() (and your version as well) *is* indeed
safe in that respect, but the proof (from filesystem POV) is that "it's
called only as ->lookup() instance, so dentry is initially unhashed
negative and will remain such until it's passed to d_splice_alias();
until that point it is guaranteed to have ->d_name and ->d_parent stable".

	Note that once you _have_ called d_splice_alias(), you can't
count upon the ->d_name stability - or, indeed, upon ->d_name.name you've
sampled still pointing to allocated memory.

	For directory-modifying methods it's "stable, since parent is held
exclusive".  Some internal function called from different environments?
Well...  Swear, look through the call graph and see what can be proven
for each.

	Expressing that kind of fun in any kind of annotations (Rust type
system included) is not pleasant.  _Probably_ might be handled by a type
that would be a dentry pointer with annotation along the lines "->d_name
and ->d_parent of that one are stable".  Then e.g. ->lookup() would
take that thing as an argument and d_splice_alias() would consume it.
->mkdir() would get the same thing, etc.  I hadn't tried to get that
all way through (the amount of annotation churn in existing filesystems
would be high and hard to split into reviewable patch series), so there
might be dragons - and there definitely are places where the stability is
proven in different ways (e.g. if dentry->d_lock is held, we have the damn
thing stable; then there's a "take a safe snapshot of name" API; etc.).

	I want to reduce the PITA of regular code audits.  If, at
some point, Rust use in parts of the tree reduces that - wonderful.
But then we'd better make sure that Rust-side uses _are_ safe, accurately
annotated and easy to grep for.  Because we'll almost certainly need to
change method calling conventions at some points through all of that.
Even if it's just the annotation-level, any such contract change (it
is doable and quite a few had been done) will require going through
the instances and checking how much massage will be needed in those.
Opaque chunks like the above promise to make that very painful...

Powered by blists - more mailing lists