Re: replacement i_version counter for xfs

[Dave Chinner <david@fromorbit.com>]

On Wed, Jan 25, 2023 at 12:58:08PM -0500, Jeff Layton wrote:
> On Wed, 2023-01-25 at 08:32 -0800, Darrick J. Wong wrote:
> > On Wed, Jan 25, 2023 at 06:47:12AM -0500, Jeff Layton wrote:
> > > Note that there are two other lingering issues with i_version. Neither
> > > of these are xfs-specific, but they may inform the changes you want to
> > > make there:
> > > 
> > > 1/ the ctime and i_version can roll backward on a crash.
> > > 
> > > 2/ the ctime and i_version are both currently updated before write data
> > > is copied to the pagecache. It would be ideal if that were done
> > > afterward instead. (FWIW, I have some draft patches for btrfs and ext4
> > > for this, but they need a lot more testing.)
> > 
> > You might also want some means for xfs to tell the vfs that it already
> > did the timestamp update (because, say, we had to allocate blocks).
> > I wonder what people will say when we have to run a transaction before
> > the write to peel off suid bits and another one after to update ctime.
> > 
> 
> That's a great question! There is a related one too once I started
> looking at this in more detail:
> 
> Most filesystems end up updating the timestamp via a the call to
> file_update_time in __generic_file_write_iter. Today, that's called very
> early in the function and if it fails, the write fails without changing
> anything.
> 
> What do we do now if the write succeeds, but update_time fails? We don't

On XFS, the timestamp update will either succeed or cause the
filesystem to shutdown as a failure with a dirty transaction is a
fatal, unrecoverable error.

> want to return an error on the write() since the data did get copied in.
> Ignoring it seems wrong too though. There could even be some way to
> exploit that by changing the contents while holding the timestamp and
> version constant.

If the filesystem has shut down, it doesn't matter that the data got
copied into the kernel - it's never going to make it to disk and
attempts to read it back will also fail. There's nothing that can be
exploited by such a failure on XFS - it's game over for everyone
once the fs has shut down....

> At this point I'm leaning toward leaving the ctime and i_version to be
> updated before the write, and just bumping the i_version a second time
> after. In most cases the second bump will end up being a no-op, unless
> an i_version query races in between.

Why not also bump ctime at write completion if a query races with
the write()? Wouldn't that put ns-granularity ctime based change
detection on a par with i_version?

Userspace isn't going to notice the difference - the ctime they
observe indicates that it was changed during the syscall. So
who/what is going to care if we bump ctime twice in the syscall
instead of just once in this rare corner case?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com