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

[PATCH v2] 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 struct 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 deviceid4 structure
and removes the pad field, which is currently not used by NFSD, from the
runtime state. The server's byte order is preserved because the deviceid4
blob on the wire is only used as a cookie by the client.

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

Changes in v2:
 - Renamed new functions
 - Removed byte swap operations
 - Updated code comment and patch description

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

diff --git a/fs/nfsd/blocklayoutxdr.c b/fs/nfsd/blocklayoutxdr.c
index bcf21fde91207..18de37ff28916 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 = svcxdr_encode_deviceid4(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 = svcxdr_decode_deviceid4(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..f9f7e38cba13f 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 = svcxdr_encode_deviceid4(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..2acc9abee668f 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_decode_deviceid4(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..e65b552bf5f5a 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -595,9 +595,43 @@ struct nfsd4_reclaim_complete {
 struct nfsd4_deviceid {
 	u64			fsid_idx;
 	u32			generation;
-	u32			pad;
 };
 
+static inline __be32 *
+svcxdr_encode_deviceid4(__be32 *p, const struct nfsd4_deviceid *devid)
+{
+	__be64 *q = (__be64 *)p;
+
+	*q = (__force __be64)devid->fsid_idx;
+	p += 2;
+	*p++ = (__force __be32)devid->generation;
+	*p++ = xdr_zero;
+	return p;
+}
+
+static inline __be32 *
+svcxdr_decode_deviceid4(__be32 *p, struct nfsd4_deviceid *devid)
+{
+	__be64 *q = (__be64 *)p;
+
+	devid->fsid_idx = (__force u64)(*q);
+	p += 2;
+	devid->generation = (__force u32)(*p++);
+	p++; /* NFSD does not use the remaining octets */
+	return p;
+}
+
+static inline __be32
+nfsd4_decode_deviceid4(struct xdr_stream *xdr, struct nfsd4_deviceid *devid)
+{
+	__be32 *p = xdr_inline_decode(xdr, NFS4_DEVICEID4_SIZE);
+
+	if (unlikely(!p))
+		return nfserr_bad_xdr;
+	svcxdr_decode_deviceid4(p, devid);
+	return nfs_ok;
+}
+
 struct nfsd4_layout_seg {
 	u32			iomode;
 	u64			offset;
-- 
2.43.0

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

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Mon, 21 Jul 2025 17:48:55 +0300, Sergey Bashirov wrote:
> Compilers may optimize the layout of C structures, so we should not rely
> on sizeof struct 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 deviceid4 structure
> and removes the pad field, which is currently not used by NFSD, from the
> runtime state. The server's byte order is preserved because the deviceid4
> blob on the wire is only used as a cookie by the client.
> 
> [...]

Applied to nfsd-testing, thanks!

Note that:

- nfsd-implement-large-extent-array-support-in-pnfs
- nfsd-fix-last-write-offset-handling-in-layoutcommit

have been dropped from nfsd-testing because of conflicts with
this patch. Please rebase these two on the current nfsd-testing
branch and resend.

[1/1] NFSD: Rework encoding and decoding of nfsd4_deviceid
      commit: 52d6381bf1caa63f6cd8b3df54162f36b03b3a68

--
Chuck Lever

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

[Christoph Hellwig]

On Mon, Jul 21, 2025 at 05:48:55PM +0300, Sergey Bashirov wrote:
> Compilers may optimize the layout of C structures,

By interpreting the standard in the most hostile way: yes.
In practice: no.

Just about every file system on-disk format and every network wire
protocol depends on the compiler not "optimizing" properly padded
C structures.

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

[Chuck Lever]

On 7/22/25 1:36 AM, Christoph Hellwig wrote:
> On Mon, Jul 21, 2025 at 05:48:55PM +0300, Sergey Bashirov wrote:
>> Compilers may optimize the layout of C structures,
> 
> By interpreting the standard in the most hostile way: yes.
> In practice: no.

Earnest question: Is NFSD/XDR properly insulated against the "randomize
structure layout" option?


> Just about every file system on-disk format and every network wire
> protocol depends on the compiler not "optimizing" properly padded
> C structures.
It's an intrinsic assumption that is not documented in the code or
anywhere else. IMO that is a latent banana peel.

While not urgent, this is a defensive change and it improves code
portability amongst compilers and languages (eg, Rust).


-- 
Chuck Lever

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

[NeilBrown]

On Wed, 23 Jul 2025, Chuck Lever wrote:
> On 7/22/25 1:36 AM, Christoph Hellwig wrote:
> > On Mon, Jul 21, 2025 at 05:48:55PM +0300, Sergey Bashirov wrote:
> >> Compilers may optimize the layout of C structures,
> > 
> > By interpreting the standard in the most hostile way: yes.
> > In practice: no.
> 
> Earnest question: Is NFSD/XDR properly insulated against the "randomize
> structure layout" option?
> 
> 
> > Just about every file system on-disk format and every network wire
> > protocol depends on the compiler not "optimizing" properly padded
> > C structures.
> It's an intrinsic assumption that is not documented in the code or
> anywhere else. IMO that is a latent banana peel.

We could document it in the code with __no_randomize_layout after the
structure definition.

But currently the only structures that are randomized in Linux are those
which are entirely function pointers, and those marked
__randomize_layout.

(See documentation for "RANDSTRUCT_FULL")

> 
> While not urgent, this is a defensive change and it improves code
> portability amongst compilers and languages (eg, Rust).
> 

I'm neither for nor against the change.  I would be interested to know
how much it changes the code side (if at all).

NeilBrown

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

[Chuck Lever]

On 7/22/25 9:28 PM, NeilBrown wrote:
> On Wed, 23 Jul 2025, Chuck Lever wrote:
>> On 7/22/25 1:36 AM, Christoph Hellwig wrote:
>>> On Mon, Jul 21, 2025 at 05:48:55PM +0300, Sergey Bashirov wrote:
>>>> Compilers may optimize the layout of C structures,
>>>
>>> By interpreting the standard in the most hostile way: yes.
>>> In practice: no.
>>
>> Earnest question: Is NFSD/XDR properly insulated against the "randomize
>> structure layout" option?
>>
>>
>>> Just about every file system on-disk format and every network wire
>>> protocol depends on the compiler not "optimizing" properly padded
>>> C structures.
>> It's an intrinsic assumption that is not documented in the code or
>> anywhere else. IMO that is a latent banana peel.
> 
> We could document it in the code with __no_randomize_layout after the
> structure definition.

We might also want __attribute__((packed)) or even
__attribute__((packed, aligned(4))).

That still leaves undocumented the fact that the fields in the structure
are treated as both endian types. In most other XDR functions we have
been careful to write source code that shows where endianness changes
or, conversely, where endianness is not consequential.


> But currently the only structures that are randomized in Linux are those
> which are entirely function pointers, and those marked
> __randomize_layout.
> 
> (See documentation for "RANDSTRUCT_FULL")
> 
>>
>> While not urgent, this is a defensive change and it improves code
>> portability amongst compilers and languages (eg, Rust).
>>
> 
> I'm neither for nor against the change.  I would be interested to know
> how much it changes the code side (if at all).
> 
> NeilBrown
> 


-- 
Chuck Lever

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

[Christoph Hellwig]

On Wed, Jul 23, 2025 at 10:35:29AM -0400, Chuck Lever wrote:
> > We could document it in the code with __no_randomize_layout after the
> > structure definition.
> 
> We might also want __attribute__((packed)) or even
> __attribute__((packed, aligned(4))).

Absolute not.  packed causes horrible code generation, and
__attribute__((packed, aligned(4))) will probably break things
that didn't properly pad (although they really should).

> That still leaves undocumented the fact that the fields in the structure
> are treated as both endian types. In most other XDR functions we have
> been careful to write source code that shows where endianness changes
> or, conversely, where endianness is not consequential.

I'm not arguing against doing XDR things in XDR code.  But the rest of
the kernel works very different.