linux-kernel - Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

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: <4ec5af772adc8c6e73fcaa25f894d6506d94e9ad.camel@kernel.org>
Date: Mon, 21 Jul 2025 14:56:38 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Sergey Bashirov <sergeybashirov@...il.com>, Chuck Lever
	 <chuck.lever@...cle.com>
Cc: Christoph Hellwig <hch@....de>, NeilBrown <neil@...wn.name>, Olga
 Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom
 Talpey <tom@...pey.com>, 	linux-nfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org,  Konstantin
 Evtushenko <koevtushenko@...dex.com>
Subject: Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

On Mon, 2025-07-21 at 21:40 +0300, Sergey Bashirov wrote:
> The data type of loca_last_write_offset is newoffset4 and is switched
> on a boolean value, no_newoffset, that indicates if a previous write
> occurred or not. If no_newoffset is FALSE, an offset is not given.

Gah, what a confusing label in the spec. I went down the rabbit hole of
trying to understand why "no_newoffset" being TRUE means that you do
get an offset. The "no_" in this case is just a prefix for "union
newoffset", and not a negation of the flag.


> This means that client does not try to update the file size. Thus,
> server should not try to calculate new file size and check if it fits
> into the segment range. See RFC 8881, section 12.5.4.2.
> 
> Sometimes the current incorrect logic may cause clients to hang when
> trying to sync an inode. If layoutcommit fails, the client marks the
> inode as dirty again.
> 
> Fixes: 9cf514ccfacb ("nfsd: implement pNFS operations")
> Cc: stable@...r.kernel.org
> Co-developed-by: Konstantin Evtushenko <koevtushenko@...dex.com>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@...dex.com>
> Signed-off-by: Sergey Bashirov <sergeybashirov@...il.com>
> Reviewed-by: Christoph Hellwig <hch@....de>
> ---
>  fs/nfsd/blocklayout.c |  5 ++---
>  fs/nfsd/nfs4proc.c    | 30 +++++++++++++++---------------
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 4c936132eb440..0822d8a119c6f 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -118,7 +118,6 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  		struct iomap *iomaps, int nr_iomaps)
>  {
>  	struct timespec64 mtime = inode_get_mtime(inode);
> -	loff_t new_size = lcp->lc_last_wr + 1;
>  	struct iattr iattr = { .ia_valid = 0 };
>  	int error;
>  
> @@ -128,9 +127,9 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  	iattr.ia_valid |= ATTR_ATIME | ATTR_CTIME | ATTR_MTIME;
>  	iattr.ia_atime = iattr.ia_ctime = iattr.ia_mtime = lcp->lc_mtime;
>  
> -	if (new_size > i_size_read(inode)) {
> +	if (lcp->lc_size_chg) {
>  		iattr.ia_valid |= ATTR_SIZE;
> -		iattr.ia_size = new_size;
> +		iattr.ia_size = lcp->lc_newsize;
>  	}
>  
>  	error = inode->i_sb->s_export_op->commit_blocks(inode, iomaps,
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 656b2e7d88407..7043fc475458d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2475,7 +2475,6 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	const struct nfsd4_layout_seg *seg = &lcp->lc_seg;
>  	struct svc_fh *current_fh = &cstate->current_fh;
>  	const struct nfsd4_layout_ops *ops;
> -	loff_t new_size = lcp->lc_last_wr + 1;
>  	struct inode *inode;
>  	struct nfs4_layout_stateid *ls;
>  	__be32 nfserr;
> @@ -2491,13 +2490,21 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  		goto out;
>  	inode = d_inode(current_fh->fh_dentry);
>  
> -	nfserr = nfserr_inval;
> -	if (new_size <= seg->offset)
> -		goto out;
> -	if (new_size > seg->offset + seg->length)
> -		goto out;
> -	if (!lcp->lc_newoffset && new_size > i_size_read(inode))
> -		goto out;
> +	lcp->lc_size_chg = false;
> +	if (lcp->lc_newoffset) {
> +		loff_t new_size = lcp->lc_last_wr + 1;
> +
> +		nfserr = nfserr_inval;
> +		if (new_size <= seg->offset)
> +			goto out;
> +		if (new_size > seg->offset + seg->length)
> +			goto out;
> +
> +		if (new_size > i_size_read(inode)) {
> +			lcp->lc_size_chg = true;
> +			lcp->lc_newsize = new_size;
> +		}
> +	}
>  
>  	nfserr = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
>  						false, lcp->lc_layout_type,
> @@ -2513,13 +2520,6 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	/* LAYOUTCOMMIT does not require any serialization */
>  	mutex_unlock(&ls->ls_mutex);
>  
> -	if (new_size > i_size_read(inode)) {
> -		lcp->lc_size_chg = true;
> -		lcp->lc_newsize = new_size;
> -	} else {
> -		lcp->lc_size_chg = false;
> -	}
> -
>  	nfserr = ops->proc_layoutcommit(inode, rqstp, lcp);
>  	nfs4_put_stid(&ls->ls_stid);
>  out:

Code looks correct to me though.

Reviewed-by: Jeff Layton <jlayton@...nel.org>