linux-kernel - Re: [PATCH v9 03/11] btrfs: add support for inserting raid stripe extents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5452bbcc-64e4-41e0-aab0-162f5a769fd2@wdc.com>
Date:   Tue, 19 Sep 2023 12:13:32 +0000
From:   Johannes Thumshirn <Johannes.Thumshirn@....com>
To:     Qu Wenruo <quwenruo.btrfs@....com>, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>
CC:     Christoph Hellwig <hch@....de>,
        Naohiro Aota <Naohiro.Aota@....com>, Qu Wenruo <wqu@...e.com>,
        Damien Le Moal <dlemoal@...nel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 03/11] btrfs: add support for inserting raid stripe
 extents

On 15.09.23 02:55, Qu Wenruo wrote:
> 
> I found a corncer case that this can be problematic.
> 
> If we have a fs with RST root tree node/leaf corrupted, mounted with
> rescue=ibadroots, then fs_info->stripe_root would be NULL, and in the
> 5th patch inside set_io_stripe() we just fall back to regular non-RST path.
> This would bring us mostly incorrect data (and can be very problematic
> for nodatacsum files).
> 
> Thus stripe_root itself is not a reliable way to determine if we're at
> RST routine, I'd say only super incompat flags is reliable.

Fixed.

> 
> And fs_info->stripe_root should only be checked for functions that do
> RST tree operations, and return -EIO properly if it's not initialized.



>> +
>> +	if (type != BTRFS_BLOCK_GROUP_DATA)
>> +		return false;
>> +
>> +	if (profile & BTRFS_BLOCK_GROUP_RAID1_MASK)
>> +		return true;
> 
> Just a stupid quest, RAID0 DATA doesn't need RST purely because they are
>    the same as SINGLE, thus we only update the file items to the real
> written logical address, and no need for the extra mapping?

Yes but there can still be discrepancies between the assumed physical 
address and the real one due to ZONE_APPEND operations. RST backed file 
systems don't go the "normal" zoned btrfs logical rewrite path but have 
their own.

Also I prefere to keep the stripes together.

> Thus only profiles with duplication relies on RST, right?
> If so, then I guess DUP should also be covered by RST.
> 

Later in this patches, DUP, RAID0 and RAID10 will get added as well.