| Message ID | 024ce4fa-cc6d-50a2-9aae-3701d0ebf668@xenosoft.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [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 | expand |
Hello Christian! On Thu, 2023-06-29 at 06:59 +0200, Christian Zigotzky wrote: > 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 > > Model: ATA ST2000DM001-9YN1 (scsi) > Disk /dev/sda: 2000GB > Sector size (logical/physical): 512B/4096B > Partition Table: amiga > Disk Flags: > > Number Start End Size File system Name Flags > 1 1057kB 123MB 122MB affs7 BDH0 hidden > 2 123MB 2274MB 2150MB DH0 boot > 3 2274MB 691GB 689GB DH2 > 4 691GB 1992GB 1301GB ext4 dhx boot What version of AmigaOS is that? > dmesg | grep -i sda > > [ 4.208905] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: > (2.00 TB/1.82 TiB) > [ 4.253995] sd 0:0:0:0: [sda] 4096-byte physical blocks > [ 4.254826] sd 0:0:0:0: [sda] Write Protect is off > [ 4.300069] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > [ 4.486476] sd 0:0:0:0: [sda] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 4.580507] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 bytes > [ 4.712624] Dev sda: unable to read partition block 4294967295 > [ 4.761532] 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) > unable to read partition table > [ 4.761892] sda: partition table beyond EOD, > [ 4.861681] Dev sda: unable to read partition block 4294967295 > [ 4.912094] 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) > unable to read partition table > [ 4.963387] sda: partition table beyond EOD, > [ 5.014769] sd 0:0:0:0: [sda] Attached SCSI disk Maybe the RDB is corrupted? Did you try on a freshly created RDB? > I created a patch for reverting the commit. [4] That can be done with just "git revert <commit hash>". > The access works again with this patch: > > [ 0.000000] Kernel command line: root=/dev/sda4 > [ 3.987717] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: > (2.00 TB/1.82 TiB) > [ 4.031349] sd 0:0:0:0: [sda] 4096-byte physical blocks > [ 4.123773] sd 0:0:0:0: [sda] Write Protect is off > [ 4.168682] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > [ 4.279304] sd 0:0:0:0: [sda] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 4.463508] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 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, > [ 4.922550] 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.948655] sda: p4 size 18446744071956107760 extends beyond EOD, > truncated Looks like the old code is complaining about your partition table as well. > Could you please check your commit? Please also make sure that your RDB is not corrupted. Adrian
Hello Adrian, On 29 June 2023 at 12:17 pm, John Paul Adrian Glaubitz wrote: > What version of AmigaOS is that? AmigaOS 4.1 > Maybe the RDB is corrupted? Did you try on a freshly created RDB? Good idea! I recreated the RDB with the Media Toolbox on the sb600sata.device, 0. (AmigaOne X1000) Unfortunately, it doesn't solve the issue. > That can be done with just "git revert <commit hash>". I know but I prefer my own patch because sometimes I can't revert the commit anymore because of dependencies. Thanks, Christian
Hi Christian, [ added Martin Steigerwald in CC ] thanks for your bug report (and of course, for testing the patch in the first instance)! 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 > > Model: ATA ST2000DM001-9YN1 (scsi) > Disk /dev/sda: 2000GB > Sector size (logical/physical): 512B/4096B > Partition Table: amiga > Disk Flags: > > Number Start End Size File system Name Flags > 1 1057kB 123MB 122MB affs7 BDH0 hidden > 2 123MB 2274MB 2150MB DH0 boot > 3 2274MB 691GB 689GB DH2 > 4 691GB 1992GB 1301GB ext4 dhx boot > > dmesg | grep -i sda > > [ 4.208905] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: > (2.00 TB/1.82 TiB) > [ 4.253995] sd 0:0:0:0: [sda] 4096-byte physical blocks > [ 4.254826] sd 0:0:0:0: [sda] Write Protect is off > [ 4.300069] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > [ 4.486476] sd 0:0:0:0: [sda] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 4.580507] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 bytes > [ 4.712624] Dev sda: unable to read partition block 4294967295 > [ 4.761532] 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) > unable to read partition table > [ 4.761892] sda: partition table beyond EOD, > [ 4.861681] Dev sda: unable to read partition block 4294967295 > [ 4.912094] 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) > unable to read partition table > [ 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.) Could you please create a dump of the RDB (first 4 MB of the disk ought to be enough) and send that? > [ 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: > > [ 0.000000] Kernel command line: root=/dev/sda4 > [ 3.987717] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: > (2.00 TB/1.82 TiB) > [ 4.031349] sd 0:0:0:0: [sda] 4096-byte physical blocks > [ 4.123773] sd 0:0:0:0: [sda] Write Protect is off > [ 4.168682] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 > [ 4.279304] sd 0:0:0:0: [sda] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 4.463508] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 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? > [ 4.922550] 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.948655] sda: p4 size 18446744071956107760 extends beyond EOD, > truncated > [ 4.998956] sd 0:0:0:0: [sda] Attached SCSI disk > [ 8.394695] EXT4-fs (sda4): mounted filesystem > 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 ro with ordered data mode. Quota > mode: none. > [ 18.578020] EXT4-fs (sda4): re-mounted > 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 ro. Quota mode: none. > [ 23.159524] EXT4-fs (sda4): re-mounted > 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 r/w. Quota mode: none. > > 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... 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)) { /* Read in terms partition table understands */ if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { pr_err("Dev %s: overflow calculating partition block %llu! Skipping partitions %u and beyond\n", Cheers, Michael > > Thanks, > Christian > > [1] http://wiki.amiga.org/index.php?title=X5000 > [2] https://en.wikipedia.org/wiki/AmigaOne_X1000 > [3] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a0433f8cae3ac51f59b4b1863032822aaa2d8164 > [4] revert_amiga.c.patch: > > --- a/block/partitions/amiga.c 2023-06-29 04:29:22.947100347 +0200 > +++ b/block/partitions/amiga.c 2023-06-26 01:29:58.000000000 +0200 > @@ -11,18 +11,10 @@ > #define pr_fmt(fmt) fmt > > #include <linux/types.h> > -#include <linux/mm_types.h> > -#include <linux/overflow.h> > #include <linux/affs_hardblocks.h> > > #include "check.h" > > -/* magic offsets in partition DosEnvVec */ > -#define NR_HD 3 > -#define NR_SECT 5 > -#define LO_CYL 9 > -#define HI_CYL 10 > - > static __inline__ u32 > checksum_block(__be32 *m, int size) > { > @@ -39,12 +31,8 @@ int amiga_partition(struct parsed_partit > unsigned char *data; > struct RigidDiskBlock *rdb; > struct PartitionBlock *pb; > - u64 start_sect, nr_sects; > - sector_t blk, end_sect; > - u32 cylblk; /* rdb_CylBlocks = nr_heads*sect_per_track */ > - u32 nr_hd, nr_sect, lo_cyl, hi_cyl; > - int part, res = 0; > - unsigned int blksize = 1; /* Multiplier for disk block size */ > + int start_sect, nr_sects, blk, part, res = 0; > + int blksize = 1; /* Multiplier for disk block size */ > int slot = 1; > > for (blk = 0; ; blk++, put_dev_sector(sect)) { > @@ -52,7 +40,7 @@ int amiga_partition(struct parsed_partit > goto rdb_done; > data = read_part_sector(state, blk, §); > if (!data) { > - pr_err("Dev %s: unable to read RDB block %llu\n", > + pr_err("Dev %s: unable to read RDB block %d\n", > state->disk->disk_name, blk); > res = -1; > goto rdb_done; > @@ -69,12 +57,12 @@ int amiga_partition(struct parsed_partit > *(__be32 *)(data+0xdc) = 0; > if (checksum_block((__be32 *)data, > be32_to_cpu(rdb->rdb_SummedLongs) & 0x7F)==0) { > - pr_err("Trashed word at 0xd0 in block %llu ignored in > checksum calculation\n", > + pr_err("Trashed word at 0xd0 in block %d ignored in > checksum calculation\n", > blk); > break; > } > > - pr_err("Dev %s: RDB in block %llu has bad checksum\n", > + pr_err("Dev %s: RDB in block %d has bad checksum\n", > state->disk->disk_name, blk); > } > > @@ -91,15 +79,10 @@ int amiga_partition(struct parsed_partit > blk = be32_to_cpu(rdb->rdb_PartitionList); > put_dev_sector(sect); > for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) { > - /* Read in terms partition table understands */ > - if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { > - pr_err("Dev %s: overflow calculating partition block > %llu! Skipping partitions %u and beyond\n", > - state->disk->disk_name, blk, part); > - break; > - } > + blk *= blksize; /* Read in terms partition table > understands */ > data = read_part_sector(state, blk, §); > if (!data) { > - pr_err("Dev %s: unable to read partition block %llu\n", > + pr_err("Dev %s: unable to read partition block %d\n", > state->disk->disk_name, blk); > res = -1; > goto rdb_done; > @@ -111,70 +94,19 @@ int amiga_partition(struct parsed_partit > if (checksum_block((__be32 *)pb, > be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 ) > continue; > > - /* RDB gives us more than enough rope to hang ourselves with, > - * many times over (2^128 bytes if all fields max out). > - * Some careful checks are in order, so check for potential > - * overflows. > - * We are multiplying four 32 bit numbers to one sector_t! > - */ > - > - nr_hd = be32_to_cpu(pb->pb_Environment[NR_HD]); > - nr_sect = be32_to_cpu(pb->pb_Environment[NR_SECT]); > - > - /* CylBlocks is total number of blocks per cylinder */ > - if (check_mul_overflow(nr_hd, nr_sect, &cylblk)) { > - pr_err("Dev %s: heads*sects %u overflows u32, skipping > partition!\n", > - state->disk->disk_name, cylblk); > - continue; > - } > - > - /* check for consistency with RDB defined CylBlocks */ > - if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) { > - pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n", > - state->disk->disk_name, cylblk, > - be32_to_cpu(rdb->rdb_CylBlocks)); > - } > - > - /* RDB allows for variable logical block size - > - * normalize to 512 byte blocks and check result. > - */ > - > - if (check_mul_overflow(cylblk, blksize, &cylblk)) { > - pr_err("Dev %s: partition %u bytes per cyl. overflows > u32, skipping partition!\n", > - state->disk->disk_name, part); > - continue; > - } > - > - /* Calculate partition start and end. Limit of 32 bit on cylblk > - * guarantees no overflow occurs if LBD support is enabled. > - */ > - > - lo_cyl = be32_to_cpu(pb->pb_Environment[LO_CYL]); > - start_sect = ((u64) lo_cyl * cylblk); > - > - hi_cyl = be32_to_cpu(pb->pb_Environment[HI_CYL]); > - nr_sects = (((u64) hi_cyl - lo_cyl + 1) * cylblk); > + /* Tell Kernel about it */ > > + nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 - > + be32_to_cpu(pb->pb_Environment[9])) * > + be32_to_cpu(pb->pb_Environment[3]) * > + be32_to_cpu(pb->pb_Environment[5]) * > + blksize; > if (!nr_sects) > continue; > - > - /* Warn user if partition end overflows u32 (AmigaDOS limit) */ > - > - if ((start_sect + nr_sects) > UINT_MAX) { > - pr_warn("Dev %s: partition %u (%llu-%llu) needs 64 bit > device support!\n", > - state->disk->disk_name, part, > - start_sect, start_sect + nr_sects); > - } > - > - if (check_add_overflow(start_sect, nr_sects, &end_sect)) { > - pr_err("Dev %s: partition %u (%llu-%llu) needs LBD device > support, skipping partition!\n", > - state->disk->disk_name, part, > - start_sect, end_sect); > - continue; > - } > - > - /* Tell Kernel about it */ > - > + start_sect = be32_to_cpu(pb->pb_Environment[9]) * > + be32_to_cpu(pb->pb_Environment[3]) * > + be32_to_cpu(pb->pb_Environment[5]) * > + blksize; > put_partition(state,slot++,start_sect,nr_sects); > { > /* Be even more informative to aid mounting */ > > >
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@lichtvoll.de/ 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@lichtvoll.de/ > > [ 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@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. Best,
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
Hi Martin, Christian, Am 01.07.2023 um 09:17 schrieb Michael Schmitz: >>> 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. Confirmed the bug on that RDB block, also that my proposed patch fixes it, at least as far as that's possible to show with a sparse image file. Now I note that this patch will actually treat any partition block address beyond the 31 bit limit as end of the linked list, but that's been the behaviour of Linux RDB partitions since very early on, so I see no reason to change that. The RDB format description URL that appears in one of your messages from the 2012 thread has gone dead. I'll try to find it on Wayback later. In the meantime, I will submit a patch to fix the new bug ... We can has out details in the inevitable review process. Cheers, Michael
Hi Michael. Michael Schmitz - 01.07.23, 04:05:30 CEST: > The RDB format description URL that appears in one of your messages > from the 2012 thread has gone dead. I'll try to find it on Wayback > later. In the meantime, I will submit a patch to fix the new bug ... > We can has out details in the inevitable review process. Hmm, I think they just went for prettier URLs: https://wiki.amigaos.net/wiki/RDB But there is not much on it. Not sure whether there is a more detailed explanation anywhere on the net. I am pretty sure there is more in the NDK/SDK tough. And indeed there is: AmigaOS 3.x NDK https://www.hyperion-entertainment.com/index.php/downloads? view=files&parent=40 I found it in: Include_H/devices/hardblocks.h This is at least an include with the exact structure of the RDB and of the partition blocks. There are some explanations in the comments. Maybe there is something more elsewhere in this NDK. You can unpack this using lhasa, easily available in Debian based distributions. AmigaOS 4.x Software Development Kit https://www.hyperion-entertainment.com/index.php/downloads? view=files&parent=30 This is a bit more time consuming to dig through. I skip it for now. Best,
--- a/block/partitions/amiga.c 2023-06-29 04:29:22.947100347 +0200 +++ b/block/partitions/amiga.c 2023-06-26 01:29:58.000000000 +0200 @@ -11,18 +11,10 @@ #define pr_fmt(fmt) fmt #include <linux/types.h> -#include <linux/mm_types.h> -#include <linux/overflow.h> #include <linux/affs_hardblocks.h> #include "check.h" -/* magic offsets in partition DosEnvVec */ -#define NR_HD 3 -#define NR_SECT 5 -#define LO_CYL 9 -#define HI_CYL 10 - static __inline__ u32 checksum_block(__be32 *m, int size) { @@ -39,12 +31,8 @@ int amiga_partition(struct parsed_partit unsigned char *data; struct RigidDiskBlock *rdb; struct PartitionBlock *pb; - u64 start_sect, nr_sects; - sector_t blk, end_sect; - u32 cylblk; /* rdb_CylBlocks = nr_heads*sect_per_track */ - u32 nr_hd, nr_sect, lo_cyl, hi_cyl; - int part, res = 0; - unsigned int blksize = 1; /* Multiplier for disk block size */ + int start_sect, nr_sects, blk, part, res = 0; + int blksize = 1; /* Multiplier for disk block size */ int slot = 1; for (blk = 0; ; blk++, put_dev_sector(sect)) { @@ -52,7 +40,7 @@ int amiga_partition(struct parsed_partit goto rdb_done; data = read_part_sector(state, blk, §); if (!data) { - pr_err("Dev %s: unable to read RDB block %llu\n", + pr_err("Dev %s: unable to read RDB block %d\n", state->disk->disk_name, blk); res = -1; goto rdb_done; @@ -69,12 +57,12 @@ int amiga_partition(struct parsed_partit *(__be32 *)(data+0xdc) = 0; if (checksum_block((__be32 *)data, be32_to_cpu(rdb->rdb_SummedLongs) & 0x7F)==0) { - pr_err("Trashed word at 0xd0 in block %llu ignored in checksum calculation\n", + pr_err("Trashed word at 0xd0 in block %d ignored in checksum calculation\n", blk); break; } - pr_err("Dev %s: RDB in block %llu has bad checksum\n", + pr_err("Dev %s: RDB in block %d has bad checksum\n", state->disk->disk_name, blk);
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 Model: ATA ST2000DM001-9YN1 (scsi) Disk /dev/sda: 2000GB Sector size (logical/physical): 512B/4096B Partition Table: amiga Disk Flags: Number Start End Size File system Name Flags 1 1057kB 123MB 122MB affs7 BDH0 hidden 2 123MB 2274MB 2150MB DH0 boot 3 2274MB 691GB 689GB DH2 4 691GB 1992GB 1301GB ext4 dhx boot dmesg | grep -i sda [ 4.208905] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: (2.00 TB/1.82 TiB) [ 4.253995] sd 0:0:0:0: [sda] 4096-byte physical blocks [ 4.254826] sd 0:0:0:0: [sda] Write Protect is off [ 4.300069] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 [ 4.486476] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 4.580507] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 bytes [ 4.712624] Dev sda: unable to read partition block 4294967295 [ 4.761532] 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) unable to read partition table [ 4.761892] sda: partition table beyond EOD, [ 4.861681] Dev sda: unable to read partition block 4294967295 [ 4.912094] 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) unable to read partition table [ 4.963387] sda: partition table beyond EOD, [ 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: [ 0.000000] Kernel command line: root=/dev/sda4 [ 3.987717] sd 0:0:0:0: [sda] 3907029168 512-byte logical blocks: (2.00 TB/1.82 TiB) [ 4.031349] sd 0:0:0:0: [sda] 4096-byte physical blocks [ 4.123773] sd 0:0:0:0: [sda] Write Protect is off [ 4.168682] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00 [ 4.279304] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 4.463508] sd 0:0:0:0: [sda] Preferred minimum I/O size 4096 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, [ 4.922550] 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.948655] sda: p4 size 18446744071956107760 extends beyond EOD, truncated [ 4.998956] sd 0:0:0:0: [sda] Attached SCSI disk [ 8.394695] EXT4-fs (sda4): mounted filesystem 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 ro with ordered data mode. Quota mode: none. [ 18.578020] EXT4-fs (sda4): re-mounted 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 ro. Quota mode: none. [ 23.159524] EXT4-fs (sda4): re-mounted 93cb7dd2-ce1b-4bf5-ba47-818cf8e8c9f4 r/w. Quota mode: none. Could you please check your commit? Thanks, Christian [1] http://wiki.amiga.org/index.php?title=X5000 [2] https://en.wikipedia.org/wiki/AmigaOne_X1000 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a0433f8cae3ac51f59b4b1863032822aaa2d8164 [4] revert_amiga.c.patch: } @@ -91,15 +79,10 @@ int amiga_partition(struct parsed_partit blk = be32_to_cpu(rdb->rdb_PartitionList); put_dev_sector(sect); for (part = 1; blk>0 && part<=16; part++, put_dev_sector(sect)) { - /* Read in terms partition table understands */ - if (check_mul_overflow(blk, (sector_t) blksize, &blk)) { - pr_err("Dev %s: overflow calculating partition block %llu! Skipping partitions %u and beyond\n", - state->disk->disk_name, blk, part); - break; - } + blk *= blksize; /* Read in terms partition table understands */ data = read_part_sector(state, blk, §); if (!data) { - pr_err("Dev %s: unable to read partition block %llu\n", + pr_err("Dev %s: unable to read partition block %d\n", state->disk->disk_name, blk); res = -1; goto rdb_done; @@ -111,70 +94,19 @@ int amiga_partition(struct parsed_partit if (checksum_block((__be32 *)pb, be32_to_cpu(pb->pb_SummedLongs) & 0x7F) != 0 ) continue; - /* RDB gives us more than enough rope to hang ourselves with, - * many times over (2^128 bytes if all fields max out). - * Some careful checks are in order, so check for potential - * overflows. - * We are multiplying four 32 bit numbers to one sector_t! - */ - - nr_hd = be32_to_cpu(pb->pb_Environment[NR_HD]); - nr_sect = be32_to_cpu(pb->pb_Environment[NR_SECT]); - - /* CylBlocks is total number of blocks per cylinder */ - if (check_mul_overflow(nr_hd, nr_sect, &cylblk)) { - pr_err("Dev %s: heads*sects %u overflows u32, skipping partition!\n", - state->disk->disk_name, cylblk); - continue; - } - - /* check for consistency with RDB defined CylBlocks */ - if (cylblk > be32_to_cpu(rdb->rdb_CylBlocks)) { - pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n", - state->disk->disk_name, cylblk, - be32_to_cpu(rdb->rdb_CylBlocks)); - } - - /* RDB allows for variable logical block size - - * normalize to 512 byte blocks and check result. - */ - - if (check_mul_overflow(cylblk, blksize, &cylblk)) { - pr_err("Dev %s: partition %u bytes per cyl. overflows u32, skipping partition!\n", - state->disk->disk_name, part); - continue; - } - - /* Calculate partition start and end. Limit of 32 bit on cylblk - * guarantees no overflow occurs if LBD support is enabled. - */ - - lo_cyl = be32_to_cpu(pb->pb_Environment[LO_CYL]); - start_sect = ((u64) lo_cyl * cylblk); - - hi_cyl = be32_to_cpu(pb->pb_Environment[HI_CYL]); - nr_sects = (((u64) hi_cyl - lo_cyl + 1) * cylblk); + /* Tell Kernel about it */ + nr_sects = (be32_to_cpu(pb->pb_Environment[10]) + 1 - + be32_to_cpu(pb->pb_Environment[9])) * + be32_to_cpu(pb->pb_Environment[3]) * + be32_to_cpu(pb->pb_Environment[5]) * + blksize; if (!nr_sects) continue; - - /* Warn user if partition end overflows u32 (AmigaDOS limit) */ - - if ((start_sect + nr_sects) > UINT_MAX) { - pr_warn("Dev %s: partition %u (%llu-%llu) needs 64 bit device support!\n", - state->disk->disk_name, part, - start_sect, start_sect + nr_sects); - } - - if (check_add_overflow(start_sect, nr_sects, &end_sect)) { - pr_err("Dev %s: partition %u (%llu-%llu) needs LBD device support, skipping partition!\n", - state->disk->disk_name, part, - start_sect, end_sect); - continue; - } - - /* Tell Kernel about it */ - + start_sect = be32_to_cpu(pb->pb_Environment[9]) * + be32_to_cpu(pb->pb_Environment[3]) * + be32_to_cpu(pb->pb_Environment[5]) * + blksize; put_partition(state,slot++,start_sect,nr_sects); { /* Be even more informative to aid mounting */