On Mon, Nov 27, 2023 at 04:03:18PM +0000, Al Viro wrote: > On Mon, Nov 27, 2023 at 09:47:47AM -0600, Eric W. Biederman wrote: > > > There is a lot going on there. I remember one of the relevant > > restrictions was marking dentries dont_mount, and inodes S_DEAD > > in unlink and rmdir. > > > > But even without out that marking if d_invalidate is called > > from d_revalidate the inode and all of it's dentries must be > > dead because the inode is stale and most go. There should > > be no resurrecting it at that point. > > > > I suspect the most fruitful way to think of the d_invalidate vs > > d_splice_alias races is an unlink vs rename race. > > > > I don't think the mechanism matters, but deeply and fundamentally > > if we detect a directory inode is dead we need to stick with > > that decision and not attempt to resurrect it with d_splice_alias. > > Wrong. Deeply and fundamentally we detect a dentry that does not > match the directory contents according to the server. > > For example, due to rename done on server. With object in question > perfectly alive there - fhandle still works, etc. > > However, it's no longer where it used to be. And we would bloody better > not have lookups for the old name result in access to that object. > We also should never allow the access to *new* name lead to two live > dentries for the same directory inode. > > Again, this is not about rmdir() or unlink() - invalidation can happen > for object that is still open, still accessed and still very much alive. > Does that all the time for any filesystem with ->d_revalidate(). Put another way, there used to be very odd song and dance in ->d_revalidate() instances along the lines of "we can't possibly tell the caller to invalidate a mountpoint"; it was racy in the best case and during the rewrite of d_invalidate() to teach it how to evict submounts those attempts had been dropped - ->d_revalidate() returning 0 does end up with mounts dissolved by d_invalidate() from caller. It always had been racy, starting with the checks that used to be in ->d_revalidate() instances way before all those changes. So the switch of d_invalidate() to dissolving submounts had been a step in the right direction, but it's not being careful enough. Again, it's about d_invalidate() caused by pathwalk running into a dentry that doesn't match the reality vs. d_splice_alias() finding that it matches the inode we had looked up elsewhere.
