linux-kernel - Re: [PATCH] iversion: update comments with info about atime updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f17b9d627703bee2a7b531a051461671648a9dbd.camel@kernel.org>
Date:   Mon, 22 Aug 2022 12:22:55 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Mimi Zohar <zohar@...ux.ibm.com>, linux-fsdevel@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-ext4@...r.kernel.org, NeilBrown <neilb@...e.de>,
        Trond Myklebust <trondmy@...merspace.com>,
        Dave Chinner <david@...morbit.com>
Subject: Re: [PATCH] iversion: update comments with info about atime updates

On Mon, 2022-08-22 at 11:40 -0400, Mimi Zohar wrote:
> On Mon, 2022-08-22 at 09:33 -0400, Jeff Layton wrote:
> > Add an explicit paragraph codifying that atime updates due to reads
> > should not be counted against the i_version counter. None of the
> > existing subsystems that use the i_version want those counted, and
> > there is an easy workaround for those that do.
> > 
> > Cc: NeilBrown <neilb@...e.de>
> > Cc: Trond Myklebust <trondmy@...merspace.com>
> > Cc: Dave Chinner <david@...morbit.com>
> > Link: https://lore.kernel.org/linux-xfs/166086932784.5425.17134712694961326033@noble.neil.brown.name/#t
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> >  include/linux/iversion.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> > index 3bfebde5a1a6..da6cc1cc520a 100644
> > --- a/include/linux/iversion.h
> > +++ b/include/linux/iversion.h
> > @@ -9,8 +9,8 @@
> >   * ---------------------------
> >   * The change attribute (i_version) is mandated by NFSv4 and is mostly for
> >   * knfsd, but is also used for other purposes (e.g. IMA). The i_version must
> > - * appear different to observers if there was a change to the inode's data or
> > - * metadata since it was last queried.
> > + * appear different to observers if there was an explicit change to the inode's
> > + * data or metadata since it was last queried.
> >   *
> >   * Observers see the i_version as a 64-bit number that never decreases. If it
> >   * remains the same since it was last checked, then nothing has changed in the
> > @@ -18,6 +18,12 @@
> >   * anything about the nature or magnitude of the changes from the value, only
> >   * that the inode has changed in some fashion.
> >   *
> > + * Note that atime updates due to reads or similar activity do _not_ represent
> > + * an explicit change to the inode. If the only change is to the atime and it
> 
> Thanks, Jeff.  The ext4 patch increments i_version on file metadata
> changes.  Could the wording here be more explicit to reflect changes
> based on either inode data or metadata changes?
> 
> 

Thanks Mimi,

Care to suggest some wording?

The main issue we have is that ext4 and xfs both increment i_version on
atime updates due to reads. I have patches in flight to fix those, but
going forward, we want to ensure that i_version gets incremented on all
changes _except_ for atime updates.

The best wording we have at the moment is what Trond suggested, which is
to classify the changes to the inode as "explicit" (someone or something
made a deliberate change to the inode) and "implicit" (the change to the
inode was due to activity such as reads that don't actually change
anything).

Is there a better way to describe this?

> > + * wasn't set via utimes() or a similar mechanism, then i_version should not be
> > + * incremented. If an observer cares about atime updates, it should plan to
> > + * fetch and store them in conjunction with the i_version.
> > + *
> >   * Not all filesystems properly implement the i_version counter. Subsystems that
> >   * want to use i_version field on an inode should first check whether the
> >   * filesystem sets the SB_I_VERSION flag (usually via the IS_I_VERSION macro).
> 
> 

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