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:
[…]

quoted

On 29/06/23 16:59, Christian Zigotzky wrote:

quoted

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

[…]

quoted

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

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 [off-list ref]

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