Re: xfs/ima: Regression caching i_version

[Jeff Layton <jlayton@kernel.org>]

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?


> > 
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c35ea613c9f8..5a71845f579e 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -272,10 +272,14 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> >          * to an initial measurement/appraisal/audit, but was modified to
> >          * assume the file changed.
> >          */
> > -       result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE,
> > +       result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE | STATX_CTIME,
> >                                    AT_STATX_SYNC_AS_STAT);
> > -       if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> > -               i_version = stat.change_cookie;
> > +       if (!result) {
> > +               if (stat.result_mask & STATX_CHANGE_COOKIE)
> > +                       i_version = stat.change_cookie;
> > +               else if (stat.result_mask & STATX_CTIME)
> > +                       i_version = stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> > +       }
> >         hash.hdr.algo = algo;
> >         hash.hdr.length = hash_digest_size[algo];
> >  

-- 
Jeff Layton <jlayton@kernel.org>