On Thu, Jul 17, 2025 at 03:27:50AM +0300, Sergey Bashirov wrote:
> On Wed, Jul 16, 2025 at 04:38:48PM +0200, Antonio Quartulli wrote:
> > In ext_tree_encode_commit() if no block extent is encoded due to lack
> > of buffer space, ret is set to -ENOSPC and we end up accessing be_prev
> > despite it being uninitialized.
>
> This static check warning appears to be a false positive. This is an
> internal static function that is not exported outside the module via
> an interface or API. Inside the module we always use a buffer size
> that is a multiple of PAGE_SIZE, so at least one page is provided.
> The block extent size does not exceed 44 bytes, so we can always
> encode at least one extent. Thus, we never fail on the first iteration.
> Either ret is zero, or ret is nonzero and at least one extent is encoded.
>
> > Fix this behaviour by bailing out right away when no extent is encoded.
> >
> > Fixes: d84c4754f874 ("pNFS: Fix extent encoding in block/scsi layout")
> > Addresses-Coverity-ID: 1647611 ("Memory - illegal accesses (UNINIT)")
> > Signed-off-by: Antonio Quartulli <antonio@xxxxxxxxxxxxx>
> > ---
> > fs/nfs/blocklayout/extent_tree.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c
> > index 315949a7e92d..82e19205f425 100644
> > --- a/fs/nfs/blocklayout/extent_tree.c
> > +++ b/fs/nfs/blocklayout/extent_tree.c
> > @@ -598,6 +598,11 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p,
> > if (ext_tree_layoutupdate_size(bl, *count) > buffer_size) {
> > (*count)--;
> > ret = -ENOSPC;
> > + /* bail out right away if no extent was encoded */
> > + if (!*count) {
>
> We can't exit here without setting the value of lastbyte, which is one
> of the function outputs. Please set it to U64_MAX to let upper layer
> logic handle it properly. Or, see the alternative solution at the end.
> + *lastbyte = U64_MAX;
>
> > + spin_unlock(&bl->bl_ext_lock);
> > + return ret;
> > + }
> > break;
> > }
> >
>
> If we need to fix this, I'd rather add an early check whether the buffer
> size is large enough to encode at least one extent at the beginning of
> the function. Before spinlock is acquired and ext_tree traversed. This
> looks more natural to me. But I'm not sure if this will satisfy the
> static checker.
>
No, it won't. I feel like the code is confusing enough that maybe a
comment is warranted. /* We always iterate through the loop at least
once so be_prev is correct. */
Another option would be to initialize the be_prev to NULL. This will
silence the uninitialized variable warning. And everyone sensible runs
with CONFIG_INIT_STACK_ALL_ZERO set in production so it doesn't affect
run time at all. Btw, we changed our test systems to set
CONFIG_INIT_STACK_ALL_PATTERN=y a few months ago and immediately ran
into an uninitialized variable bug. So I've heard there are a couple
distros which don't set CONFIG_INIT_STACK_ALL_ZERO which is very daring
when every single developer is testing with uninitialized variables
defaulting to zero.
regards,
dan carpenter
