[PATCH] nfsd: Use correct error code when decoding extents

[PATCH] nfsd: Use correct error code when decoding extents

[Sergey Bashirov]

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@gmail.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;
 }
-- 
2.43.0

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

[Chuck Lever]

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@gmail.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

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

[Sergey Bashirov]

I also have some doubts about this code:
if (xdr_stream_decode_u64(&xdr, &bex.len))
         return -NFS4ERR_BADXDR;
if (bex.len & (block_size - 1))
         return -NFS4ERR_BADXDR;

The first error code is clear to me, it is all about decoding. But should
not we return -NFS4ERR_EINVAL in the second check? On one hand, we
encountered an invalid value after successful decoding, but on the other
hand, we stopped decoding the extent array, so we can say that this is
also a decoding error.

--
Sergey Bashirov

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

[Chuck Lever]

On 6/11/25 12:24 PM, Sergey Bashirov wrote:
> I also have some doubts about this code:
> if (xdr_stream_decode_u64(&xdr, &bex.len))
>         return -NFS4ERR_BADXDR;
> if (bex.len & (block_size - 1))
>         return -NFS4ERR_BADXDR;
> 
> The first error code is clear to me, it is all about decoding. But should
> not we return -NFS4ERR_EINVAL in the second check? On one hand, we
> encountered an invalid value after successful decoding, but on the other
> hand, we stopped decoding the extent array, so we can say that this is
> also a decoding error.

On first read of Section 2.3 of RFC 5663, there's no mandated alignment
requirement for bex_length. IMO this is a case where the implementation
is deciding that a decoded value is not valid, so NFS4ERR_INVAL might be
a better choice here.


-- 
Chuck Lever

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

[Christoph Hellwig]

On Wed, Jun 11, 2025 at 12:29:51PM -0400, Chuck Lever wrote:
> On 6/11/25 12:24 PM, Sergey Bashirov wrote:
> > I also have some doubts about this code:
> > if (xdr_stream_decode_u64(&xdr, &bex.len))
> >         return -NFS4ERR_BADXDR;
> > if (bex.len & (block_size - 1))
> >         return -NFS4ERR_BADXDR;
> > 
> > The first error code is clear to me, it is all about decoding. But should
> > not we return -NFS4ERR_EINVAL in the second check? On one hand, we
> > encountered an invalid value after successful decoding, but on the other
> > hand, we stopped decoding the extent array, so we can say that this is
> > also a decoding error.
> 
> On first read of Section 2.3 of RFC 5663, there's no mandated alignment
> requirement for bex_length. IMO this is a case where the implementation
> is deciding that a decoded value is not valid, so NFS4ERR_INVAL might be
> a better choice here.

Section 2.1 of RFC 5663 says:

Clients must be able to perform I/O to the block extents without 
affecting additional areas of storage (especially important for writes);
therefore, extents MUST be aligned to 512-byte boundaries, and writable
extents MUST be aligned to the block size used by the NFSv4 server in
managing the actual file system (4 kilobytes and 8 kilobytes are common
block sizes).  This block size is available as the NFSv4.1 layout_blksize
attribute.

While it would be nice to state this again in 2.3, the language looks
normative enough (TM) to me.

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

[Chuck Lever]

On 6/16/25 8:38 AM, Christoph Hellwig wrote:
> On Wed, Jun 11, 2025 at 12:29:51PM -0400, Chuck Lever wrote:
>> On 6/11/25 12:24 PM, Sergey Bashirov wrote:
>>> I also have some doubts about this code:
>>> if (xdr_stream_decode_u64(&xdr, &bex.len))
>>>         return -NFS4ERR_BADXDR;
>>> if (bex.len & (block_size - 1))
>>>         return -NFS4ERR_BADXDR;
>>>
>>> The first error code is clear to me, it is all about decoding. But should
>>> not we return -NFS4ERR_EINVAL in the second check? On one hand, we
>>> encountered an invalid value after successful decoding, but on the other
>>> hand, we stopped decoding the extent array, so we can say that this is
>>> also a decoding error.
>>
>> On first read of Section 2.3 of RFC 5663, there's no mandated alignment
>> requirement for bex_length. IMO this is a case where the implementation
>> is deciding that a decoded value is not valid, so NFS4ERR_INVAL might be
>> a better choice here.
> 
> Section 2.1 of RFC 5663 says:
> 
> Clients must be able to perform I/O to the block extents without 
> affecting additional areas of storage (especially important for writes);
> therefore, extents MUST be aligned to 512-byte boundaries, and writable
> extents MUST be aligned to the block size used by the NFSv4 server in
> managing the actual file system (4 kilobytes and 8 kilobytes are common
> block sizes).  This block size is available as the NFSv4.1 layout_blksize
> attribute.
> 
> While it would be nice to state this again in 2.3, the language looks
> normative enough (TM) to me.

No argument from me.

Passing in a non-aligned bex_length still doesn't seem to me to be an
XDR issue. Failing with NFS4ERR_INVAL seems like the correct server
response for this situation.


-- 
Chuck Lever

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

[Christoph Hellwig]

On Mon, Jun 16, 2025 at 09:21:27AM -0400, Chuck Lever wrote:
> Passing in a non-aligned bex_length still doesn't seem to me to be an
> XDR issue. Failing with NFS4ERR_INVAL seems like the correct server
> response for this situation.

Fine with me.