linux-kernel - Re: [RFC PATCH 3/3][RESEND] xfs: mark the inode for high-res timestamp update in getattr

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: <20230411-schokolade-hegemonie-83b1f0ecfd6c@brauner>
Date:   Tue, 11 Apr 2023 17:15:17 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     Jeff Layton <jlayton@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Chinner <david@...morbit.com>,
        Chuck Lever <chuck.lever@...cle.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-mm@...ck.org,
        linux-nfs@...r.kernel.org
Subject: Re: [RFC PATCH 3/3][RESEND] xfs: mark the inode for high-res
 timestamp update in getattr

On Tue, Apr 11, 2023 at 07:54:46AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 11, 2023 at 10:37:02AM -0400, Jeff Layton wrote:
> > When the mtime or ctime is being queried via getattr, ensure that we
> > mark the inode for a high-res timestamp update on the next pass. Also,
> > switch to current_cmtime for other c/mtime updates.
> > 
> > With this change, we're better off having the NFS server just ignore
> > the i_version field and have it use the ctime instead, so clear the
> > STATX_CHANGE_COOKIE flag in the result mask in ->getattr.
> > 
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
> >  fs/xfs/xfs_acl.c                |  2 +-
> >  fs/xfs/xfs_inode.c              |  2 +-
> >  fs/xfs/xfs_iops.c               | 15 ++++++++++++---
> >  4 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 8b5547073379..9ad7c229c617 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -63,7 +63,7 @@ xfs_trans_ichgtime(
> >  	ASSERT(tp);
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  
> >  	if (flags & XFS_ICHGTIME_MOD)
> >  		inode->i_mtime = tv;
> > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > index 791db7d9c849..461adc58cf8c 100644
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -233,7 +233,7 @@ xfs_acl_set_mode(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	inode->i_mode = mode;
> > -	inode->i_ctime = current_time(inode);
> > +	inode->i_ctime = current_cmtime(inode);
> 
> Hmm, now we're adding a spinlock to all these updates.
> Does lockdep have anything exciting to say about this?
> 
> (I don't think it will, just wondering aloud...)
> 
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> >  	if (xfs_has_wsync(mp))
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 5808abab786c..80f9d731e261 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -843,7 +843,7 @@ xfs_init_new_inode(
> >  	ip->i_df.if_nextents = 0;
> >  	ASSERT(ip->i_nblocks == 0);
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  	inode->i_mtime = tv;
> >  	inode->i_atime = tv;
> >  	inode->i_ctime = tv;
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 24718adb3c16..a0b07f90e16c 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -565,6 +565,15 @@ xfs_vn_getattr(
> >  	if (xfs_is_shutdown(mp))
> >  		return -EIO;
> >  
> > +	/*
> > +	 * XFS uses the i_version infrastructure to track any change to
> > +	 * the inode, including atime updates. This means that the i_version
> > +	 * returned by getattr doesn't conform to what the callers expect.
> > +	 * Clear it here so that nfsd will fake up a change cookie from the
> > +	 * ctime instead.
> > +	 */
> > +	stat->result_mask &= ~STATX_CHANGE_COOKIE;
> > +
> >  	stat->size = XFS_ISIZE(ip);
> >  	stat->dev = inode->i_sb->s_dev;
> >  	stat->mode = inode->i_mode;
> > @@ -573,8 +582,8 @@ xfs_vn_getattr(
> >  	stat->gid = vfsgid_into_kgid(vfsgid);
> >  	stat->ino = ip->i_ino;
> >  	stat->atime = inode->i_atime;
> > -	stat->mtime = inode->i_mtime;
> > -	stat->ctime = inode->i_ctime;
> > +	if (request_mask & (STATX_CTIME|STATX_MTIME))
> > +		fill_cmtime_and_mark(inode, stat);
> 
> Should we be setting STATX_[CM]TIME in the result_mask, just in case the
> caller asked for ctime and not mtime?

I think the expectation is that everything in STATX_BASIC_MASK is always
returned to allow equivalence between stat() and statx(). So requesting
STATX_CTIME separately from STATX_MTIME isn't implemented widely, maybe
even not at atll?, yet.