Linux NFS development
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sergey Bashirov <sergeybashirov@gmail.com>
Cc: linux-nfs@vger.kernel.org
Subject: [bug report] nfsd: Implement large extent array support in pNFS
Date: Wed, 16 Jul 2025 14:40:55 -0500	[thread overview]
Message-ID: <9892f785-e9f5-4a29-9ff7-fd89dbf7e474@sabinyo.mountain> (raw)

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.

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 }


             reply	other threads:[~2025-07-16 19:40 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-16 19:40 Dan Carpenter [this message]
2025-07-16 20:31 ` [bug report] nfsd: Implement large extent array support in pNFS Chuck Lever
2025-07-17  9:51   ` Sergey Bashirov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9892f785-e9f5-4a29-9ff7-fd89dbf7e474@sabinyo.mountain \
    --to=dan.carpenter@linaro.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=sergeybashirov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox