On Mon, 2023-01-30 at 13:05 +1100, Dave Chinner wrote: > 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. > Ok. So for xfs, we could move all of this to be afterward. Clearing setuid bits is quite rare, so that would only rarely require a transaction (in principle). > > 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? > We could bump the ctime too in this situation, but it would be more costly. In most cases the i_version bump will be a no-op. The only exception would be when a query of i_version races in between the two bumps. That wouldn't be the case with the ctime, which would almost always require a second transaction. -- Jeff Layton <jlayton@xxxxxxxxxx>
