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

[David Sterba <dsterba@suse.cz>]

On Tue, Sep 19, 2023 at 11:37:20AM +0930, Qu Wenruo wrote:
> 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.

Yes this looks like the better option. Changing all the kmalloc to
kzalloc could be done but it means touching the initialized memory twice
in case it's done by struct members. And this would be done on each
allocation, while resetting the failed destination buffer is done at
most once in case of a rare error.

> > 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.

I'm talking about this
https://lore.kernel.org/linux-btrfs/20230727141840.GC17922@twin.jikos.cz/