Re: xfs/ima: Regression caching i_version

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-12-11 at 16:29 -0600, Frederick Lawler wrote:
> On Fri, Dec 12, 2025 at 06:41:00AM +0900, Jeff Layton wrote:
> > On Thu, 2025-12-11 at 15:12 -0600, Frederick Lawler wrote:
> > > On Fri, Dec 12, 2025 at 05:55:45AM +0900, Jeff Layton wrote:
> > > > On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> > > > > Hi Jeff,
> > > > > 
> > > > > While testing 6.18, I think I found a regression with
> > > > > commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> > > > > where IMA is no longer able to properly cache i_version when we overlay
> > > > > tmpfs on top of XFS. Each measurement diff check in function
> > > > > process_measurement() reports that the i_version is
> > > > > always set to zero for iint->real_inode.version.
> > > > > 
> > > > > The function ima_collect_measurement() is looking to extract the version
> > > > > from the cookie on next measurement to cache i_version.
> > > > > 
> > > > > I'm unclear from the commit description what the right approach here is:
> > > > > update in IMA land by checking for time changes, or do
> > > > > something else such as adding the cookie back.
> > > > > 
> > > > > 
> > > > 
> > > > What we probably want to do is switch to using the ctime to manufacture
> > > > a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
> > > > reply.
> > > > 
> > > > IIRC, IMA doesn't need to persist these values across reboot, so
> > > > something like this (completely untested) might work, but it may be
> > > > better to lift nfsd4_change_attribute() into a common header and use
> > > > the same mechanism for both:
> > > 
> > > I agree lifting nfsd4_change_attribute(), if anything else, a consistent
> > > place to fetch the i_version from. Am I correct in my understanding that
> > > the XOR on the times will cancel out and result in just the i_version?
> > 
> > No. I was just using the XOR to mix the tv_sec and tv_nsec fields
> > together in a way that (hopefully) wouldn't generate collisions. It's
> > quite not as robust as what nfsd4_change_attribute() does, but might be
> > sane enough for IMA.
> > 
> > > IMA is calling into inode_eq_iversion() to perform the comparison
> > > between the cached value and inode.i_version.
> > 
> > That just looks at the i_version field directly without going through -
> > > getattr, so that would need to be switched over as well. Could
> > integrity_inode_attrs_changed() use vfs_getattr_nosec() and compare the
> > result?
> 
> That makes sense to me. I'll look through it a bit more, roll a patch, and
> see what the IMA folks have to say (unless they comment here first).
> 

Great, thanks. Looks like ima_check_last_writer() might also need a
similar change.

I'm not sure, but It's possible that vfs_getattr_nosec() may not be
callable from all the contexts where integrity_inode_attrs_changed() is
called. If that is the case, then you may just need to convert IMA back
to accessing the i_version field directly instead of going through
vfs_getattr_nosec().

That's less than ideal though, as it will limit the filesystems where
IMA can be implemented. You'll need to come up with a way to deal with
the fact that XFS's i_version field can change on atime updates.
-- 
Jeff Layton <jlayton@kernel.org>