[PATCH] NFSD: Rework encoding and decoding of nfsd4_deviceid

[PATCH] NFSD: Rework encoding and decoding of nfsd4_deviceid

[Sergey Bashirov]

Compilers may optimize the layout of C structures, so we should not rely
on sizeof and memcpy to encode and decode XDR structures. The byte order
of the fields should also be taken into account. This patch adds the
correct functions to handle the nfsd4_deviceid structure and removes the
pad field, which is currently unused.

Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
---
 Tested on the block layout setup, checked with smatch.

 fs/nfsd/blocklayoutxdr.c    |  7 ++-----
 fs/nfsd/flexfilelayoutxdr.c |  3 +--
 fs/nfsd/nfs4layouts.c       |  1 -
 fs/nfsd/nfs4xdr.c           | 14 +-------------
 fs/nfsd/xdr4.h              | 31 ++++++++++++++++++++++++++++++-
 5 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/fs/nfsd/blocklayoutxdr.c b/fs/nfsd/blocklayoutxdr.c
index bcf21fde91207..9ff2a23470e61 100644
--- a/fs/nfsd/blocklayoutxdr.c
+++ b/fs/nfsd/blocklayoutxdr.c
@@ -29,8 +29,7 @@ nfsd4_block_encode_layoutget(struct xdr_stream *xdr,
 	*p++ = cpu_to_be32(len);
 	*p++ = cpu_to_be32(1);		/* we always return a single extent */
 
-	p = xdr_encode_opaque_fixed(p, &b->vol_id,
-			sizeof(struct nfsd4_deviceid));
+	p = nfsd4_encode_deviceid(p, &b->vol_id);
 	p = xdr_encode_hyper(p, b->foff);
 	p = xdr_encode_hyper(p, b->len);
 	p = xdr_encode_hyper(p, b->soff);
@@ -156,9 +155,7 @@ nfsd4_block_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
 	for (i = 0; i < nr_iomaps; i++) {
 		struct pnfs_block_extent bex;
 
-		memcpy(&bex.vol_id, p, sizeof(struct nfsd4_deviceid));
-		p += XDR_QUADLEN(sizeof(struct nfsd4_deviceid));
-
+		p = nfsd4_decode_deviceid(p, &bex.vol_id);
 		p = xdr_decode_hyper(p, &bex.foff);
 		if (bex.foff & (block_size - 1)) {
 			goto fail;
diff --git a/fs/nfsd/flexfilelayoutxdr.c b/fs/nfsd/flexfilelayoutxdr.c
index aeb71c10ff1b9..28eb5bedb7e13 100644
--- a/fs/nfsd/flexfilelayoutxdr.c
+++ b/fs/nfsd/flexfilelayoutxdr.c
@@ -54,8 +54,7 @@ nfsd4_ff_encode_layoutget(struct xdr_stream *xdr,
 	*p++ = cpu_to_be32(1);			/* single mirror */
 	*p++ = cpu_to_be32(1);			/* single data server */
 
-	p = xdr_encode_opaque_fixed(p, &fl->deviceid,
-			sizeof(struct nfsd4_deviceid));
+	p = nfsd4_encode_deviceid(p, &fl->deviceid);
 
 	*p++ = cpu_to_be32(1);			/* efficiency */
 
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index aea905fcaf87a..683bd1130afe2 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -120,7 +120,6 @@ nfsd4_set_deviceid(struct nfsd4_deviceid *id, const struct svc_fh *fhp,
 
 	id->fsid_idx = fhp->fh_export->ex_devid_map->idx;
 	id->generation = device_generation;
-	id->pad = 0;
 	return 0;
 }
 
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ea91bad4eee2c..f3a089df164c6 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -587,18 +587,6 @@ nfsd4_decode_state_owner4(struct nfsd4_compoundargs *argp,
 }
 
 #ifdef CONFIG_NFSD_PNFS
-static __be32
-nfsd4_decode_deviceid4(struct nfsd4_compoundargs *argp,
-		       struct nfsd4_deviceid *devid)
-{
-	__be32 *p;
-
-	p = xdr_inline_decode(argp->xdr, NFS4_DEVICEID4_SIZE);
-	if (!p)
-		return nfserr_bad_xdr;
-	memcpy(devid, p, sizeof(*devid));
-	return nfs_ok;
-}
 
 static __be32
 nfsd4_decode_layoutupdate4(struct nfsd4_compoundargs *argp,
@@ -1783,7 +1771,7 @@ nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp,
 	__be32 status;
 
 	memset(gdev, 0, sizeof(*gdev));
-	status = nfsd4_decode_deviceid4(argp, &gdev->gd_devid);
+	status = nfsd4_stream_decode_deviceid(argp->xdr, &gdev->gd_devid);
 	if (status)
 		return status;
 	if (xdr_stream_decode_u32(argp->xdr, &gdev->gd_layout_type) < 0)
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index a23bc56051caf..ec70cb9c17788 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -595,9 +595,38 @@ struct nfsd4_reclaim_complete {
 struct nfsd4_deviceid {
 	u64			fsid_idx;
 	u32			generation;
-	u32			pad;
 };
 
+static inline __be32 *
+nfsd4_encode_deviceid(__be32 *p, const struct nfsd4_deviceid *devid)
+{
+	p = xdr_encode_hyper(p, devid->fsid_idx);
+	*p++ = cpu_to_be32(devid->generation);
+	*p++ = cpu_to_be32(0); /* pad field is currently unused */
+	return p;
+}
+
+static inline __be32 *
+nfsd4_decode_deviceid(__be32 *p, struct nfsd4_deviceid *devid)
+{
+	p = xdr_decode_hyper(p, &devid->fsid_idx);
+	devid->generation = be32_to_cpup(p++);
+	p++; /* pad field is currently unused */
+	return p;
+}
+
+static inline __be32
+nfsd4_stream_decode_deviceid(struct xdr_stream *xdr,
+			     struct nfsd4_deviceid *devid)
+{
+	__be32 *p = xdr_inline_decode(xdr, NFS4_DEVICEID4_SIZE);
+
+	if (unlikely(!p))
+		return nfserr_bad_xdr;
+	nfsd4_decode_deviceid(p, devid);
+	return nfs_ok;
+}
+
 struct nfsd4_layout_seg {
 	u32			iomode;
 	u64			offset;
-- 
2.43.0

Re: [PATCH] NFSD: Rework encoding and decoding of nfsd4_deviceid

[Chuck Lever]

On 7/19/25 7:37 AM, Sergey Bashirov wrote:
> Compilers may optimize the layout of C structures, so we should not rely
> on sizeof and memcpy to encode and decode XDR structures. The byte order
> of the fields should also be taken into account. This patch adds the
> correct functions to handle the nfsd4_deviceid structure and removes the
> pad field, which is currently unused.
> 
> Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>

Very thorough. At first I didn't think we would need to update the other
layout types, but yes, I now agree that's prudent to do.

A few nits below.


> ---
>  Tested on the block layout setup, checked with smatch.
> 
>  fs/nfsd/blocklayoutxdr.c    |  7 ++-----
>  fs/nfsd/flexfilelayoutxdr.c |  3 +--
>  fs/nfsd/nfs4layouts.c       |  1 -
>  fs/nfsd/nfs4xdr.c           | 14 +-------------
>  fs/nfsd/xdr4.h              | 31 ++++++++++++++++++++++++++++++-
>  5 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayoutxdr.c b/fs/nfsd/blocklayoutxdr.c
> index bcf21fde91207..9ff2a23470e61 100644
> --- a/fs/nfsd/blocklayoutxdr.c
> +++ b/fs/nfsd/blocklayoutxdr.c
> @@ -29,8 +29,7 @@ nfsd4_block_encode_layoutget(struct xdr_stream *xdr,
>  	*p++ = cpu_to_be32(len);
>  	*p++ = cpu_to_be32(1);		/* we always return a single extent */
>  
> -	p = xdr_encode_opaque_fixed(p, &b->vol_id,
> -			sizeof(struct nfsd4_deviceid));
> +	p = nfsd4_encode_deviceid(p, &b->vol_id);
>  	p = xdr_encode_hyper(p, b->foff);
>  	p = xdr_encode_hyper(p, b->len);
>  	p = xdr_encode_hyper(p, b->soff);
> @@ -156,9 +155,7 @@ nfsd4_block_decode_layoutupdate(__be32 *p, u32 len, struct iomap **iomapp,
>  	for (i = 0; i < nr_iomaps; i++) {
>  		struct pnfs_block_extent bex;
>  
> -		memcpy(&bex.vol_id, p, sizeof(struct nfsd4_deviceid));
> -		p += XDR_QUADLEN(sizeof(struct nfsd4_deviceid));
> -
> +		p = nfsd4_decode_deviceid(p, &bex.vol_id);
>  		p = xdr_decode_hyper(p, &bex.foff);
>  		if (bex.foff & (block_size - 1)) {
>  			goto fail;
> diff --git a/fs/nfsd/flexfilelayoutxdr.c b/fs/nfsd/flexfilelayoutxdr.c
> index aeb71c10ff1b9..28eb5bedb7e13 100644
> --- a/fs/nfsd/flexfilelayoutxdr.c
> +++ b/fs/nfsd/flexfilelayoutxdr.c
> @@ -54,8 +54,7 @@ nfsd4_ff_encode_layoutget(struct xdr_stream *xdr,
>  	*p++ = cpu_to_be32(1);			/* single mirror */
>  	*p++ = cpu_to_be32(1);			/* single data server */
>  
> -	p = xdr_encode_opaque_fixed(p, &fl->deviceid,
> -			sizeof(struct nfsd4_deviceid));
> +	p = nfsd4_encode_deviceid(p, &fl->deviceid);
>  
>  	*p++ = cpu_to_be32(1);			/* efficiency */
>  
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index aea905fcaf87a..683bd1130afe2 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -120,7 +120,6 @@ nfsd4_set_deviceid(struct nfsd4_deviceid *id, const struct svc_fh *fhp,
>  
>  	id->fsid_idx = fhp->fh_export->ex_devid_map->idx;
>  	id->generation = device_generation;
> -	id->pad = 0;
>  	return 0;
>  }
>  
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ea91bad4eee2c..f3a089df164c6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -587,18 +587,6 @@ nfsd4_decode_state_owner4(struct nfsd4_compoundargs *argp,
>  }
>  
>  #ifdef CONFIG_NFSD_PNFS
> -static __be32
> -nfsd4_decode_deviceid4(struct nfsd4_compoundargs *argp,
> -		       struct nfsd4_deviceid *devid)
> -{
> -	__be32 *p;
> -
> -	p = xdr_inline_decode(argp->xdr, NFS4_DEVICEID4_SIZE);
> -	if (!p)
> -		return nfserr_bad_xdr;
> -	memcpy(devid, p, sizeof(*devid));
> -	return nfs_ok;
> -}
>  
>  static __be32
>  nfsd4_decode_layoutupdate4(struct nfsd4_compoundargs *argp,
> @@ -1783,7 +1771,7 @@ nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp,
>  	__be32 status;
>  
>  	memset(gdev, 0, sizeof(*gdev));
> -	status = nfsd4_decode_deviceid4(argp, &gdev->gd_devid);
> +	status = nfsd4_stream_decode_deviceid(argp->xdr, &gdev->gd_devid);
>  	if (status)
>  		return status;
>  	if (xdr_stream_decode_u32(argp->xdr, &gdev->gd_layout_type) < 0)
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index a23bc56051caf..ec70cb9c17788 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -595,9 +595,38 @@ struct nfsd4_reclaim_complete {
>  struct nfsd4_deviceid {
>  	u64			fsid_idx;
>  	u32			generation;
> -	u32			pad;
>  };
>  
> +static inline __be32 *
> +nfsd4_encode_deviceid(__be32 *p, const struct nfsd4_deviceid *devid)

Let's call this one svcxdr_encode_deviceid4()

(Note the "4" on the end, which matches the name of the XDR type as
it is specified in RFC 8881).


> +{
> +	p = xdr_encode_hyper(p, devid->fsid_idx);
> +	*p++ = cpu_to_be32(devid->generation);
> +	*p++ = cpu_to_be32(0); /* pad field is currently unused */
> +	return p;
> +}

The byte swap operations are unnecessary here. The deviceid4 blob on the
wire is not used by clients in any way other than as a cookie. So the
content of the blob can remain in the same endian-ness as the server.

What we do want is to document that fact. The usual way to do that is
with "(__force xxx)". Maybe we want to do something clever with a union
type, but here's something quick, dirty, and untested:

{
	__be64 *q = (__be64)p;

	*q = (__force __be64)devid->fsid_idx;
	*p += 2;
	*p++ (__force __be32)devid->generation;
	*p++ = xdr_zero;
	return p;
}

For the new "is unused" comment, IMHO it's not adding much value. But if
you want to keep it, let's make it active voice so it's clear this is an
peculiarity of our implementation, rather than a protocol requirement.
Something like:

	/* NFSD does not use the remaining octets */


> +
> +static inline __be32 *
> +nfsd4_decode_deviceid(__be32 *p, struct nfsd4_deviceid *devid)

This one should be named svcxdr_decode_deviceid4()


> +{
> +	p = xdr_decode_hyper(p, &devid->fsid_idx);
> +	devid->generation = be32_to_cpup(p++);
> +	p++; /* pad field is currently unused */
> +	return p;
> +}

Likewise, replace the byte-swaps with forced type casts here, and fix up
the new comment.


> +
> +static inline __be32
> +nfsd4_stream_decode_deviceid(struct xdr_stream *xdr,
> +			     struct nfsd4_deviceid *devid)

And now this one can be called nfsd4_decode_deviceid4(), which matches
the naming scheme of the helpers at the top of fs/nfsd/xdr4.h.


> +{
> +	__be32 *p = xdr_inline_decode(xdr, NFS4_DEVICEID4_SIZE);
> +
> +	if (unlikely(!p))
> +		return nfserr_bad_xdr;
> +	nfsd4_decode_deviceid(p, devid);
> +	return nfs_ok;
> +}
> +
>  struct nfsd4_layout_seg {
>  	u32			iomode;
>  	u64			offset;


-- 
Chuck Lever