Message-ID: <bd98a7dd123a68c9a6481a451c1cc4e5132740ac.camel@kernel.org>
Date: Wed, 07 Jan 2026 16:10:12 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Frederick Lawler <fred@...udflare.com>
Cc: Mimi Zohar <zohar@...ux.ibm.com>, Roberto Sassu
 <roberto.sassu@...wei.com>,  Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
 Eric Snowberg <eric.snowberg@...cle.com>, Paul Moore	
 <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, "Serge E. Hallyn"	
 <serge@...lyn.com>, "Darrick J. Wong" <djwong@...nel.org>, Christian
 Brauner	 <brauner@...nel.org>, Josef Bacik <josef@...icpanda.com>, 
	linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org, 
	linux-security-module@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH RFC] ima: Fallback to a ctime guard without i_version
 updates

On Wed, 2026-01-07 at 13:56 -0600, Frederick Lawler wrote:
> On Tue, Jan 06, 2026 at 02:50:31PM -0500, Jeff Layton wrote:
> > On Tue, 2026-01-06 at 13:33 -0600, Frederick Lawler wrote:
> > > On Tue, Jan 06, 2026 at 10:43:01AM -0600, Frederick Lawler wrote:
> > > > Hi Jeff,
> > > > 
> > > > On Tue, Jan 06, 2026 at 07:01:08AM -0500, Jeff Layton wrote:
> > > > > On Mon, 2025-12-29 at 11:52 -0600, Frederick Lawler wrote:
> > > > > > Since commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps"), IMA
> > > > > > is no longer able to correctly track inode.i_version due to the struct
> > > > > > kstat.change_cookie no longer containing an updated i_version.
> > > > > > 
> > > > > > Introduce a fallback mechanism for IMA that instead tracks a
> > > > > > integrity_ctime_guard() in absence of or outdated i_version
> > > > > > for stacked file systems.
> > > > > > 
> > > > > > EVM is left alone since it mostly cares about the backing inode.
> > > > > > 
> > > > > > Link: https://lore.kernel.org/all/aTspr4_h9IU4EyrR@CMGLRV3
> > > > > > Fixes: 1cf7e834a6fb ("xfs: switch to multigrain timestamps")
> > > > > > Suggested-by: Jeff Layton <jlayton@...nel.org>
> > > > > > Signed-off-by: Frederick Lawler <fred@...udflare.com>
> > > > > > ---
> > > > > > The motivation behind this was that file systems that use the
> > > > > > cookie to set the i_version for stacked file systems may still do so.
> > > > > > Then add in the ctime_guard as a fallback if there's a detected change.
> > > > > > The assumption is that the ctime will be different if the i_version is
> > > > > > different anyway for non-stacked file systems.
> > > > > > 
> > > > > > I'm not too pleased with passing in struct file* to
> > > > > > integrity_inode_attrs_changed() since EVM doesn't currently use
> > > > > > that for now, but I couldn't come up with another idea to get the
> > > > > > stat without coming up with a new stat function to accommodate just
> > > > > > the file path, fully separate out IMA/EVM checks, or lastly add stacked
> > > > > > file system support to EVM (which doesn't make much sense to me
> > > > > > at the moment).
> > > > > > 
> > > > > > I plan on adding in self test infrastructure for the v1, but I would
> > > > > > like to get some early feedback on the approach first.
> > > > > > ---
> > > > > >  include/linux/integrity.h           | 29 ++++++++++++++++++++++++-----
> > > > > >  security/integrity/evm/evm_crypto.c |  2 +-
> > > > > >  security/integrity/evm/evm_main.c   |  2 +-
> > > > > >  security/integrity/ima/ima_api.c    | 21 +++++++++++++++------
> > > > > >  security/integrity/ima/ima_main.c   | 17 ++++++++++-------
> > > > > >  5 files changed, 51 insertions(+), 20 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> > > > > > index f5842372359be5341b6870a43b92e695e8fc78af..4964c0f2bbda0ca450d135b9b738bc92256c375a 100644
> > > > > > --- a/include/linux/integrity.h
> > > > > > +++ b/include/linux/integrity.h
> > > > > > @@ -31,19 +31,27 @@ static inline void integrity_load_keys(void)
> > > > > >  
> > > > > >  /* An inode's attributes for detection of changes */
> > > > > >  struct integrity_inode_attributes {
> > > > > > +	u64 ctime_guard;
> > > > > >  	u64 version;		/* track inode changes */
> > > > > >  	unsigned long ino;
> > > > > >  	dev_t dev;
> > > > > >  };
> > > > > >  
> > > > > > +static inline u64 integrity_ctime_guard(struct kstat stat)
> > > > > > +{
> > > > > > +	return stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> > > > > > +}
> > > > > > +
> > > > > >  /*
> > > > > >   * On stacked filesystems the i_version alone is not enough to detect file data
> > > > > >   * or metadata change. Additional metadata is required.
> > > > > >   */
> > > > > >  static inline void
> > > > > >  integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
> > > > > > -			    u64 i_version, const struct inode *inode)
> > > > > > +			    u64 i_version, u64 ctime_guard,
> > > > > > +			    const struct inode *inode)
> > > > > >  {
> > > > > > +	attrs->ctime_guard = ctime_guard;
> > > > > >  	attrs->version = i_version;
> > > > > >  	attrs->dev = inode->i_sb->s_dev;
> > > > > >  	attrs->ino = inode->i_ino;
> > > > > > @@ -54,11 +62,22 @@ integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
> > > > > >   */
> > > > > >  static inline bool
> > > > > >  integrity_inode_attrs_changed(const struct integrity_inode_attributes *attrs,
> > > > > > -			      const struct inode *inode)
> > > > > > +			      struct file *file, struct inode *inode)
> > > > > >  {
> > > > > > -	return (inode->i_sb->s_dev != attrs->dev ||
> > > > > > -		inode->i_ino != attrs->ino ||
> > > > > > -		!inode_eq_iversion(inode, attrs->version));
> > > > > > +	struct kstat stat;
> > > > > > +
> > > > > > +	if (inode->i_sb->s_dev != attrs->dev ||
> > > > > > +	    inode->i_ino != attrs->ino)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	if (inode_eq_iversion(inode, attrs->version))
> > > > > > +		return false;
> > > > > > +
> > > > > > +	if (!file || vfs_getattr_nosec(&file->f_path, &stat, STATX_CTIME,
> > > > > > +				       AT_STATX_SYNC_AS_STAT))
> > > > > > +		return true;
> > > > > > +
> > > > > 
> > > > > This is rather odd. You're sampling the i_version field directly, but
> > > > > if it's not equal then you go through ->getattr() to get the ctime.
> > > > > 
> > > > > It's particularly odd since you don't know whether the i_version field
> > > > > is even implemented on the fs. On filesystems where it isn't, the
> > > > > i_version field generally stays at 0, so won't this never fall through
> > > > > to do the vfs_getattr_nosec() call on those filesystems?
> > > > > 
> > > > 
> > > > You're totally right. I didn't consider FS's caching the value at zero.
> > > 
> > > Actually, I'm going to amend this. I think I did consider FSs without an
> > > implementation. Where this is called at, it is often guarded by a
> > > !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> > > understanding this correctly, the check call doesn't occur unless the inode
> > > has i_version support.
> > > 
> > 
> > 
> > It depends on what you mean by i_version support:
> > 
> > That flag just tells the VFS that it needs to bump the i_version field
> > when updating timestamps. It's not a reliable indicator of whether the
> > i_version field is suitable for the purpose you want here.
> > 
> 
> So, it would make sense then to also remove those guards at the callsite
> then if the intention is to compare against the cookie & ctime regardless?
> 

Yes, I'd say so. IS_I_VERSION() is not a reliable indicator of whether
the filesystem can provide a proper change attribute. For instance,
cephfs and NFS don't set that flag, but they can (sometimes) provide
STATX_CHANGE_COOKIE. Going through getattr() is a much more reliable
method.

> > The problem here and the one that we ultimately fixed with multigrain
> > timestamps is that XFS in particular will bump i_version on any change
> > to the log. That includes atime updates due to reads.
> > 
> > XFS still tracks the i_version the way it always has, but we've stopped
> > getattr() from reporting it because it's not suitable for the purpose
> > that nfsd (and IMA) need it for.
> > 
> > > It seems to me the suggestion then is to remove the IS_I_VERSION()
> > > checks guarding the call sites, grab both ctime and cookie from stat,
> > > and if IS_I_VERSION() use that, otherwise cookie, and compare
> > > against the cached i_version with one of those values, and then fall
> > > back to ctime?
> > > 
> > 
> > Not exactly.
> > 
> > You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
> > then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
> > use that. If it's not then use the ctime.
> 
> Ok, I think I understand. To reiterate my understanding, ignore calling
> into inode_eq_iversion() all together.
> 
> 	return other_checks || ((mask & cookie) ? cache->i_version == cookie_val :
> 	compare_ctime())
> 

Yes, that psuedocode looks about right. IS_I_VERSION(),
inode_eq_iversion(), etc. are really filesystem-internal APIs and not
something IMA should be relying on as a fstype-agnostic LSM.


> > 
> > The part I'm not sure about is whether it's actually safe to do this.
> > vfs_getattr_nosec() can block in some situations. Is it ok to do this
> > in any context where integrity_inode_attrs_changed() may be called? 
> > 
> > ISTR that this was an issue at one point, but maybe isn't now that IMA
> > is an LSM?
> 
> Poking around, callers to integrity_inode_attrs_changed() are currently behind
> mutex_lock(&iinit->mutex) (similar to vfs_getattr_nosec() calls), and currently
> only called from process_measurement().
> 
> While I can't say for certain this will always be the case for future use
> use cases, would it be helpful to include a may_sleep() in
> integrity_inode_attrs_changed() to drive this point and make a comment?
> 

Those kind of annotations do tend to be useful.

> 
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists