* [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
@ 2023-09-15 7:10 Dan Carpenter
2023-09-15 19:00 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-09-15 7:10 UTC (permalink / raw)
To: wqu; +Cc: linux-btrfs
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.
4103
4104 memcpy(dstv, eb_addr + start, len);
4105 }
Sample caller:
fs/btrfs/volumes.c
7405 static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
7406 const struct btrfs_dev_stats_item *ptr,
7407 int index)
7408 {
7409 u64 val;
7410
7411 read_extent_buffer(eb, &val,
7412 offsetof(struct btrfs_dev_stats_item, values) +
7413 ((unsigned long)ptr) + (index * sizeof(u64)),
7414 sizeof(val));
--> 7415 return val;
7416 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
2023-09-15 7:10 [bug report] btrfs: extent_io: do extra check for extent buffer read write functions Dan Carpenter
@ 2023-09-15 19:00 ` David Sterba
2023-09-15 21:11 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2023-09-15 19:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: wqu, linux-btrfs
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
2023-09-15 19:00 ` David Sterba
@ 2023-09-15 21:11 ` Qu Wenruo
2023-09-18 16:57 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2023-09-15 21:11 UTC (permalink / raw)
To: dsterba, Dan Carpenter; +Cc: wqu, linux-btrfs
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?
Thanks,
Qu
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
2023-09-15 21:11 ` Qu Wenruo
@ 2023-09-18 16:57 ` David Sterba
2023-09-19 2:07 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2023-09-18 16:57 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, Dan Carpenter, wqu, linux-btrfs
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. Also
please s/continuous/contiguous/. We can then continue the discussion
regarding the allocator behaviour.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
2023-09-18 16:57 ` David Sterba
@ 2023-09-19 2:07 ` Qu Wenruo
2023-09-19 16:26 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2023-09-19 2:07 UTC (permalink / raw)
To: dsterba; +Cc: Dan Carpenter, wqu, linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] btrfs: extent_io: do extra check for extent buffer read write functions
2023-09-19 2:07 ` Qu Wenruo
@ 2023-09-19 16:26 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2023-09-19 16:26 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, Dan Carpenter, wqu, linux-btrfs
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/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-19 16:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-15 7:10 [bug report] btrfs: extent_io: do extra check for extent buffer read write functions Dan Carpenter
2023-09-15 19:00 ` David Sterba
2023-09-15 21:11 ` Qu Wenruo
2023-09-18 16:57 ` David Sterba
2023-09-19 2:07 ` Qu Wenruo
2023-09-19 16:26 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox