Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/9/19 02:27, David Sterba wrote:
> On Sat, Sep 16, 2023 at 06:41:12AM +0930, Qu Wenruo wrote:
>>
>>
>> On 2023/9/16 04:30, David Sterba wrote:
>>> On Fri, Sep 15, 2023 at 10:10:13AM +0300, Dan Carpenter wrote:
>>>> Hello Qu Wenruo,
>>>>
>>>> The patch f98b6215d7d1: "btrfs: extent_io: do extra check for extent
>>>> buffer read write functions" from Aug 19, 2020 (linux-next), leads to
>>>> the following Smatch static checker warning:
>>>>
>>>> fs/btrfs/print-tree.c:186 print_uuid_item() error: uninitialized symbol 'subvol_id'.
>>>> fs/btrfs/tests/extent-io-tests.c:338 check_eb_bitmap() error: uninitialized symbol 'has'.
>>>> fs/btrfs/tests/extent-io-tests.c:353 check_eb_bitmap() error: uninitialized symbol 'has'.
>>>> fs/btrfs/uuid-tree.c:203 btrfs_uuid_tree_remove() error: uninitialized symbol 'read_subid'.
>>>> fs/btrfs/uuid-tree.c:353 btrfs_uuid_tree_iterate() error: uninitialized symbol 'subid_le'.
>>>> fs/btrfs/uuid-tree.c:72 btrfs_uuid_tree_lookup() error: uninitialized symbol 'data'.
>>>> fs/btrfs/volumes.c:7415 btrfs_dev_stats_value() error: uninitialized symbol 'val'.
>>>>
>>>> fs/btrfs/extent_io.c
>>>>     4096  void read_extent_buffer(const struct extent_buffer *eb, void *dstv,
>>>>     4097                          unsigned long start, unsigned long len)
>>>>     4098  {
>>>>     4099          void *eb_addr = btrfs_get_eb_addr(eb);
>>>>     4100
>>>>     4101          if (check_eb_range(eb, start, len))
>>>>     4102                  return;
>>>>                           ^^^^^^^
>>>>
>>>> Originally this used to memset dstv to zero but now it just returns.
>>>> I can easily just mark that error path as impossible.  These days
>>>> everyone with a brain zeros their stack variables anyway so it shouldn't
>>>> affect anyone who doesn't deserve it.
>>>
>>> Thanks, this explains the other errors reported on linux-next with
>>> possibly uninitialized variables.
>>
>> Mind me to fix those uninitialized variables?
>> Or should I just revert to the old behavior?
>
> I'd like to keep the branch in for-next so please fix the warnings.

Unfortunately these warnings are not complete.

I checked all the read_extent_buffer() callers, almost all of them needs
some initialization.

One example in split_item() of ctree.c:

static noinline int split_item()
{
	char *buf;

	buf = kmalloc();
	read_extent_buffer(leaf, buf, );
}

In above case, if the read range is invalid, then @buf is just garbage.

I'm not sure if we should fix all call sites, it looks a little
impractical (over 70 call sites).

Thus I'd prefer to reset the target memory to zero if the range is invalid.

> Also
> please s/continuous/contiguous/. We can then continue the discussion
> regarding the allocator behaviour.

I guess you're talking about the extent buffer allocator (going vm
mapped memory to skip cross-page handling).
However this patch is years old and already in upstream.

Thanks,
Qu