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
