Re: [PATCH] xfs: reset child dir '..' entry when unlinking child

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 06, 2021 at 07:20:21AM +0800, Gao Xiang wrote:
> Hi Dave and Christoph,
> 
> On Tue, Jul 06, 2021 at 08:09:25AM +1000, Dave Chinner wrote:
> > On Fri, Jul 02, 2021 at 08:02:33PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > While running xfs/168, I noticed a second source of post-shrink
> > > corruption errors causing shutdowns.
> > > 
> > > Let's say that directory B has a low inode number and is a child of
> > > directory A, which has a high number.  If B is empty but open, and
> > > unlinked from A, B's dotdot link continues to point to A.  If A is then
> > > unlinked and the filesystem shrunk so that A is no longer a valid inode,
> > > a subsequent AIL push of B will trip the inode verifiers because the
> > > dotdot entry points outside of the filesystem.
> > 
> > So we have a directory inode that is empty and unlinked but held
> > open, with a back pointer to an invalid inode number? Which can
> > never be followed, because the directory has been unlinked.
> > 
> > Can't this be handled in the inode verifier? This seems to me to
> > be a pretty clear cut case where the ".." back pointer should
> > always be considered invalid (because the parent dir has no
> > existence guarantee once the child has been removed from it), not
> > just in the situation where the filesystem has been shrunk...
> 
> Yes, I agree all of this, this field can be handled by the inode
> verifier. The only concern I can think out might be fs freeze
> with a directory inode that is empty and unlinked but held open,
> and then recovery on a unpatched old kernels, not sure if such
> case can be handled properly by old kernel verifier.

I decided against changing the shortform directory verifier to ignore
the dotdot pointer on nlink==0 directories because older kernels will
still have the current behavior and that will cause recovery failure for
the following sequence:

1. create A and B and delete A as per above, leave B open
2. shrink fs so that A is no longer a valid inode number
3. flush the shrink transaction to disk
4. futimens(B) (or anything to dirty the inode)
5. crash
6. boot old kernel
7. try to recover fs with old kernel, which will crash since B hadn't
   been inactivated

But I guess I'll have to go write an fstest to prove that I'm not
making this up...

--D

> 
> Otherwise, it's also ok I think.
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com