linux-kernel - Re: [PATCH 01/19] VFS: introduce vfs_mkdir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <173916216443.22054.1710124383016579976@noble.neil.brown.name>
Date: Mon, 10 Feb 2025 15:36:04 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Linus Torvalds" <torvalds@...ux-foundation.org>,
 "Jeff Layton" <jlayton@...nel.org>, "Dave Chinner" <david@...morbit.com>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/19] VFS: introduce vfs_mkdir_return()

On Sat, 08 Feb 2025, Al Viro wrote:
> On Thu, Feb 06, 2025 at 04:42:38PM +1100, NeilBrown wrote:
> > vfs_mkdir() does not guarantee to make the child dentry positive on
> > success.  It may leave it negative and then the caller needs to perform a
> > lookup to find the target dentry.
> > 
> > This patch introduced vfs_mkdir_return() which performs the lookup if
> > needed so that this code is centralised.
> > 
> > This prepares for a new inode operation which will perform mkdir and
> > returns the correct dentry.
> 
> * Calling conventions stink; make it _consume_ dentry reference and
> return dentry reference or ERR_PTR().  Callers will be happier that way
> (check it).

With later patches it will need to consume the lock on the dentry as
well, and either transfer it to the new one or (for error) unlock it.
We need to have the result dentry still locked for fsnotify_mkdir().

Transferring the dentry lock would have to be done in d_splice_alias(). 
The __d_unalias() branch should be ok because I already trylock in there
and fail if I can't get the lock.  For the IS_ROOT branch ....  I think
it is safe to fail if a trylock doesn't succeed.

So I can probably make that work - thanks.

Hmm... kernfs reportedly can leave the mkdir dentry negative and fill in
the inode later.  How does that work?  I assume it will still be hashed
so mkdir won't try the lookup.

done_path_create() will need to accept an IS_ERR() dentry.

> 
> * Calling conventions should be documented in commit message *and* in
> D/f/porting

What is the scope of "porting" ?  IT seems to be mostly about
_operations interfaces, but I do see other things in there.  I'll try to
remember that - thanks.

> 
> * devpts, nfs4recover and xfs might as well convert (not going to hit
> the "need a lookup" case anyway)

good point - avoiding the lookup when not requested is a pointless
optimisation because it is hardly every needed and should always be
cheap - we expect it to be in the dcache.

> 
> * that 
> +                       /* Need a "const" pointer.  We know d_name is const
> +                        * because we hold an exclusive lock on i_rwsem
> +                        * in d_parent.
> +                        */
> +                       const struct qstr *d_name = (void*)&dentry->d_name;
> +                       d = lookup_dcache(d_name, dentry->d_parent, 0);
> +                       if (!d)
> +                               d = __lookup_slow(d_name, dentry->d_parent, 0);
> doesn't need a cast.  C is perfectly fine with
> 	T *x = foo();
> 	const T *y = x;
> 
> You are not allowed to _strip_ qualifiers; adding them is fine.
> Same reason why you are allowed to pass char * to strlen() without
> any casts whatsoever.

hmm..  I thought I had tried that.  Maybe I didn't try hard enough.
Thanks for the guidance.
> 
> Comment re stability is fine; the cast is pure WTF material.
> 

Thanks,
NeilBrown