On 7/16/25 3:40 PM, Dan Carpenter wrote:
> Hello Sergey Bashirov,
>
> Commit 067b594beb16 ("nfsd: Implement large extent array support in
> pNFS") from Jun 21, 2025 (linux-next), leads to the following Smatch
> static checker warning:
>
> fs/nfsd/blocklayoutxdr.c:160 nfsd4_block_decode_layoutupdate()
> warn: error code type promoted to positive: 'ret'
>
> fs/nfsd/blocklayoutxdr.c
> 135 nfsd4_block_decode_layoutupdate(struct xdr_stream *xdr, struct iomap **iomapp,
> 136 int *nr_iomapsp, u32 block_size)
> 137 {
> 138 struct iomap *iomaps;
> 139 u32 nr_iomaps, expected, len, i;
> 140 __be32 nfserr;
> 141
> 142 if (xdr_stream_decode_u32(xdr, &nr_iomaps))
> 143 return nfserr_bad_xdr;
> 144
> 145 len = sizeof(__be32) + xdr_stream_remaining(xdr);
> 146 expected = sizeof(__be32) + nr_iomaps * PNFS_BLOCK_EXTENT_SIZE;
> 147 if (len != expected)
> 148 return nfserr_bad_xdr;
> 149
> 150 iomaps = kcalloc(nr_iomaps, sizeof(*iomaps), GFP_KERNEL);
> 151 if (!iomaps)
> 152 return nfserr_delay;
> 153
> 154 for (i = 0; i < nr_iomaps; i++) {
> 155 struct pnfs_block_extent bex;
> 156 ssize_t ret;
> 157
> 158 ret = xdr_stream_decode_opaque_fixed(xdr,
> 159 &bex.vol_id, sizeof(bex.vol_id));
> --> 160 if (ret < sizeof(bex.vol_id)) {
>
> xdr_stream_decode_opaque_fixed() returns negative error codes or
> sizeof(bex.vol_id) on success. If it returns a negative then the
> condition is type promoted to size_t and the negative becomes a high
> positive value and is treated as success.
>
> There are so many ways to fix this bug.
>
> #1: if (ret < 0 || ret < sizeof(bex.vol_id)) {
> I love this approach but every other person in the world seems to hate
> it.
>
> #2: if (ret < (int)sizeof(bex.vol_id)) {
> Fine. I don't love casts, but fine.
>
> #3: if (ret != sizeof(bex.vol_id)) {
> I like this well enough.
>
> #4: Change xdr_stream_decode_opaque_fixed() to return zero on success.
> This is the best fix.
Since the XDR field is fixed in size, the caller already knows how many
bytes were decoded, on success. Thus, xdr_stream_decode_opaque_fixed()
doesn't need to return that value. And, xdr_stream_decode_u32 and _u64
both return zero on success.
Since there is only one other caller, modifying the set of return values
of xdr_stream_decode_opaque_fixed() seems sensible to me.
But, I spot something else amiss here. Note that "sizeof(bex.vol_id)"
is the size of
595 struct nfsd4_deviceid {
596 u64 fsid_idx;
597 u32 generation;
598 u32 pad;
599 };
This is a C structure. The compiler is permitted to add padding, though
it probably doesn't in this simple case. Distributions are known to add
padding to internal structures to enable additions of new fields without
breaking ABI compatibility.
Thus there is no hard guarantee that sizeof(nfsd4_deviceid) will always
be exactly 16 bytes.
To ensure that the XDR stream is incremented the correct number of bytes
while decoding this field, the decoder needs to pass the size of the XDR
field, not the size of the in-memory C structure. Thankfully,
include/linux/nfs4.h has:
800 #define NFS4_DEVICEID4_SIZE 16
So let's use NFS4_DEVICEID4_SIZE rather than sizeof(bex.vol_id) when
rewriting the problematic decoder.
But that doesn't guarantee that the bex field (of type struct
nfsd4_deviceid) is large enough to receive the raw 16-byte XDR data. It
probably is, but why not write code that documents that assumption.
Maybe what this needs is a type-specific decoder for deviceid4 fields
that will take a char xx[16] off the wire and decode it properly into
the in-memory fsid_idx and generation fields in a struct nfsd4_deviceid.
We already do something similar for file handles.
> regards,
> dan carpenter
>
> 161 nfserr = nfserr_bad_xdr;
> 162 goto fail;
> 163 }
> 164
> 165 if (xdr_stream_decode_u64(xdr, &bex.foff)) {
> 166 nfserr = nfserr_bad_xdr;
> 167 goto fail;
> 168 }
> 169 if (bex.foff & (block_size - 1)) {
> 170 nfserr = nfserr_inval;
> 171 goto fail;
> 172 }
> 173
> 174 if (xdr_stream_decode_u64(xdr, &bex.len)) {
> 175 nfserr = nfserr_bad_xdr;
> 176 goto fail;
> 177 }
> 178 if (bex.len & (block_size - 1)) {
> 179 nfserr = nfserr_inval;
> 180 goto fail;
> 181 }
> 182
> 183 if (xdr_stream_decode_u64(xdr, &bex.soff)) {
> 184 nfserr = nfserr_bad_xdr;
> 185 goto fail;
> 186 }
> 187 if (bex.soff & (block_size - 1)) {
> 188 nfserr = nfserr_inval;
> 189 goto fail;
> 190 }
> 191
> 192 if (xdr_stream_decode_u32(xdr, &bex.es)) {
> 193 nfserr = nfserr_bad_xdr;
> 194 goto fail;
> 195 }
> 196 if (bex.es != PNFS_BLOCK_READWRITE_DATA) {
> 197 nfserr = nfserr_inval;
> 198 goto fail;
> 199 }
> 200
> 201 iomaps[i].offset = bex.foff;
> 202 iomaps[i].length = bex.len;
> 203 }
> 204
> 205 *iomapp = iomaps;
> 206 *nr_iomapsp = nr_iomaps;
> 207 return nfs_ok;
> 208 fail:
> 209 kfree(iomaps);
> 210 return nfserr;
> 211 }
>
>
--
Chuck Lever
