Re: [PATCH] btrfs: replace a wrong memset() with memzero_page()

Graham Cobb
2022-03-30T22:30:30Z

On 30/03/2022 22:34, Filipe Manana wrote:
> On Wed, Mar 30, 2022 at 12:03:36PM +0100, Graham Cobb wrote:
>>
>> On 30/03/2022 11:42, Qu Wenruo wrote:
>>>
>>>
>>> On 2022/3/30 18:34, Filipe Manana wrote:
>>>> On Wed, Mar 30, 2022 at 10:27:53AM +0100, Filipe Manana wrote:
>>>>> On Wed, Mar 30, 2022 at 07:52:04AM +0800, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> On 2022/3/29 19:39, Filipe Manana wrote:
>>>>>>> On Tue, Mar 29, 2022 at 06:49:10PM +0800, Qu Wenruo wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2022/3/29 17:57, Filipe Manana wrote:
>>>>>>>>> On Mon, Mar 28, 2022 at 08:51:21PM +0200, David Sterba wrote:
>>>>>>>>>> On Fri, Mar 25, 2022 at 05:37:59PM +0800, Qu Wenruo wrote:
>>>>>>>>>>> The original code is not really setting the memory to 0x00 but
>>>>>>>>>>> 0x01.
>>>>>>>>>>>
>>>>>>>>>>> To prevent such problem from happening, use memzero_page()
>>>>>>>>>>> instead.
>>>>>>>>>>
>>>>>>>>>> This should at least mention we think that setting it to 0 is
>>>>>>>>>> right, as
>>>>>>>>>> you call it wrong but give no hint why it's thought to be wrong.
>>>>>>>>>
>>>>>>>>> My guess is that something different from zero makes it easier to
>>>>>>>>> spot
>>>>>>>>> the problem in user space, as 0 is not uncommon (holes,
>>>>>>>>> prealloced extents)
>>>>>>>>> and may get unnoticed by applications/users.
>>>>>>>>
>>>>>>>> OK, that makes some sense.
>>>>>>>>
>>>>>>>> But shouldn't user space tool get an -EIO directly?
>>>>>>>
>>>>>>> It should.
>>>>>>>
>>>>>>> But even if applications get -EIO, they may often ignore return
>>>>>>> values.
>>>>>>> It's their fault, but if we can make it less likely that errors are
>>>>>>> not noticed,
>>>>>>> the better. I think we all did often, ignore all or just some
>>>>>>> return values
>>>>>>> from read(), write(), open(), etc.
>>>>>>>
>>>>>>> One recent example is the MariaDB case with io-uring. They were
>>>>>>> reporting
>>>>>>> corruption to the users, but the problem is that didn't properly check
>>>>>>> return values, ignoring partial reads and treating them as success:
>>>>>>>
>>>>>>> https://jira.mariadb.org/browse/MDEV-27900?focusedCommentId=216582&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-216582
>>>>>>>
>>>>>>>
>>>>>>> The data was fine, not corrupted, they just didn't deal with
>>>>>>> partial reads
>>>>>>> and then read the remaining data when a read returns less data than
>>>>>>> expected.
>>>>>>
>>>>>> Then can we slightly improve the filling pattern?
>>>>>>
>>>>>> Instead of 0x01, introduce some poison value?
>>>>>
>>>>> Why isn't 0x01 a good enough value?
>>>>>
>>>>> A long range of 0x01 values is certainly unexpected in text files,
>>>>> and very likely
>>>>> on binary formats as well. Or do you think there's some case where
>>>>> long sequences
>>>>> of 0x01 are common and not unexpected?
>>>>>
>>>>>>
>>>>>> And of course, change the lable "zeroit" to something like "poise_it"?
>>>>>
>>>>> "poison_it", poise has a very different and unrelated meaning in
>>>>> English.
>>>>
>>>> It's also worth considering if we should change the page content at all.
>>>>
>>>> Adding a poison value makes it easier to detect the corruption, both for
>>>> developers and for sloppy applications that don't check error values.
>>>>
>>>> However people often want to still have access to the corrupted data,
>>>> they
>>>> can tolerate a few corrupted bytes and find the remaining useful. This
>>>> has
>>>> been requested a few times in the past.
>>>
>>> This looks more favorable.
>>>
>>> And I didn't think the change would break anything.
>>>
>>> For proper user space programs checking the error, they know it's an
>>> -EIO and will error out.
>>>
>>> For bad programs not checking the error, it will just read the corrupted
>>> data, and may even pass its internal sanity checks (if the corrupted
>>> bytes are not in some critical part).
>>>
>>> Or is there something we haven't taken into consideration?
>>
>> Well.... If an error occurred something has gone wrong. It could be a
>> simple bit flip in the storage itself, or it could be something else.
>> The something else could be a software or firmware bug, or a memory
>> corruption or memory controller power fluctuation blip, or .... I guess
>> it might even be possible in some architectures that the problem could
>> result in page table updates validating a page that actually was never
>> written to at all and could still contain some previous contents.
>>
>> In a time when we worry about things like Spectre, Meltdown, Rowhammer,
>> stale cache leakage, etc it may be a good security principle that data
>> left over from failed operations should never be made visible to user mode.
>>
>> Possibly unnecessary worrying, but then who thought that jump prediction
>> would create a side-channel for exposing data that can actually be
>> exploited in real life.
>
> Hum, then a filesystem that doesn't have data checksums (the vast majority of
> them), like ext4, xfs, etc, has a serious security flaw, no? You ask to read an
> extent, and it returns whatever is on disk, even if it has suffered some corruption.
>
> Shall we open a CVE, and force all filesystems to implement data checksums? ;)
> We would also have to drop support for nodatacow and nodatasum from btrfs.

No. There is a clear difference between cases where you don't know
whether the data is valid and cases where there has been an error
detected (a device-reported read error, a checksum error, a logic error,
or whatever).

You asked if there was anything you hadn't taken into consideration. It
is entirely up to you and the team to consider it - in cases where you
*know* an error has occurred should you refuse to return the data? I
don't know - but the decision should be deliberate and deserves a comment.