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?
thanks, Holger