linux-kernel - Re: [PATCH 1/6] Change inode_operations.mkdir to return struct dentry *

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: <174036658872.74271.7972364767583388815@noble.neil.brown.name>
Date: Mon, 24 Feb 2025 14:09:48 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Al Viro" <viro@...iv.linux.org.uk>, Trond Myklebust <trondmy@...nel.org>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Miklos Szeredi" <miklos@...redi.hu>, "Xiubo Li" <xiubli@...hat.com>,
 "Ilya Dryomov" <idryomov@...il.com>, "Richard Weinberger" <richard@....at>,
 "Anton Ivanov" <anton.ivanov@...bridgegreys.com>,
 "Johannes Berg" <johannes@...solutions.net>,
 "Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>,
 "Sergey Senozhatsky" <senozhatsky@...omium.org>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-cifs@...r.kernel.org, linux-nfs@...r.kernel.org,
 linux-um@...ts.infradead.org, ceph-devel@...r.kernel.org,
 netfs@...ts.linux.dev
Subject:
 Re: [PATCH 1/6] Change inode_operations.mkdir to return struct dentry *

On Mon, 24 Feb 2025, Al Viro wrote:
> On Mon, Feb 24, 2025 at 12:34:06PM +1100, NeilBrown wrote:
> > On Sat, 22 Feb 2025, Al Viro wrote:
> > > On Fri, Feb 21, 2025 at 10:36:30AM +1100, NeilBrown wrote:
> > > 
> > > > +In general, filesystems which use d_instantiate_new() to install the new
> > > > +inode can safely return NULL.  Filesystems which may not have an I_NEW inode
> > > > +should use d_drop();d_splice_alias() and return the result of the latter.
> > > 
> > > IMO that's a bad pattern, _especially_ if you want to go for "in-update"
> > > kind of stuff later.
> > 
> > Agreed.  I have a draft patch to change d_splice_alias() and
> > d_exact_alias() to work on hashed dentrys.  I thought it should go after
> > these mkdir patches rather than before.
> 
> Could you give a braindump on the things d_exact_alias() is needed for?
> It's a recurring headache when doing ->d_name/->d_parent audits; see e.g.
> https://lore.kernel.org/all/20241213080023.GI3387508@ZenIV/ for related
> mini-rant from the latest iteration.
> 
> Proof of correctness is bloody awful; it feels like the primitive itself
> is wrong, but I'd never been able to write anything concise regarding
> the things we really want there ;-/
> 

As I understand it, it is needed (or wanted) to handle the possibility
of an inode becoming "stale" and then recovering.  This could happen,
for example, with a temporarily misconfigured NFS server.

If ->d_revalidate gets a NFSERR_STALE from the server it will return '0'
so lookup_fast() and others will call d_invalidate() which will d_drop()
the dentry.  There are other paths on which -ESTALE can result in d_drop().

If a subsequent attempt to "open" the name successfully finds the same
inode we want to reuse the old dentry rather than create a new one.

I don't really understand why.  This code was added 20 years ago before
git.
It was introduced by

commit 89a45174b6b32596ea98fa3f89a243e2c1188a01
Author: Trond Myklebust <trond.myklebust@....uio.no>
Date:   Tue Jan 4 21:41:37 2005 +0100

     VFS: Avoid dentry aliasing problems in filesystems like NFS, where
          inodes may be marked as stale in one instance (causing the dentry
          to be dropped) then re-enabled in the next instance.
    
     Signed-off-by: Trond Myklebust <trond.myklebust@....uio.no>

in history.git

Trond: do you have any memory of this?  Can you explain what the symptom
was that you wanted to fix?

The original patch used d_add_unique() for lookup and atomic_open and
readdir prime-dcache.  We now only use it for v4 atomic_open.  Maybe we
don't need it at all?  Or maybe we need to restore it to those other
callers? 


Thanks,
NeilBrown