On 2023/10/27 18:22, Holger Hoffstätte wrote:
On 2023-10-27 09:00, Qu Wenruo wrote:
On 2023/10/27 17:25, Holger Hoffstätte wrote:
On 2023-10-26 23:01, Qu Wenruo wrote:
On 2023/10/27 00:30, Holger Hoffstätte wrote:
On 2023-10-26 15:31, Sam James wrote:
'btrfs: scrub: fix grouping of read IO' seems to intorduce a
-Wmaybe-uninitialized warning (which becomes fatal with the kernel's
passed -Werror=...) with 6.5.9:
```
/var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c: In function ‘scrub_simple_mirror.isra’:
/var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c:2075:29: error: ‘found_logical’ may be used uninitialized [-Werror=maybe-uninitialized[https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized]]
2075 | cur_logical = found_logical +
BTRFS_STRIPE_LEN;
/var/tmp/portage/sys-kernel/gentoo-kernel-6.5.9/work/linux-6.5/fs/btrfs/scrub.c:2040:21: note: ‘found_logical’ was declared here
2040 | u64 found_logical;
| ^~~~~~~~~~~~~
```
Good find! found_logical is passed by reference to
queue_scrub_stripe(..) (inlined)
where it is used without ever being set:
...
/* Either >0 as no more extents or <0 for error. */
if (ret)
return ret;
if (found_logical_ret)
*found_logical_ret = stripe->logical;
sctx->cur_stripe++;
...
Something is missing here, and somehow I don't think it's just the
top-level
initialisation of found_logical.
This looks like a false alert for me.
@found_logical is intentionally uninitialized to catch any
uninitialized usage by compiler.
It would be set by queue_scrub_stripe() when there is any stripe found.
Can you show me where the reference is set before the quoted if block?
Sure.
Firstly inside queue_scrub_stripe():
```
ret = scrub_find_fill_first_stripe(bg, &sctx->extent_path,
&sctx->csum_path, dev,
physical,
mirror_num, logical, length,
stripe);
/* Either >0 as no more extents or <0 for error. */
if (ret)
return ret;
if (found_logical_ret)
*found_logical_ret = stripe->logical;
```
In this case, we would only set @found_logical_ret to the found stripe's
logical.
Either we got ret > 0, meaning no more extent in the chunk, or we got
some critical error.
Then back to scrub_simple_mirrors():
I think I now understand the comment. This is a terrible foot gun, and
in fact
shows that this is not a false alert at all.
Within queue_scrub_stripe() you are "possibly" using (checking) a
reference that
has not been initialized, neither in the caller nor within
queue_scrub_stripe().
To the compiler this reference may not exist yet (even if we know that
it is on
the stack..) and point to la-la land. The fact that the logic depends on
ret
always being != 0 is not visible to the compiler, so the warning is
completely
correct: 0 is a valid possible value for ret, so this potentially allows
the
uninitialized read to happen.
What happens after that is not the point. You cannot safely check an
uninitialized
reference, even when the control flow "cannot happen" due to a data
dependency.
It seems to me the safest way forward is to simply initialize
found_logical in the
parent and remove the "if (found_logical_ret)" check, since it does
nothing useful.
Would that work?
Sure, that sounds pretty reasonable.
The "if (found_logical_ret)" is totally duplicated, if following the
regular btrfs way, I'd convert it to an "ASSERT(found_logical_ret);" at
the beginning of the function queue_scrub_stripe().
In that case, I'm totally fine to submit a patch to fix this warning.
Thanks for the report,
Qu
thanks,
Holger
