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: <f1fdddde-2450-4a2c-a1e8-ee6a3ff81090@oracle.com>
Date: Fri, 4 Jul 2025 12:29:58 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Sergey Bashirov <sergeybashirov@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        Jeff Layton <jlayton@...nel.org>, NeilBrown <neil@...wn.name>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Konstantin Evtushenko <koevtushenko@...dex.com>
Subject: Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

Hi Sergey, Konstantin -


On 7/4/25 7:49 AM, 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.
> 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 seg range.

The patch description should describe the impact of the current
incorrect logic -- does it result in file corruption, failed tests, etc?
That way support engineers at distributions can more easily find this
patch if a customer runs across bad behavior.

Also, let's reference RFC 8881 Section 12.5.4.2, where the properly
compliant behavior is specified.

Fixes: 9cf514ccfacb ("nfsd: implement pNFS operations")


> Co-developed-by: Konstantin Evtushenko <koevtushenko@...dex.com>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@...dex.com>
> Signed-off-by: Sergey Bashirov <sergeybashirov@...il.com>
> ---
>  fs/nfsd/blocklayout.c |  2 +-
>  fs/nfsd/nfs4proc.c    | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 19078a043e85..ee6544bdc045 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -118,7 +118,7 @@ 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;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
>  	struct iattr iattr = { .ia_valid = 0 };
>  	int error;

See below for an alternative.


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37bdb937a0ae..ff38be803d8b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2482,7 +2482,7 @@ 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;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
>   	struct inode *inode;
>  	struct nfs4_layout_stateid *ls;
>  	__be32 nfserr;
> @@ -2498,13 +2498,13 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  		goto out;
>  	inode = d_inode(current_fh->fh_dentry);
>  

How about instead, drop the new_size initializer above, and do this:

	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 = 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;
> +	if (new_size) {
> +		nfserr = nfserr_inval;
> +		if (new_size <= seg->offset)
> +			goto out;
> +		if (new_size > seg->offset + seg->length)
> +			goto out;
> +	}
>  
>  	nfserr = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
>  						false, lcp->lc_layout_type,

And lastly:

-	if (new_size > i_size_read(inode)) {
-		lcp->lc_size_chg = true;
-		lcp->lc_newsize = new_size;
-	} else {
-		lcp->lc_size_chg = false;
-	}




Also, I notice that nfsd4_decode_layoutcommit() has:

        if (xdr_stream_decode_bool(argp->xdr, &lcp->lc_reclaim) < 0)
                return nfserr_bad_xdr;

but:

        if (xdr_stream_decode_u32(argp->xdr, &lcp->lc_newoffset) < 0)
                return nfserr_bad_xdr;

The no_newoffset field should be decoded with xdr_stream_decode_bool too
(though the end result is the same). For just this nit, please make a
separate patch. Thanks!

-- 
Chuck Lever