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 > > [ 4.963387] sda: partition table beyond EOD, > > Haven't see this one in any of my tests. I wonder whether your > partitioning software used that value of 4294967295 (32 bit unsigned > int max.) as marker for the end of the partition list, instead of > NULL? It's clearly beyond the end of your disk, so cannot be a > legitimate partition block address. (The parted dump above (showing > only four partitions) appears to support that notion.) I don't know about "-1" for end of partition list. However, on reviewing my bug report I think I used Media Toolbox myself to create the partition table. However I am not completely sure about that. It has been a long time. I used Media Toolbox on AmigaOS 4.1 to extract the binary copy of the RDB, but I do not recall what I used to partition it. I bet it was Media Toolbox as well, likely on some development version of AmigaOS 4. I was member of the AmigaOS 4 team back then. The disk is repartitioned as GPT and I do not even know at the moment where it is. So that is all I can say. I could ask someone from the AmigaOS 4 team, however I am not a member of it anymore. And in any case, whether Media Toolbox or another tool writes a "-1" to end partition list or not, I'd not treat a signed value "-1" as valid partition size. We could also ask Joanne again, however she was not involved in AmigaOS 4 development and does not know about Media Toolbox. She created the "hdwrench.library" for AmigaOS 3.5/3.9 HDToolBox. So maybe it is just good to assume "-1" as end of partition table, at least after confirming from the binary of the affected RDB that the "-1" is indeed at the end of the partition block list. Maybe also in the RDB from my bug report there is this "-1" again… however, the disk worked okay in Linux after testing with Joanne's initial patch from https://bugzilla.kernel.org/show_bug.cgi?id=43511#c7 Subject: Re: Partitions: Amiga RDB partition on 2 TB disk way too big, while OK in AmigaOS 4.1 https://lore.kernel.org/linux-m68k/ 201206192146.09327.Martin@xxxxxxxxxxxx/ This comment from an AmigaOS developer is only about the maximum sizes and does not detail a "-1" as partition size – however I did not ask about it back then: https://lore.kernel.org/linux-m68k/ 201206182239.26647.Martin@xxxxxxxxxxxx/ > > [ 5.014769] sd 0:0:0:0: [sda] Attached SCSI disk > > > > I created a patch for reverting the commit. [4] > > > > The access works again with this patch: […] > > bytes [ 4.519477] sda: RDSK (512) sda1 (DOS^G)(res 2 spb 2) > > sda2 (SFS^B)(res 2 spb 1) sda3 (SFS^B)(res 2 spb 2) sda4 ((res 2 > > spb 1) [ 4.720896] sda: p4 size 18446744071956107760 extends > > beyond EOD, > > That's the 32 bit overflow that the patch series was meant to correct. > Parsing the partition table ends before looking at the next partition > block in the list, so we never hit the bug you've seen above. > > 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. 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. > > 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 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. > 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@xxxxxxxxx> > > 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. Best, -- Martin
