fs/nfs/blocklayout/extent_tree.c | 5 +++++ 1 file changed, 5 insertions(+)
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.
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@mandelbit.com>
---
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) {
+ spin_unlock(&bl->bl_ext_lock);
+ return ret;
+ }
break;
}
--
2.49.1On 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@mandelbit.com>
> ---
> 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.
diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c
index 315949a7e92d..e80f2f82378f 100644
--- a/fs/nfs/blocklayout/extent_tree.c
+++ b/fs/nfs/blocklayout/extent_tree.c
@@ -588,6 +588,12 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p,
struct pnfs_block_extent *be, *be_prev;
int ret = 0;
+ if (ext_tree_layoutupdate_size(bl, 1) > buffer_size) {
+ *count = 0;
+ *lastbyte = U64_MAX;
+ return -ENOSPC;
+ }
+
spin_lock(&bl->bl_ext_lock);
for (be = ext_tree_first(&bl->bl_ext_rw); be; be = ext_tree_next(be)) {
if (be->be_state != PNFS_BLOCK_INVALID_DATA ||
--
Sergey Bashirov
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@mandelbit.com>
> > ---
> > 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
On 17/07/2025 06:56, Dan Carpenter wrote:
> 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.
Ok. It wasn't clear to me that
657 buffer_size = NFS_SERVER(arg->inode)->wsize;
would always set buffer_size to something large enough to encode at
least one block extent.
>>
>>> 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@mandelbit.com>
>>> ---
>>> 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. */
>
I agree a comment would help.
> Another option would be to initialize the be_prev to NULL. This will
> silence the uninitialized variable warning.
But will likely trigger a potential NULL-ptr-deref, because the static
analyzer believes we can get there with count==0.
> 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.
>
Thanks for the hint about setting CONFIG_INIT_STACK_ALL_PATTERN=y.
I'll add it to our test system as well.
Regards,
--
Antonio Quartulli
CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com
On Thu, Jul 17, 2025 at 10:01:42AM +0200, Antonio Quartulli wrote: > > I agree a comment would help. > > > Another option would be to initialize the be_prev to NULL. This will > > silence the uninitialized variable warning. > > But will likely trigger a potential NULL-ptr-deref, because the static > analyzer believes we can get there with count==0. > I don't know how Coverity does this. In my experience, writing Smatch I had to treat initializations to NULL as "ignore this variable". We used to have an uninitialized_var() macro to silence uninitialized variables. It did an assignment to itself something like: #define uninitialized_var(x) x = x But we removed it and changed all those places to just initialize the variables to zero. Even before, initializing things to zero was the standard way to silence GCC uninitialized variable warnings, so warning about NULL pointer dereferences tended to be prone to false positives and the worst kind of really complicated false positives too. regards, dan carpenter
Hi Antonio, Dan, I have an idea how to refactor the code a little to make the static analyzer happy. Will send a patch soon, but first I want to check that it won't affect functionality. On Thu, Jul 17, 2025 at 10:01:42AM +0200, Antonio Quartulli wrote: > On 17/07/2025 06:56, Dan Carpenter wrote: > > 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. */ > > > > I agree a comment would help. I see, will add a comment. > > Another option would be to initialize the be_prev to NULL. This will > > silence the uninitialized variable warning. > > But will likely trigger a potential NULL-ptr-deref, because the static > analyzer believes we can get there with count==0. I agree, this will most likely result in another warning. -- Sergey Bashirov
© 2016 - 2025 Red Hat, Inc.