On Tue, Feb 27, 2024 at 5:45 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > On Tue, Feb 27, 2024 at 11:23:39AM +0200, Amir Goldstein wrote: > > On Tue, Feb 27, 2024 at 4:18 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: [...] > > Maybe a stupid question, but under which circumstances would mtime > > change and ctime not change? Why are both needed? > > It's the other way 'round -- mtime doesn't change but ctime does. The > race I'm trying to protect against is: > > Thread 0 Thread 1 > <snapshot fd cmtime> > <start writing tempfd> > <fstat fd> > <write to fd> > <futimens to reset mtime> > <commitrange> > > mtime is controllable by "attackers" but ctime isn't. I think we only > need to capture ctime, but ye olde swapext ioctl (from which this > derives) did both. > Yes, that's what I meant. was just a braino. mtime seems redundant, but if you want to keep it for compatibility with legacy API, so be it. > > And for a new API, wouldn't it be better to use change_cookie (a.k.a i_version)? > > Seeing as iversion (as the vfs and/or jlayton seems to want it) doesn't > work in the intended manner in XFS, no. > OK. for the record, AFAICT, the problem of NFS guys with xfs iversion is that it is too aggressive to their taste (i.e. bumped on atime updates), but because ctime is always updated along with iversion in xfs and because iversion has no granularity problem, I think it is a better choice for you, regardless of any other filesystems and their interpretation of ctime or iversion. > > Even if this API is designed to be hoisted out of XFS at some future time, > > Is there a real need to support it on filesystems that do not support > > i_version(?) > > Given the way the iversion discussions have gone (file data write > counter) I don't think there's a way to support commitrange on > non-iversion filesystems. > > I withdrew any plans to make this more than an XFS-specific ioctl last > year after giving up on ever getting through fsdevel review. I think > the last reply I got was from viro back in 2021... > understandable. I wasn't implying that you should hoist this out of XFS. I was wondering about why not use xfs's iversion, which seems like a better change counter than ctime. > > Not to mention the fact that POSIX does not explicitly define how ctime should > > behave with changes to fiemap (uninitialized extent and all), so who knows > > how other filesystems may update ctime in those cases. > > ...and given the lack of interest from any other filesystem developers > in porting it to !xfs, I'm not likely to take this up ever again. To be > faiproblemr, I think the only filesystems that could possibly support > EXCHANGE_RANGE are /maybe/ btrfs and /probably/ bcachefs. > Again, sorry if my question were misinterpreted. I was not trying to imply that this API should be hoisted. > > I realize that STATX_CHANGE_COOKIE is currently kernel internal, but > > it seems that XFS_IOC_EXCHANGE_RANGE is a case where userspace > > really explicitly requests a bump of i_version on the next change. > > Another way I could've structured this (and still could!) would be to > deproblemclare the entire freshness region as an untyped u64 fresh[4] blob and > add a START_COMMIT ioctl to fill it out. Then the kernel fs drivers > gets to determine what goes in there. > > That at least would be less work for userspace to do. > To me that makes sense. Cleaner API. Less questions asked. But it's up to you. Thanks, Amir.
