linux-kernel - Re: [PATCH] nfsd: Use correct error code when decoding extents

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: <5c8c207c-0844-492f-99e0-48b874b5404a@oracle.com>
Date: Wed, 11 Jun 2025 11:58:44 -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] nfsd: Use correct error code when decoding extents

On 6/11/25 11:44 AM, Sergey Bashirov wrote:
> Update error codes in the block layout driver decoding functions to match
> the core nfsd code. NFS4ERR_EINVAL means that the server was able to decode
> the request, but the decoded values are invalid. Use NFS4ERR_BADXDR instead
> to indicate a decoding error.
> 
> Signed-off-by: Sergey Bashirov <sergeybashirov@...il.com>
> ---
>  fs/nfsd/blocklayoutxdr.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayoutxdr.c b/fs/nfsd/blocklayoutxdr.c
> index ce78f74715ee..66f75ee70db3 100644
> --- a/fs/nfsd/blocklayoutxdr.c
> +++ b/fs/nfsd/blocklayoutxdr.c
> @@ -121,19 +121,19 @@ nfsd4_block_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  
>  	if (len < sizeof(u32)) {
>  		dprintk("%s: extent array too small: %u\n", __func__, len);
> -		return -EINVAL;
> +		return -NFS4ERR_BADXDR;
>  	}
>  	len -= sizeof(u32);
>  	if (len % PNFS_BLOCK_EXTENT_SIZE) {
>  		dprintk("%s: extent array invalid: %u\n", __func__, len);
> -		return -EINVAL;
> +		return -NFS4ERR_BADXDR;
>  	}
>  
>  	nr_iomaps = be32_to_cpup(p++);
>  	if (nr_iomaps != len / PNFS_BLOCK_EXTENT_SIZE) {
>  		dprintk("%s: extent array size mismatch: %u/%u\n",
>  			__func__, len, nr_iomaps);
> -		return -EINVAL;
> +		return -NFS4ERR_BADXDR;
>  	}
>  
>  	iomaps = kcalloc(nr_iomaps, sizeof(*iomaps), GFP_KERNEL);
> @@ -181,7 +181,7 @@ nfsd4_block_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  	return nr_iomaps;
>  fail:
>  	kfree(iomaps);
> -	return -EINVAL;
> +	return -NFS4ERR_BADXDR;
>  }
>  
>  int
> @@ -193,7 +193,7 @@ nfsd4_scsi_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  
>  	if (len < sizeof(u32)) {
>  		dprintk("%s: extent array too small: %u\n", __func__, len);
> -		return -EINVAL;
> +		return -NFS4ERR_BADXDR;
>  	}
>  
>  	nr_iomaps = be32_to_cpup(p++);
> @@ -201,7 +201,7 @@ nfsd4_scsi_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  	if (len != expected) {
>  		dprintk("%s: extent array size mismatch: %u/%u\n",
>  			__func__, len, expected);
> -		return -EINVAL;
> +		return -NFS4ERR_BADXDR;
>  	}
>  
>  	iomaps = kcalloc(nr_iomaps, sizeof(*iomaps), GFP_KERNEL);
> @@ -232,5 +232,5 @@ nfsd4_scsi_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  	return nr_iomaps;
>  fail:
>  	kfree(iomaps);
> -	return -EINVAL;
> +	return -NFS4ERR_BADXDR;
>  }

nfsd4_block_decode_layoutupdate()'s only caller is
nfsd4_block_proc_layoutcommit(), which takes an error return and passes
it directly to nfserrno(). If nfserrno() gets a negative NFS4ERR status
value, it will bark. Changing only the return values is not enough.

Instead, nfsd4_block_decode_layoutupdate() needs to return /only/ a
positive or zero I/O map count or a negative NFS4ERR status code. Then,
if nfsd4_block_proc_layoutcommit() sees a negative return value, it
converts it to an nfserr by calling cpu_to_be32() on it.

(The -ENOMEM can be converted to -NFS4ERR_DELAY).

It would also help to get a nice kdoc comment added in front of
nfsd4_block_decode_layoutupdate() that explains the return value
convention.

I see that nfsd4_scsi_decode_layoutupdate() needs the same treatment.


-- 
Chuck Lever