[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

[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

[Christian Zigotzky]

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:

--- 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, &sect);
          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, &sect);
          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 */

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

[John Paul Adrian Glaubitz]

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

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[Christian Zigotzky]

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

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]

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, &sect);
>          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, &sect);
>          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 */
>
>
>

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

[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

> > [    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,
-- 
Martin

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]

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

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]

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

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

[Martin Steigerwald]

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,
-- 
Martin