Re: [FSL P50x0] [PASEMI] The Access to partitions on disks with an Amiga partition table doesn't work anymore after the block updates 2023-06-23

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Martin,

Am 30.06.2023 um 20:35 schrieb Martin Steigerwald:
> Hi Michael, hi Christian.
>
> Michael Schmitz - 29.06.23, 22:27:59 CEST:
> […]
>> On 29/06/23 16:59, Christian Zigotzky wrote:
>>> Hello,
>>>
>>> The access  to partitions on disks with an Amiga partition table
>>> (via the Rigid Disk Block RDB) doesn't work anymore on my Cyrus+
>>> board with a FSL P50x0 PowerPC SoC [1] and on my P.A. Semi Nemo
>>> board [2] after the block updates 2023-06-23 [3].
>>>
>>> parted -l
> […]
>>> dmesg | grep -i sda
>>>
>>> [    4.208905] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks:
>>> (2.00 TB/1.82 TiB)
>
> That is roughly the size of the disk that triggered my bug report from
> 2012.
>
> Jun 19 21:19:09 merkaba kernel: [ 7891.821315] ata8.00: 3907029168
> sectors, multi 0: LBA48 NCQ (depth 31/32)
>
> Bug 43511 - Partitions: Amiga RDB partition on 2 TB disk way too big,
> while OK in AmigaOS 4.1
>
> https://bugzilla.kernel.org/show_bug.cgi?id=43511

Yes, that's been the first disk size allowing the overflow to occur. 
This time it's not about partition size but partition block address though.

>> By reverting my patch, you just reintroduce the old bug, which could
>> result in mis-parsing the partition table in a way that is not
>> detected by inane values of partition sizes as above, and as far as I
>> recall this bug was reported because it did cause data corruption. Do
>> I have that correct, Martin? Do you still have a copy of the
>> problematic RDB from the old bug report around?
>
> It is in the first attachment of the bug report I mentioned above. The
> bug the patch fixed.

Thanks, I'll get it from there.

> In the bug report I wrote:
>
> "I had a BTRFS filesystem that had some checksum errors. Maybe thats
> somehow related to this issue and AmigaOS and/or Linux overwrote
> something it shouldn´t have touched."
>
> (Meanwhile I bet it is safe to assume that in case the checksum error
> was from overwriting something it was not AmigaOS 4.)
>
> This is no proof, but as I re-read my bug report: It is clearly an
> overflow issue worsened by Linux back then truncating the too high
> partition sizes larger than the disk to the disk size instead of bailing
> out. So the partition I created for the Linux LVM in front of the Amiga
> partitions overflowed into the Amiga partitions. Had I used that place
> inside the PV for any LV and written to it… I bet it would have been
> goodbye to the Amiga data.

Thanks, that's good enough reason for me to not back out patch 3.

>
>>> Could you please check your commit?
>>
>> The patch series has undergone the usual thirteen versions in review,
>> but the reviewers as well as myself may well have missed this point of
>> detail...
>
> I think the patch series has been very well reviewed, but I would not
> have spotted such an issue as I am not really an RDB expert and even

I agree - not meant as a slight to the reviewers but more a dig at my 
patch record.

> then, with all the big endian conversions and what not inside of there…
> In my understanding the RDB format is not really as Rigid as the name
> implies. It is quite flexible, especially when compared to what had been
> used on PC back then and sometimes even now. So there is a chance for a
> RDB partitioning that triggers an oversight in the patch series.

At least it did show up in testing real fast.

>
>> Could you please check this (whitespace-damaged) patch?
>>
>>      block/partitions - Amiga partition overflow fix bugfix
>>
>>      Making 'blk' sector_t (i.e. 64 bit if LBD support is active)
>>      fails the 'blk>0' test in the partition block loop if a
>>      value of (signed int) -1 is used to mark the end of the
>>      partition block list.
>>
>>      Explicitly cast 'blk' to signed int to catch this.
>>
>>      Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
>> index ed222b9c901b..506921095412 100644
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>> @@ -90,7 +90,7 @@ int amiga_partition(struct parsed_partitions *state)
>> }
>>          blk = be32_to_cpu(rdb->rdb_PartitionList);
>>          put_dev_sector(sect);
>> -       for (part = 1; blk>0 && part<=16; part++,
>> put_dev_sector(sect)) {
>> +       for (part = 1; (s32) blk>0 && part<=16; part++,
>> put_dev_sector(sect)) {
>
> Makes sense to me.

Good, now we just need to see whether it does indeed fix the issue.

Cheers,

	Michael