linux-kernel - Re: [PATCH 51/87] fs/nfsd: convert to new inode {a,m}time accessors

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: <ZRWGBGqYe3rF5CRY@tissot.1015granger.net>
Date:   Thu, 28 Sep 2023 09:56:20 -0400
From:   Chuck Lever <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        linux-nfs@...r.kernel.org
Subject: Re: [PATCH 51/87] fs/nfsd: convert to new inode {a,m}time accessors

On Thu, Sep 28, 2023 at 07:03:00AM -0400, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/nfsd/blocklayout.c | 3 ++-
>  fs/nfsd/nfs3proc.c    | 4 ++--
>  fs/nfsd/nfs4proc.c    | 8 ++++----
>  fs/nfsd/nfsctl.c      | 2 +-
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 01d7fd108cf3..bdc582777738 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -119,10 +119,11 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  {
>  	loff_t new_size = lcp->lc_last_wr + 1;
>  	struct iattr iattr = { .ia_valid = 0 };
> +	struct timespec64 mtime = inode_get_mtime(inode);

Nit: Please use reverse Christmas tree for new variable declarations.


>  	int error;
>  
>  	if (lcp->lc_mtime.tv_nsec == UTIME_NOW ||
> -	    timespec64_compare(&lcp->lc_mtime, &inode->i_mtime) < 0)
> +	    timespec64_compare(&lcp->lc_mtime, &mtime) < 0)
>  		lcp->lc_mtime = current_time(inode);
>  	iattr.ia_valid |= ATTR_ATIME | ATTR_CTIME | ATTR_MTIME;
>  	iattr.ia_atime = iattr.ia_ctime = iattr.ia_mtime = lcp->lc_mtime;
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 268ef57751c4..b1c90a901d3e 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -294,8 +294,8 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS3_CREATE_EXCLUSIVE:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&

"inode_get_atime(yada).tv_sec" seems to be a frequently-repeated
idiom, at least in this patch. Would it be helpful to have an
additional helper that extracted just the seconds field, and one
that extracts just the nsec field?


>  			    d_inode(child)->i_size == 0) {
>  				break;
>  			}
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 4199ede0583c..b17309aac0d5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -322,8 +322,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS4_CREATE_EXCLUSIVE:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&
>  			    d_inode(child)->i_size == 0) {
>  				open->op_created = true;
>  				break;		/* subtle */
> @@ -331,8 +331,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS4_CREATE_EXCLUSIVE4_1:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&
>  			    d_inode(child)->i_size == 0) {
>  				open->op_created = true;
>  				goto set_attr;	/* subtle */
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..846559e4769b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1132,7 +1132,7 @@ static struct inode *nfsd_get_inode(struct super_block *sb, umode_t mode)
>  	/* Following advice from simple_fill_super documentation: */
>  	inode->i_ino = iunique(sb, NFSD_MaxReserved);
>  	inode->i_mode = mode;
> -	inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode);
> +	simple_inode_init_ts(inode);

An observation about the whole series: Should these helpers use the
usual naming convention of:

  <subsystem>-<subject>-<verb>

So we get:

  simple_inode_ts_init(inode);

  inode_atime_get(inode)


>  	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		inode->i_fop = &simple_dir_operations;
> -- 
> 2.41.0
> 

Otherwise, for the patch(es) touching nfsd:

Acked-by: Chuck Lever <chuck.lever@...cle.com>

-- 
Chuck Lever