Subject: [PATCH RFC] block: fix Amiga RDB partition support for disks >= 2 TB

Commit Message

Michael Schmitz June 27, 2018, 1:24 a.m. UTC

From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001

The Amiga RDB partition parser module uses int for partition sector
address and count, which will overflow for disks 2 TB and larger.

Use sector_t as type for sector address and size (as expected by
put_partition) to allow using such disks without danger of data
corruption.

This bug was reported originally in 2012 by Martin Steigerwald
<[e-mail]>, and the fix was created by the RDB author,
Joanne Dow <[e-mail]>. The patch had been discussed and
reviewed on linux-m68k at that time but never officially submitted.

Following a stern warning by Joanne, a warning is printed if any
partition is found to overflow the old 32 bit calculations, on the
grounds that such a partition would be misparses on legacy 32 bit
systems (other than Linux).

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
Reported-by: Martin Steigerwald <[e-mail]>
Message-ID: <[e-mail]>
Signed-off-by: Michael Schmitz <[e-mail]>
Tested-by: Martin Steigerwald <[e-mail]>
Tested-by: Michael Schmitz <[e-mail]>
---
 block/partitions/amiga.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Martin Steigerwald June 27, 2018, 8:13 a.m. UTC | #1

[e-mail] - 27.06.18, 03:24:
> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
> 
> The Amiga RDB partition parser module uses int for partition sector
> address and count, which will overflow for disks 2 TB and larger.
> 
> Use sector_t as type for sector address and size (as expected by
> put_partition) to allow using such disks without danger of data
> corruption.
> 
> This bug was reported originally in 2012 by Martin Steigerwald
> <[e-mail]>, and the fix was created by the RDB author,
> Joanne Dow <[e-mail]>. The patch had been discussed and
> reviewed on linux-m68k at that time but never officially submitted.
> 
> Following a stern warning by Joanne, a warning is printed if any
> partition is found to overflow the old 32 bit calculations, on the
> grounds that such a partition would be misparses on legacy 32 bit
> systems (other than Linux).
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
> Reported-by: Martin Steigerwald <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Michael Schmitz <[e-mail]>
> Tested-by: Martin Steigerwald <[e-mail]>
> Tested-by: Michael Schmitz <[e-mail]>
> ---
>  block/partitions/amiga.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
> index 5609366..42c3f38 100644
> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c
> @@ -32,7 +32,8 @@ int amiga_partition(struct parsed_partitions *state)
> unsigned char *data;
>  	struct RigidDiskBlock *rdb;
>  	struct PartitionBlock *pb;
> -	int start_sect, nr_sects, blk, part, res = 0;
> +	sector_t start_sect, nr_sects;
> +	int blk, part, res = 0;
>  	int blksize = 1;	/* Multiplier for disk block size */
>  	int slot = 1;
>  	char b[BDEVNAME_SIZE];
> @@ -111,6 +112,16 @@ int amiga_partition(struct parsed_partitions
> *state) be32_to_cpu(pb->pb_Environment[3]) *
>  			     be32_to_cpu(pb->pb_Environment[5]) *
>  			     blksize;
> +		if (start_sect > INT_MAX || nr_sects > INT_MAX
> +			|| (start_sect + nr_sects) > INT_MAX) {
> +			pr_err("%s: Warning: RDB partition overflow!\n",
> +				bdevname(state->bdev, b));
> +			pr_err("%s: start 0x%llX size 0x%llX\n",
> +				bdevname(state->bdev, b), start_sect,
> +				nr_sects);
> +			pr_err("%s: partition incompatible with 32 bit OS\n",
> +				bdevname(state->bdev, b));
> +		}

I do think the wording of that warning is inaccurate, as outlined in my 
other mails in thread "Re: moving affs + RDB partition support to 
staging?" just a few minutes ago (see there for a more complete 
reasoning). I´d word it like this:

partition needs 64 bit disk device support in AmigaOS or AmigaOS like 
operating systems (NSD64, TD64 or SCSI direct)

I think I would not include any more details, and let Amiga people 
research what they need and since when it is included officially on 
their own. As there are at least three variants out there: AmigaOS, 
MorphOS, AROS.

AmigaOS 4 at least can handle disks of 2 TB size or more. I do not think 
the wording "RDB overflow" is right either.

http://wiki.amigaos.net/wiki/RDB

Filesystem size limits are a different matter.

http://www.amigawiki.de/doku.php?id=de:system:filesystems_limits

>  		put_partition(state,slot++,start_sect,nr_sects);
>  		{
>  			/* Be even more informative to aid mounting */

Thanks,

Martin Steigerwald June 27, 2018, 8:24 a.m. UTC | #2

Thanks a lot again for your patch.

[e-mail] - 27.06.18, 03:24:
> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
> +                       || (start_sect + nr_sects) > INT_MAX) {
> +                       pr_err("%s: Warning: RDB partition
> overflow!\n", +                               bdevname(state->bdev,

I´d word this:

Warning: RDB partition 32-bit overflow

AmigaOS developers can do 64 bit math on a 32 bit operating system. Just 
like Linux can.

> b));
> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
> +                               bdevname(state->bdev, b), start_sect,
> +                               nr_sects);
> +                       pr_err("%s: partition incompatible with 32 bit
> OS\n", +                               bdevname(state->bdev, b));
> +               }

And as stated in my other reply to the patch:

partition needs 64 bit disk device support in AmigaOS or AmigaOS like 
operating systems (NSD64, TD64 or SCSI direct)

see my other reply to the patch and my other mails in the
"Re: moving affs + RDB partition support to staging?" thread as to why. 
And for references.

Thanks,

Geert Uytterhoeven June 27, 2018, 1:30 p.m. UTC | #3

Hi Michael,

Thanks for your patch!

On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001

??

> The Amiga RDB partition parser module uses int for partition sector
> address and count, which will overflow for disks 2 TB and larger.
>
> Use sector_t as type for sector address and size (as expected by
> put_partition) to allow using such disks without danger of data
> corruption.

Note that sector_t is not guaranteed to be 64-bit:

    #ifdef CONFIG_LBDAF
    typedef u64 sector_t;
    typedef u64 blkcnt_t;
    #else
    typedef unsigned long sector_t;
    typedef unsigned long blkcnt_t;
    #endif

And it seems CONFIG_LBDAF can still be disabled on 32-bit...

> This bug was reported originally in 2012 by Martin Steigerwald
> <[e-mail]>, and the fix was created by the RDB author,
> Joanne Dow <[e-mail]>. The patch had been discussed and
> reviewed on linux-m68k at that time but never officially submitted.
>
> Following a stern warning by Joanne, a warning is printed if any
> partition is found to overflow the old 32 bit calculations, on the
> grounds that such a partition would be misparses on legacy 32 bit
> systems (other than Linux).
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
> Reported-by: Martin Steigerwald <[e-mail]>
> Message-ID: <[e-mail]>
> Signed-off-by: Michael Schmitz <[e-mail]>
> Tested-by: Martin Steigerwald <[e-mail]>
> Tested-by: Michael Schmitz <[e-mail]>
> ---
>  block/partitions/amiga.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
> index 5609366..42c3f38 100644
> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c
> @@ -32,7 +32,8 @@ int amiga_partition(struct parsed_partitions *state)
>         unsigned char *data;
>         struct RigidDiskBlock *rdb;
>         struct PartitionBlock *pb;
> -       int start_sect, nr_sects, blk, part, res = 0;
> +       sector_t start_sect, nr_sects;

As sector_t can still be 32-bit, I think you should use an explicit u64 here.

> +       int blk, part, res = 0;
>         int blksize = 1;        /* Multiplier for disk block size */
>         int slot = 1;
>         char b[BDEVNAME_SIZE];
> @@ -111,6 +112,16 @@ int amiga_partition(struct parsed_partitions *state)
>                              be32_to_cpu(pb->pb_Environment[3]) *
>                              be32_to_cpu(pb->pb_Environment[5]) *
>                              blksize;

Without adding any unsigned long long or ULL stuff to the calculations
for start_sect and nr_sects above, the math will still be done using 32-bit
arithmetic. Or am I missing something?

> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
> +                       || (start_sect + nr_sects) > INT_MAX) {
> +                       pr_err("%s: Warning: RDB partition overflow!\n",
> +                               bdevname(state->bdev, b));
> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
> +                               bdevname(state->bdev, b), start_sect,
> +                               nr_sects);
> +                       pr_err("%s: partition incompatible with 32 bit OS\n",
> +                               bdevname(state->bdev, b));
> +               }

I don't know if the check above is really needed here.
There's also int vs. unsigned int. But see below.

>                 put_partition(state,slot++,start_sect,nr_sects);

Given sector_t may be 32-bit, values may be truncated when calling
put_partition(), so you need to check for that.

Interestingly, even partition parsers that do use u64 (efi, ldm) or loff_t
(ibm) do not have such checks.

Perhaps put_partition() should take u64, and print a warning and ignore the
partition if conversion to sector_t involves truncation?

>                 {
>                         /* Be even more informative to aid mounting */

Gr{oetje,eeting}s,

                        Geert

Michael Schmitz June 27, 2018, 8:13 p.m. UTC | #4

Hi Martin,

thanks for your comments.

On Wed, Jun 27, 2018 at 8:24 PM, Martin Steigerwald <[e-mail]> wrote:
> Thanks a lot again for your patch.
>
> [e-mail] - 27.06.18, 03:24:
>> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
>> +                       || (start_sect + nr_sects) > INT_MAX) {
>> +                       pr_err("%s: Warning: RDB partition
>> overflow!\n", +                               bdevname(state->bdev,
>
> I´d word this:
>
> Warning: RDB partition 32-bit overflow
>
> AmigaOS developers can do 64 bit math on a 32 bit operating system. Just
> like Linux can.

Yes, I realize that. I hadn't gone back through all the mails on the
subject to find out what the exact requrements are on the AmigaOS
side.

Just trying to be as terse as possible to keep checkpatch happy :-(

>
>> b));
>> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
>> +                               bdevname(state->bdev, b), start_sect,
>> +                               nr_sects);
>> +                       pr_err("%s: partition incompatible with 32 bit
>> OS\n", +                               bdevname(state->bdev, b));
>> +               }
>
> And as stated in my other reply to the patch:
>
> partition needs 64 bit disk device support in AmigaOS or AmigaOS like
> operating systems (NSD64, TD64 or SCSI direct)

I'd probably leave it at 'disk needs 64 bit disk device support on
native OS', and only print that warning once.

Geert has raised another important point about 64 bt device support -
all this is moot when the Linux kernel wasn't built with large block
device support enabled (you'd get the same buggy behaviour as before
the patch there).

> see my other reply to the patch and my other mails in the
> "Re: moving affs + RDB partition support to staging?" thread as to why.
> And for references.

Thanks for collating all the references. Please understand that I
can't read all of that, and as a simple patch mechanic I won't even
try to grasp all the subtleties of RDB (I don't even own an Amiga so I
am quite unlikey to ever use this code path).
But please also understand that for that reason, I take Joanne's
advice about backwards compatibility very serious. My patch (actually
Joanne's originally) changes kernel behaviour from what we consider
broken (allowing 32 bit overflow in partition address calculations) to
what we think is the right thing to do. But there might be someone out
there who used the current behaviour to craft a RDB that aliows two
separate sets of partitions to coexist on the same disk (one set
visible to 32 bit disk drivers, before the 32 bit overflow mark, and a
second set above that mark, visible only to 64 bit drivers. Silently
changing our parser behaviour might cause said user to now trash data
past the overflow mark.). This is a little contrived, and perhaps I am
overcomplicating matters (again), but can't be ruled out.

In the interest of least surprises, we have to fix the 32 bit overflow
(so we can even detect that it would have happened), and give the user
the chance to carefully consider whether to accept the new behaviour.
That means refusing to make available any partition that would have
been affected by such overflow.

The user has then all options available - force old behaviour by using
an older kernel, override the parser to force new behaviour (which we
all assume is correct), or leave the disk well alone.

Cheers,

  Michael

> Thanks,
> --
> Martin
>
>

Michael Schmitz June 27, 2018, 8:43 p.m. UTC | #5

Hi Geert,

thanks for your feedback!

On Thu, Jun 28, 2018 at 1:30 AM, Geert Uytterhoeven
<[e-mail]> wrote:
> Hi Michael,
>
> Thanks for your patch!
>
> On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
>> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
>
> ??

Comes from not using git send-email. Don't ask ...

>> The Amiga RDB partition parser module uses int for partition sector
>> address and count, which will overflow for disks 2 TB and larger.
>>
>> Use sector_t as type for sector address and size (as expected by
>> put_partition) to allow using such disks without danger of data
>> corruption.
>
> Note that sector_t is not guaranteed to be 64-bit:
>
>     #ifdef CONFIG_LBDAF
>     typedef u64 sector_t;
>     typedef u64 blkcnt_t;
>     #else
>     typedef unsigned long sector_t;
>     typedef unsigned long blkcnt_t;
>     #endif

Yes, I had seen that ...

> And it seems CONFIG_LBDAF can still be disabled on 32-bit...

Ouch - missed that bit.

>
>> This bug was reported originally in 2012 by Martin Steigerwald
>> <[e-mail]>, and the fix was created by the RDB author,
>> Joanne Dow <[e-mail]>. The patch had been discussed and
>> reviewed on linux-m68k at that time but never officially submitted.
>>
>> Following a stern warning by Joanne, a warning is printed if any
>> partition is found to overflow the old 32 bit calculations, on the
>> grounds that such a partition would be misparses on legacy 32 bit
>> systems (other than Linux).
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
>> Reported-by: Martin Steigerwald <[e-mail]>
>> Message-ID: <[e-mail]>
>> Signed-off-by: Michael Schmitz <[e-mail]>
>> Tested-by: Martin Steigerwald <[e-mail]>
>> Tested-by: Michael Schmitz <[e-mail]>
>> ---
>>  block/partitions/amiga.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
>> index 5609366..42c3f38 100644
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>> @@ -32,7 +32,8 @@ int amiga_partition(struct parsed_partitions *state)
>>         unsigned char *data;
>>         struct RigidDiskBlock *rdb;
>>         struct PartitionBlock *pb;
>> -       int start_sect, nr_sects, blk, part, res = 0;
>> +       sector_t start_sect, nr_sects;
>
> As sector_t can still be 32-bit, I think you should use an explicit u64 here.

You're spot on there.

>
>> +       int blk, part, res = 0;
>>         int blksize = 1;        /* Multiplier for disk block size */
>>         int slot = 1;
>>         char b[BDEVNAME_SIZE];
>> @@ -111,6 +112,16 @@ int amiga_partition(struct parsed_partitions *state)
>>                              be32_to_cpu(pb->pb_Environment[3]) *
>>                              be32_to_cpu(pb->pb_Environment[5]) *
>>                              blksize;
>
> Without adding any unsigned long long or ULL stuff to the calculations
> for start_sect and nr_sects above, the math will still be done using 32-bit
> arithmetic. Or am I missing something?

It did appear to do 64 bit arithmetic alright. But I better check what
instrunctions are used there.
I also need to look up the rules on default type promotion - additions
and subtractions would certainly still do 32 bit arithmetics but
multiplication may be different.

>
>> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
>> +                       || (start_sect + nr_sects) > INT_MAX) {
>> +                       pr_err("%s: Warning: RDB partition overflow!\n",
>> +                               bdevname(state->bdev, b));
>> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
>> +                               bdevname(state->bdev, b), start_sect,
>> +                               nr_sects);
>> +                       pr_err("%s: partition incompatible with 32 bit OS\n",
>> +                               bdevname(state->bdev, b));
>> +               }
>
> I don't know if the check above is really needed here.

It will be once I add a jump to rdb_done there ... The third test may
only be needed if no LBD support is present, but we absolutely have to
bail if the calculation of the partition end sector later on in the
kernel truncates.  Maybe I should take a page out of Christoph's book
and put a BUG() there to realy get people's attention?

> There's also int vs. unsigned int. But see below.

start_sect and nr_sects were int, not unsigned int before so I have to
compare to INT_MAX to see whether the old code would have overflowed.
What am I missing? Are you concerned the comparison will always be
false due to data type? I think I would have seen a warning. Anyway,
'seemed to work as intended' with Martin's test case.

>
>>                 put_partition(state,slot++,start_sect,nr_sects);
>
> Given sector_t may be 32-bit, values may be truncated when calling
> put_partition(), so you need to check for that.

put_partition() is inlined so I'm not sure a cast would help there,
And all that happens in put_partition() is that the start address and
size get stuffed into the parsed_partitions struct:

                p->parts[n].from = from;
                p->parts[n].size = size;

That struct has u32 types for from and size if compiled without
CONFIG_LBDAF, so whatever we do with 64 bit arithmetics, this will
lead to truncation here. Only safe option is to bail out on these
kernels.

> Interestingly, even partition parsers that do use u64 (efi, ldm) or loff_t
> (ibm) do not have such checks.

I would expect things to go horribly wrong if these are used without
LBD support. But perhaps the same checks need to be added there
indeed.

> Perhaps put_partition() should take u64, and print a warning and ignore the
> partition if conversion to sector_t involves truncation?

That's something for Jens to ponder :-)

I'll work in ann the suggested changes and then submit this to
linux-block for real.

Thanks again,

  Michael


>
>>                 {
>>                         /* Be even more informative to aid mounting */
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Martin Steigerwald June 27, 2018, 9:20 p.m. UTC | #6

Hi Michael.

Michael Schmitz - 27.06.18, 22:13:
> On Wed, Jun 27, 2018 at 8:24 PM, Martin Steigerwald 
<[e-mail]> wrote:
> > Thanks a lot again for your patch.
> > 
> > [e-mail] - 27.06.18, 03:24:
> >> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
> >> +                       || (start_sect + nr_sects) > INT_MAX) {
> >> +                       pr_err("%s: Warning: RDB partition
> >> overflow!\n", +                               bdevname(state->bdev,
> > 
> > I´d word this:
> > 
> > Warning: RDB partition 32-bit overflow
> > 
> > AmigaOS developers can do 64 bit math on a 32 bit operating system.
> > Just like Linux can.
> 
> Yes, I realize that. I hadn't gone back through all the mails on the
> subject to find out what the exact requrements are on the AmigaOS
> side.
> 
> Just trying to be as terse as possible to keep checkpatch happy :-(
> 
> >> b));
> >> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
> >> +                               bdevname(state->bdev, b),
> >> start_sect,
> >> +                               nr_sects);
> >> +                       pr_err("%s: partition incompatible with 32
> >> bit OS\n", +                               bdevname(state->bdev,
> >> b)); +               }
> > 
> > And as stated in my other reply to the patch:
> > 
> > partition needs 64 bit disk device support in AmigaOS or AmigaOS
> > like
> > operating systems (NSD64, TD64 or SCSI direct)
> 
> I'd probably leave it at 'disk needs 64 bit disk device support on
> native OS', and only print that warning once.

This is fine with me.

> Geert has raised another important point about 64 bt device support -
> all this is moot when the Linux kernel wasn't built with large block
> device support enabled (you'd get the same buggy behaviour as before
> the patch there).
> 
> > see my other reply to the patch and my other mails in the
> > "Re: moving affs + RDB partition support to staging?" thread as to
> > why. And for references.
> 
> Thanks for collating all the references. Please understand that I
> can't read all of that, and as a simple patch mechanic I won't even
> try to grasp all the subtleties of RDB (I don't even own an Amiga so I
> am quite unlikey to ever use this code path).

I understand that.

> But please also understand that for that reason, I take Joanne's
> advice about backwards compatibility very serious. My patch (actually
> Joanne's originally) changes kernel behaviour from what we consider
> broken (allowing 32 bit overflow in partition address calculations) to
> what we think is the right thing to do. But there might be someone
> out there who used the current behaviour to craft a RDB that aliows
> two separate sets of partitions to coexist on the same disk (one set
> visible to 32 bit disk drivers, before the 32 bit overflow mark, and
> a second set above that mark, visible only to 64 bit drivers.
> Silently changing our parser behaviour might cause said user to now
> trash data past the overflow mark.). This is a little contrived, and
> perhaps I am overcomplicating matters (again), but can't be ruled
> out.
> 
> In the interest of least surprises, we have to fix the 32 bit overflow
> (so we can even detect that it would have happened), and give the
> user the chance to carefully consider whether to accept the new
> behaviour. That means refusing to make available any partition that
> would have been affected by such overflow.

That is acceptable for me as I told before. Either mount or refuse to 
mount, but do not overflow and mount nonetheless :)

Mind you, I am not using my Amiga machines either at the moment. And I 
repurposed the 2 TB disk years ago. 
 
> The user has then all options available - force old behaviour by using
> an older kernel, override the parser to force new behaviour (which we
> all assume is correct), or leave the disk well alone.

Sure.

I would not name the kernel option "eat_my_rdb", but use a less 
dramatizing name.

Maybe just: "allow_64bit_rdb" or something like that.

How does the user come to know about this kernel option? Will you print 
its name in kernel log?

Thanks,

jdow June 28, 2018, 3:23 a.m. UTC | #7

Three issues exist here in two different places.

As far as a 32 TG disk is concerned RDBs can describe it and mount it safely - 
sort of - modulo the following issues. They are not a problem, I believe, with 
Amiga OSs new enough to understand RDBs. I cannot prove that. They are not 
sufficient, apparently, for Linux, which is why an __int64 equivalent is needed 
rather than a int equivalent.

As far as 4GB is concerned you are limited at 2 GB by int fseek( int ). This is 
not an RDB issue. Take it up with the filesystems. Linux seems to "think" 
differently than AmigaDOS.

As far as going over 4GB disk size you are limited within the OS by anything 
that deals with byte rather than block calculations for position on disk 
calculations. I think everything speaks properly within the OS to handle at 
least up to 8.7 GB partition sizes. For AmigaDOS we're probably OK up to 32 TB 
(with 8k block size). It some Joe somewhere needs 512 byte block size some other 
partition format is needed. RDBs will not handle it without changes which MAY 
cause RDB interpretation issues. (Change block headers and the spirit of RDBs 
can be preserved safely. But - why? GPT exists, is tested, works nicely. One 
must be careful with it to make sure it supports required features. Note that a 
loadable filesystem is nice. It can make a NEARLY impossible to kill home for 
malware. But it does allow patching in newer filesystems with some backwards 
compatibility. Does GPT have a way to support this? Does AmigaDOS support a 
clean way to clean a disk of a corrupted filesystem image?)

The changes to READING the RDBs for Linux are "obvious", __int64 or equivalent. 
That change is sane and clean. (Be careful writing RDBs mkfs-(amigafs). But 
that's a given. We're adults here.) Changes to make Amigas handle 64 bit 
filesystems are "easy". They just won't necessarily be safe past 4 G blocks. I 
suspect if file sizes are kept under 2GiB a compatibility layer can even keep 
the OS thinking it is on a disk it understands and to it safely.

Linux apparently needs the change. It is not an RDB change; it is an RDB parser 
change. Changing RDB, however, needs care. And at least to my perceptions this 
discussion wandered around the Linux RDB parser and actual RDB changes. These 
need separate discussions as to compatibility issues with any AmigaOS that may 
have a chance of even "tasting" the disk let alone "chewing" on the disk.

On the RDB changes issue, I would recommend disappointing an Amigoid (as utterly 
death defying as that may be) by his not being able to mount a disk rather than 
angering that same Amigoid by damaging or losing his data. That's simply life 
preservation on the part of the designers.

{^_^}

On 20180627 01:13, Martin Steigerwald wrote:
> [e-mail] - 27.06.18, 03:24:
>>  From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
>>
>> The Amiga RDB partition parser module uses int for partition sector
>> address and count, which will overflow for disks 2 TB and larger.
>>
>> Use sector_t as type for sector address and size (as expected by
>> put_partition) to allow using such disks without danger of data
>> corruption.
>>
>> This bug was reported originally in 2012 by Martin Steigerwald
>> <[e-mail]>, and the fix was created by the RDB author,
>> Joanne Dow <[e-mail]>. The patch had been discussed and
>> reviewed on linux-m68k at that time but never officially submitted.
>>
>> Following a stern warning by Joanne, a warning is printed if any
>> partition is found to overflow the old 32 bit calculations, on the
>> grounds that such a partition would be misparses on legacy 32 bit
>> systems (other than Linux).
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
>> Reported-by: Martin Steigerwald <[e-mail]>
>> Message-ID: <[e-mail]>
>> Signed-off-by: Michael Schmitz <[e-mail]>
>> Tested-by: Martin Steigerwald <[e-mail]>
>> Tested-by: Michael Schmitz <[e-mail]>
>> ---
>>   block/partitions/amiga.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
>> index 5609366..42c3f38 100644
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>> @@ -32,7 +32,8 @@ int amiga_partition(struct parsed_partitions *state)
>> unsigned char *data;
>>   	struct RigidDiskBlock *rdb;
>>   	struct PartitionBlock *pb;
>> -	int start_sect, nr_sects, blk, part, res = 0;
>> +	sector_t start_sect, nr_sects;
>> +	int blk, part, res = 0;
>>   	int blksize = 1;	/* Multiplier for disk block size */
>>   	int slot = 1;
>>   	char b[BDEVNAME_SIZE];
>> @@ -111,6 +112,16 @@ int amiga_partition(struct parsed_partitions
>> *state) be32_to_cpu(pb->pb_Environment[3]) *
>>   			     be32_to_cpu(pb->pb_Environment[5]) *
>>   			     blksize;
>> +		if (start_sect > INT_MAX || nr_sects > INT_MAX
>> +			|| (start_sect + nr_sects) > INT_MAX) {
>> +			pr_err("%s: Warning: RDB partition overflow!\n",
>> +				bdevname(state->bdev, b));
>> +			pr_err("%s: start 0x%llX size 0x%llX\n",
>> +				bdevname(state->bdev, b), start_sect,
>> +				nr_sects);
>> +			pr_err("%s: partition incompatible with 32 bit OS\n",
>> +				bdevname(state->bdev, b));
>> +		}
> 
> I do think the wording of that warning is inaccurate, as outlined in my
> other mails in thread "Re: moving affs + RDB partition support to
> staging?" just a few minutes ago (see there for a more complete
> reasoning). I´d word it like this:
> 
> partition needs 64 bit disk device support in AmigaOS or AmigaOS like
> operating systems (NSD64, TD64 or SCSI direct)
> 
> I think I would not include any more details, and let Amiga people
> research what they need and since when it is included officially on
> their own. As there are at least three variants out there: AmigaOS,
> MorphOS, AROS.
> 
> AmigaOS 4 at least can handle disks of 2 TB size or more. I do not think
> the wording "RDB overflow" is right either.
> 
> http://wiki.amigaos.net/wiki/RDB
> 
> Filesystem size limits are a different matter.
> 
> http://www.amigawiki.de/doku.php?id=de:system:filesystems_limits
> 
>>   		put_partition(state,slot++,start_sect,nr_sects);
>>   		{
>>   			/* Be even more informative to aid mounting */
> 
> Thanks,
>

jdow June 28, 2018, 3:45 a.m. UTC | #8

Oops! It MUST be 64 bit or everything go boom.

{O.O}

On 20180627 06:30, Geert Uytterhoeven wrote:
> Hi Michael,
> 
> Thanks for your patch!
> 
> On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
>>  From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
> 
> ??
> 
>> The Amiga RDB partition parser module uses int for partition sector
>> address and count, which will overflow for disks 2 TB and larger.
>>
>> Use sector_t as type for sector address and size (as expected by
>> put_partition) to allow using such disks without danger of data
>> corruption.
> 
> Note that sector_t is not guaranteed to be 64-bit:
> 
>      #ifdef CONFIG_LBDAF
>      typedef u64 sector_t;
>      typedef u64 blkcnt_t;
>      #else
>      typedef unsigned long sector_t;
>      typedef unsigned long blkcnt_t;
>      #endif
> 
> And it seems CONFIG_LBDAF can still be disabled on 32-bit...
> 
>> This bug was reported originally in 2012 by Martin Steigerwald
>> <[e-mail]>, and the fix was created by the RDB author,
>> Joanne Dow <[e-mail]>. The patch had been discussed and
>> reviewed on linux-m68k at that time but never officially submitted.
>>
>> Following a stern warning by Joanne, a warning is printed if any
>> partition is found to overflow the old 32 bit calculations, on the
>> grounds that such a partition would be misparses on legacy 32 bit
>> systems (other than Linux).
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=43511
>> Reported-by: Martin Steigerwald <[e-mail]>
>> Message-ID: <[e-mail]>
>> Signed-off-by: Michael Schmitz <[e-mail]>
>> Tested-by: Martin Steigerwald <[e-mail]>
>> Tested-by: Michael Schmitz <[e-mail]>
>> ---
>>   block/partitions/amiga.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
>> index 5609366..42c3f38 100644
>> --- a/block/partitions/amiga.c
>> +++ b/block/partitions/amiga.c
>> @@ -32,7 +32,8 @@ int amiga_partition(struct parsed_partitions *state)
>>          unsigned char *data;
>>          struct RigidDiskBlock *rdb;
>>          struct PartitionBlock *pb;
>> -       int start_sect, nr_sects, blk, part, res = 0;
>> +       sector_t start_sect, nr_sects;
> 
> As sector_t can still be 32-bit, I think you should use an explicit u64 here.
> 
>> +       int blk, part, res = 0;
>>          int blksize = 1;        /* Multiplier for disk block size */
>>          int slot = 1;
>>          char b[BDEVNAME_SIZE];
>> @@ -111,6 +112,16 @@ int amiga_partition(struct parsed_partitions *state)
>>                               be32_to_cpu(pb->pb_Environment[3]) *
>>                               be32_to_cpu(pb->pb_Environment[5]) *
>>                               blksize;
> 
> Without adding any unsigned long long or ULL stuff to the calculations
> for start_sect and nr_sects above, the math will still be done using 32-bit
> arithmetic. Or am I missing something?
> 
>> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
>> +                       || (start_sect + nr_sects) > INT_MAX) {
>> +                       pr_err("%s: Warning: RDB partition overflow!\n",
>> +                               bdevname(state->bdev, b));
>> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
>> +                               bdevname(state->bdev, b), start_sect,
>> +                               nr_sects);
>> +                       pr_err("%s: partition incompatible with 32 bit OS\n",
>> +                               bdevname(state->bdev, b));
>> +               }
> 
> I don't know if the check above is really needed here.
> There's also int vs. unsigned int. But see below.
> 
>>                  put_partition(state,slot++,start_sect,nr_sects);
> 
> Given sector_t may be 32-bit, values may be truncated when calling
> put_partition(), so you need to check for that.
> 
> Interestingly, even partition parsers that do use u64 (efi, ldm) or loff_t
> (ibm) do not have such checks.
> 
> Perhaps put_partition() should take u64, and print a warning and ignore the
> partition if conversion to sector_t involves truncation?
> 
>>                  {
>>                          /* Be even more informative to aid mounting */
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>

jdow June 28, 2018, 3:48 a.m. UTC | #9

Um. new 64 bit stuff must be invisible to old 32 bit stuff.
{^_^}

On 20180627 14:20, Martin Steigerwald wrote:
> Hi Michael.
> 
> Michael Schmitz - 27.06.18, 22:13:
>> On Wed, Jun 27, 2018 at 8:24 PM, Martin Steigerwald
> <[e-mail]> wrote:
>>> Thanks a lot again for your patch.
>>>
>>> [e-mail] - 27.06.18, 03:24:
>>>> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
>>>> +                       || (start_sect + nr_sects) > INT_MAX) {
>>>> +                       pr_err("%s: Warning: RDB partition
>>>> overflow!\n", +                               bdevname(state->bdev,
>>>
>>> I´d word this:
>>>
>>> Warning: RDB partition 32-bit overflow
>>>
>>> AmigaOS developers can do 64 bit math on a 32 bit operating system.
>>> Just like Linux can.
>>
>> Yes, I realize that. I hadn't gone back through all the mails on the
>> subject to find out what the exact requrements are on the AmigaOS
>> side.
>>
>> Just trying to be as terse as possible to keep checkpatch happy :-(
>>
>>>> b));
>>>> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
>>>> +                               bdevname(state->bdev, b),
>>>> start_sect,
>>>> +                               nr_sects);
>>>> +                       pr_err("%s: partition incompatible with 32
>>>> bit OS\n", +                               bdevname(state->bdev,
>>>> b)); +               }
>>>
>>> And as stated in my other reply to the patch:
>>>
>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
>>> like
>>> operating systems (NSD64, TD64 or SCSI direct)
>>
>> I'd probably leave it at 'disk needs 64 bit disk device support on
>> native OS', and only print that warning once.
> 
> This is fine with me.
> 
>> Geert has raised another important point about 64 bt device support -
>> all this is moot when the Linux kernel wasn't built with large block
>> device support enabled (you'd get the same buggy behaviour as before
>> the patch there).
>>
>>> see my other reply to the patch and my other mails in the
>>> "Re: moving affs + RDB partition support to staging?" thread as to
>>> why. And for references.
>>
>> Thanks for collating all the references. Please understand that I
>> can't read all of that, and as a simple patch mechanic I won't even
>> try to grasp all the subtleties of RDB (I don't even own an Amiga so I
>> am quite unlikey to ever use this code path).
> 
> I understand that.
> 
>> But please also understand that for that reason, I take Joanne's
>> advice about backwards compatibility very serious. My patch (actually
>> Joanne's originally) changes kernel behaviour from what we consider
>> broken (allowing 32 bit overflow in partition address calculations) to
>> what we think is the right thing to do. But there might be someone
>> out there who used the current behaviour to craft a RDB that aliows
>> two separate sets of partitions to coexist on the same disk (one set
>> visible to 32 bit disk drivers, before the 32 bit overflow mark, and
>> a second set above that mark, visible only to 64 bit drivers.
>> Silently changing our parser behaviour might cause said user to now
>> trash data past the overflow mark.). This is a little contrived, and
>> perhaps I am overcomplicating matters (again), but can't be ruled
>> out.
>>
>> In the interest of least surprises, we have to fix the 32 bit overflow
>> (so we can even detect that it would have happened), and give the
>> user the chance to carefully consider whether to accept the new
>> behaviour. That means refusing to make available any partition that
>> would have been affected by such overflow.
> 
> That is acceptable for me as I told before. Either mount or refuse to
> mount, but do not overflow and mount nonetheless :)
> 
> Mind you, I am not using my Amiga machines either at the moment. And I
> repurposed the 2 TB disk years ago.
>   
>> The user has then all options available - force old behaviour by using
>> an older kernel, override the parser to force new behaviour (which we
>> all assume is correct), or leave the disk well alone.
> 
> Sure.
> 
> I would not name the kernel option "eat_my_rdb", but use a less
> dramatizing name.
> 
> Maybe just: "allow_64bit_rdb" or something like that.
> 
> How does the user come to know about this kernel option? Will you print
> its name in kernel log?
> 
> Thanks,
>

jdow June 28, 2018, 3:49 a.m. UTC | #10

Error NNNN: Conventional RDBs cannot define more than 4,294,967,296 blocks.
or
Error NNNN: Conventional RDB block count overflow.

That is a HARD limit. The documentation for error NNNN should suggest larger 
logical block (cluster, whatever) sizes as a way out. Of course, block size 
"could" go up to at least 65536 bytes (if the nonsense size 0 is reinterpreted). 
Then Bob's your uncle up to 281 TB. It should also include a dry notation that 
you are best off with a power of 2 block size larger than or equal to the disk's 
actual sector size, if you can learn that.

{^_^}
(Get everybody)

On 20180627 01:24, Martin Steigerwald wrote:
> Thanks a lot again for your patch.
> 
> [e-mail] - 27.06.18, 03:24:
>> +               if (start_sect > INT_MAX || nr_sects > INT_MAX
>> +                       || (start_sect + nr_sects) > INT_MAX) {
>> +                       pr_err("%s: Warning: RDB partition
>> overflow!\n", +                               bdevname(state->bdev,
> 
> I´d word this:
> 
> Warning: RDB partition 32-bit overflow
> 
> AmigaOS developers can do 64 bit math on a 32 bit operating system. Just
> like Linux can.
> 
>> b));
>> +                       pr_err("%s: start 0x%llX size 0x%llX\n",
>> +                               bdevname(state->bdev, b), start_sect,
>> +                               nr_sects);
>> +                       pr_err("%s: partition incompatible with 32 bit
>> OS\n", +                               bdevname(state->bdev, b));
>> +               }
> 
> And as stated in my other reply to the patch:
> 
> partition needs 64 bit disk device support in AmigaOS or AmigaOS like
> operating systems (NSD64, TD64 or SCSI direct)
> 
> see my other reply to the patch and my other mails in the
> "Re: moving affs + RDB partition support to staging?" thread as to why.
> And for references.
> 
> Thanks,
>

Michael Schmitz June 28, 2018, 4:58 a.m. UTC | #11

Hi Martin,

Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
>>> And as stated in my other reply to the patch:
>>>
>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
>>> like
>>> operating systems (NSD64, TD64 or SCSI direct)
>>
>> I'd probably leave it at 'disk needs 64 bit disk device support on
>> native OS', and only print that warning once.
>
> This is fine with me.

OK, I'll go with that.

>> In the interest of least surprises, we have to fix the 32 bit overflow
>> (so we can even detect that it would have happened), and give the
>> user the chance to carefully consider whether to accept the new
>> behaviour. That means refusing to make available any partition that
>> would have been affected by such overflow.
>
> That is acceptable for me as I told before. Either mount or refuse to
> mount, but do not overflow and mount nonetheless :)
>
> Mind you, I am not using my Amiga machines either at the moment. And I
> repurposed the 2 TB disk years ago.

That's fine - I understand the 'profile' image was a true binary copy of 
the RDB, and placing that file at offset 0 in an image file is a 
legitimate use?

> I would not name the kernel option "eat_my_rdb", but use a less
> dramatizing name.
>
> Maybe just: "allow_64bit_rdb" or something like that.

I don't expect to get away with that :-)

> How does the user come to know about this kernel option? Will you print
> its name in kernel log?

Depends on how easy we want to make it for users. If I put a BUG() trap 
with the check, the resulting log section will point to a specific line 
in block/partitions/amiga.c, from which the override option will be 
obvious. But that might be a little too opaque for some...

Cheers,

	Michael

Geert Uytterhoeven June 28, 2018, 6:45 a.m. UTC | #12

Hi Michael,

On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> wrote:
> Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
> >>> And as stated in my other reply to the patch:
> >>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
> >>> like
> >>> operating systems (NSD64, TD64 or SCSI direct)
> >>
> >> I'd probably leave it at 'disk needs 64 bit disk device support on
> >> native OS', and only print that warning once.
> >
> > This is fine with me.
>
> OK, I'll go with that.

Do we really need the warning?
Once the parsing is fixed doing 64-bit math, it does not matter for Linux
anymore.

Won't it make more sense to have the warning in the tool that created the
partition table in the first place?

> > I would not name the kernel option "eat_my_rdb", but use a less
> > dramatizing name.
> >
> > Maybe just: "allow_64bit_rdb" or something like that.
>
> I don't expect to get away with that :-)

I still fail to see what's the added value of the kernel option...
Either the partition is usable, or not.

> > How does the user come to know about this kernel option? Will you print
> > its name in kernel log?
>
> Depends on how easy we want to make it for users. If I put a BUG() trap
> with the check, the resulting log section will point to a specific line
> in block/partitions/amiga.c, from which the override option will be
> obvious. But that might be a little too opaque for some...

Please don't use BUG(), unless your goal is to attract attention (from
Linus, who dislikes BUG()!).

Using BUG() would be a nice way to DoS someones machine by plugging in
a USB stick with a malformed RDB.

Gr{oetje,eeting}s,

                        Geert

Martin Steigerwald June 28, 2018, 7:13 a.m. UTC | #13

Hi Geert.

Geert Uytterhoeven - 28.06.18, 08:45: 
> On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> 
wrote:
> > Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
> > >>> And as stated in my other reply to the patch:
> > >>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
> > >>> like
> > >>> operating systems (NSD64, TD64 or SCSI direct)
> > >> 
> > >> I'd probably leave it at 'disk needs 64 bit disk device support
> > >> on
> > >> native OS', and only print that warning once.
> > > 
> > > This is fine with me.
> > 
> > OK, I'll go with that.
> 
> Do we really need the warning?
> Once the parsing is fixed doing 64-bit math, it does not matter for
> Linux anymore.

Well, irony of this is: In my case the RDB has been created on a machine 
with a native OS. So Linux warns me about something I already did so on 
the native OS without any warning. In this case AmigaOS 4.0.
 
> Won't it make more sense to have the warning in the tool that created
> the partition table in the first place?

Well that would be up to the AmigaOS developers to decide.

And well for amiga-fdisk or parted developers if they ever choose to 
support this or already do. (I doubt that amiga-fdisk can handle this.)

> > > I would not name the kernel option "eat_my_rdb", but use a less
> > > dramatizing name.
> > > 
> > > Maybe just: "allow_64bit_rdb" or something like that.
> > 
> > I don't expect to get away with that :-)
> 
> I still fail to see what's the added value of the kernel option...
> Either the partition is usable, or not.

Well, I could try to contact some of the current AmigaOS developers 
about that and ask them whether they would like to give me a statement 
about this that I am allowed to post here.

I would not know whether they answer and it may take a time. My offer 
stands, but I would only do this, if you really like to have that 
official feedback.

Again, I am pretty sure that what I did is safe on AmigaOS 4 at least, 
but I bet also on AmigaOS <4 with NSD64 or TD64 (except for the 
filesystem sizes, but AmigaOS < 4 does not have JXFS anyway, and did not 
have SFS2 as well, maybe that is available now, I don´t know).

However Joanne is without doubt an authority on RDBs, but she has not 
been involved with AmigaOS development for quite some time and, correct 
me if this is wrong, Joanne, does not know as much about the recent 
versions, as I or even more so as current AmigaOS developers know.

Thanks,

Martin Steigerwald June 28, 2018, 7:28 a.m. UTC | #14

Hi Michael.

Probably I was right with not submitting a patch myself. I´d likely 
would have been overwhelmed by the discussion and feedback :)

Michael Schmitz - 28.06.18, 06:58:
[…]
> >> In the interest of least surprises, we have to fix the 32 bit
> >> overflow (so we can even detect that it would have happened), and
> >> give the user the chance to carefully consider whether to accept
> >> the new behaviour. That means refusing to make available any
> >> partition that would have been affected by such overflow.
> > 
> > That is acceptable for me as I told before. Either mount or refuse
> > to
> > mount, but do not overflow and mount nonetheless :)
> > 
> > Mind you, I am not using my Amiga machines either at the moment. And
> > I repurposed the 2 TB disk years ago.
> 
> That's fine - I understand the 'profile' image was a true binary copy
> of the RDB, and placing that file at offset 0 in an image file is a
> legitimate use?

You actually ask me to remember about what that 'profile' image was? :)

Well, in the attachment note on the bug report I wrote: "should be just 
a binary copy", so I did not know exactly back then either. However the 
file starts with "RDSK" and then it has "PART" headers and so on. That 
looks pretty much like a binary copy of an RDB. I am a bit surprised by 
its small size of 2 KiB. But I see three partitions in there. According 
to the screenshot I also provided, the disk had three partitions. So 
probably Media Toolbox has been intelligent enough to just copy the used 
space of the reserved RDB area. Cause I think the reserved space must 
have been higher than 2 KiB. However the RDB/disk geometry editing 
screen does not display it and off hand I do not know where to look 
inside the RDB to see how much space has been reserved. Interestingly 
the "Total sectors" value in that Media Toolbox window also overflowed. 
But from my memory this was just a cosmetic issue in Media Toolbox. The 
"*.device" device drivers, the filesystems and the RDB handling code in 
AmigaOS do their own math. That is what NSD64 / TD64 was about.

https://bugzilla.kernel.org/show_bug.cgi?id=43511

> > I would not name the kernel option "eat_my_rdb", but use a less
> > dramatizing name.
> > 
> > Maybe just: "allow_64bit_rdb" or something like that.
> 
> I don't expect to get away with that :-)

Heh. :)

> > How does the user come to know about this kernel option? Will you
> > print its name in kernel log?
> 
> Depends on how easy we want to make it for users. If I put a BUG()
> trap with the check, the resulting log section will point to a
> specific line in block/partitions/amiga.c, from which the override
> option will be obvious. But that might be a little too opaque for
> some...

kernel-parameters.txt or mentioning in the warning would also be an 
option.

Thanks,

Geert Uytterhoeven June 28, 2018, 7:39 a.m. UTC | #15

Hi Martin,

On Thu, Jun 28, 2018 at 9:29 AM Martin Steigerwald <[e-mail]> wrote:
> Michael Schmitz - 28.06.18, 06:58:
> […]
> > >> In the interest of least surprises, we have to fix the 32 bit
> > >> overflow (so we can even detect that it would have happened), and
> > >> give the user the chance to carefully consider whether to accept
> > >> the new behaviour. That means refusing to make available any
> > >> partition that would have been affected by such overflow.
> > >
> > > That is acceptable for me as I told before. Either mount or refuse
> > > to
> > > mount, but do not overflow and mount nonetheless :)
> > >
> > > Mind you, I am not using my Amiga machines either at the moment. And
> > > I repurposed the 2 TB disk years ago.
> >
> > That's fine - I understand the 'profile' image was a true binary copy
> > of the RDB, and placing that file at offset 0 in an image file is a
> > legitimate use?
>
> You actually ask me to remember about what that 'profile' image was? :)
>
> Well, in the attachment note on the bug report I wrote: "should be just
> a binary copy", so I did not know exactly back then either. However the
> file starts with "RDSK" and then it has "PART" headers and so on. That
> looks pretty much like a binary copy of an RDB. I am a bit surprised by
> its small size of 2 KiB. But I see three partitions in there. According
> to the screenshot I also provided, the disk had three partitions. So
> probably Media Toolbox has been intelligent enough to just copy the used
> space of the reserved RDB area. Cause I think the reserved space must
> have been higher than 2 KiB. However the RDB/disk geometry editing
> screen does not display it and off hand I do not know where to look
> inside the RDB to see how much space has been reserved. Interestingly
> the "Total sectors" value in that Media Toolbox window also overflowed.

The RDB can be anywhere in the first 2 tracks of the disk, and is identified
by the "RDSK" block (with a correct checksum). The remainder (e.g. "PART"
blocks) is in a linked list. So 2 KiB sounds fine for 3 partitions (1 RDSK +
3 * PART = 4 blocks = 4 * 512 bytes).

Gr{oetje,eeting}s,

                        Geert

jdow June 28, 2018, 9:20 a.m. UTC | #16

On 20180627 23:45, Geert Uytterhoeven wrote:
> Hi Michael,
> 
> On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> wrote:
>> Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
>>>>> And as stated in my other reply to the patch:
>>>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
>>>>> like
>>>>> operating systems (NSD64, TD64 or SCSI direct)
>>>>
>>>> I'd probably leave it at 'disk needs 64 bit disk device support on
>>>> native OS', and only print that warning once.
>>>
>>> This is fine with me.
>>
>> OK, I'll go with that.
> 
> Do we really need the warning?
> Once the parsing is fixed doing 64-bit math, it does not matter for Linux
> anymore.

Dual booting.
{^_^}

Geert Uytterhoeven June 28, 2018, 9:25 a.m. UTC | #17

Hi Martin,

On Thu, Jun 28, 2018 at 9:13 AM Martin Steigerwald <[e-mail]> wrote:
> Geert Uytterhoeven - 28.06.18, 08:45:
> > On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]>
> wrote:
> > > Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
> > > >>> And as stated in my other reply to the patch:
> > > >>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
> > > >>> like
> > > >>> operating systems (NSD64, TD64 or SCSI direct)
> > > >>
> > > >> I'd probably leave it at 'disk needs 64 bit disk device support
> > > >> on
> > > >> native OS', and only print that warning once.
> > > >
> > > > This is fine with me.
> > >
> > > OK, I'll go with that.
> >
> > Do we really need the warning?
> > Once the parsing is fixed doing 64-bit math, it does not matter for
> > Linux anymore.
>
> Well, irony of this is: In my case the RDB has been created on a machine
> with a native OS. So Linux warns me about something I already did so on
> the native OS without any warning. In this case AmigaOS 4.0.

Exactly.

So moving a disk partitioned under AmigaOS 4.0 to a system running an
older version of AmigaOS can fail miserably. Not a Linux issue.
Linux also doesn't warn about disks with GPT failing to work on old MSDOS.

> > > > I would not name the kernel option "eat_my_rdb", but use a less
> > > > dramatizing name.
> > > >
> > > > Maybe just: "allow_64bit_rdb" or something like that.
> > >
> > > I don't expect to get away with that :-)
> >
> > I still fail to see what's the added value of the kernel option...
> > Either the partition is usable, or not.
>
> Well, I could try to contact some of the current AmigaOS developers
> about that and ask them whether they would like to give me a statement
> about this that I am allowed to post here.
>
> I would not know whether they answer and it may take a time. My offer
> stands, but I would only do this, if you really like to have that
> official feedback.

Let me clarify: what exactly would the kernel option allow? When to use it?

> Again, I am pretty sure that what I did is safe on AmigaOS 4 at least,
> but I bet also on AmigaOS <4 with NSD64 or TD64 (except for the
> filesystem sizes, but AmigaOS < 4 does not have JXFS anyway, and did not
> have SFS2 as well, maybe that is available now, I don´t know).
>
> However Joanne is without doubt an authority on RDBs, but she has not

As a former AmigaOS user, I'm fully aware of that (Thanks Joanne! ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Geert Uytterhoeven June 28, 2018, 9:29 a.m. UTC | #18

Hi Joanne,

On Thu, Jun 28, 2018 at 11:20 AM jdow <[e-mail]> wrote:
> On 20180627 23:45, Geert Uytterhoeven wrote:
> > On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> wrote:
> >> Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
> >>>>> And as stated in my other reply to the patch:
> >>>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
> >>>>> like
> >>>>> operating systems (NSD64, TD64 or SCSI direct)
> >>>>
> >>>> I'd probably leave it at 'disk needs 64 bit disk device support on
> >>>> native OS', and only print that warning once.
> >>>
> >>> This is fine with me.
> >>
> >> OK, I'll go with that.
> >
> > Do we really need the warning?
> > Once the parsing is fixed doing 64-bit math, it does not matter for Linux
> > anymore.
>
> Dual booting.

(Dial/Triple/...) Booting older AmigaOS variants is an issue anyway, with
or without Linux.

It's like _Linux_ printing warnings that you cannot use your disk, created with
Solaris 2, on an old SunOS 4.1 machine ;-)

IMHO (s)he who creates partitions on large disks should make sure (s)he only
uses them on machines that can handle them.  This is not a Linux issue.

Gr{oetje,eeting}s,

                        Geert

jdow June 28, 2018, 9:34 a.m. UTC | #19

On 20180628 00:39, Geert Uytterhoeven wrote:
> Hi Martin,
> 
> On Thu, Jun 28, 2018 at 9:29 AM Martin Steigerwald <[e-mail]> wrote:
>> Michael Schmitz - 28.06.18, 06:58:
>> […]
>>>>> In the interest of least surprises, we have to fix the 32 bit
>>>>> overflow (so we can even detect that it would have happened), and
>>>>> give the user the chance to carefully consider whether to accept
>>>>> the new behaviour. That means refusing to make available any
>>>>> partition that would have been affected by such overflow.
>>>>
>>>> That is acceptable for me as I told before. Either mount or refuse
>>>> to
>>>> mount, but do not overflow and mount nonetheless :)
>>>>
>>>> Mind you, I am not using my Amiga machines either at the moment. And
>>>> I repurposed the 2 TB disk years ago.
>>>
>>> That's fine - I understand the 'profile' image was a true binary copy
>>> of the RDB, and placing that file at offset 0 in an image file is a
>>> legitimate use?
>>
>> You actually ask me to remember about what that 'profile' image was? :)
>>
>> Well, in the attachment note on the bug report I wrote: "should be just
>> a binary copy", so I did not know exactly back then either. However the
>> file starts with "RDSK" and then it has "PART" headers and so on. That
>> looks pretty much like a binary copy of an RDB. I am a bit surprised by
>> its small size of 2 KiB. But I see three partitions in there. According
>> to the screenshot I also provided, the disk had three partitions. So
>> probably Media Toolbox has been intelligent enough to just copy the used
>> space of the reserved RDB area. Cause I think the reserved space must
>> have been higher than 2 KiB. However the RDB/disk geometry editing
>> screen does not display it and off hand I do not know where to look
>> inside the RDB to see how much space has been reserved. Interestingly
>> the "Total sectors" value in that Media Toolbox window also overflowed.
> 
> The RDB can be anywhere in the first 2 tracks of the disk, and is identified
> by the "RDSK" block (with a correct checksum). The remainder (e.g. "PART"
> blocks) is in a linked list. So 2 KiB sounds fine for 3 partitions (1 RDSK +
> 3 * PART = 4 blocks = 4 * 512 bytes).

The RDB can be anywhere in the first 16 blocks on the disk if we are speaking 
officially. That's the entire area that is guaranteed to be inspected for an 
RDSK block. The PART block can, I think, even be located in front of the RDSK if 
you want to be obscene about it. {^_-} That's the kind of thing I checked with 
the HardFrame device driver ROM image. I preferred block 3 to keep it away from 
the space used by other partitioning schemes. It also enabled me to embed it a 
reserved area within the first actual partition just for the halibut. (Pronounce 
it sideways and it makes more sense.) I used that technique fairly often. If you 
think about it that gives you a wee tiny bit more disk space because you can 
tailor the reserved area to precisely fit the filesystem image plus some extra 
in case of updates. I toyed with using a pointer to FDSK blocks in the Dev 
directory but that got too insane. RDBs are insanely flexible, which may not be 
a good thing.

{^_^}

Michael Schmitz June 29, 2018, 8:42 a.m. UTC | #20

Hi Geert,


Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
>
>>> Do we really need the warning?
>>> Once the parsing is fixed doing 64-bit math, it does not matter for
>>> Linux anymore.
>> Well, irony of this is: In my case the RDB has been created on a machine
>> with a native OS. So Linux warns me about something I already did so on
>> the native OS without any warning. In this case AmigaOS 4.0.
> Exactly.
>
> So moving a disk partitioned under AmigaOS 4.0 to a system running an
> older version of AmigaOS can fail miserably. Not a Linux issue.
> Linux also doesn't warn about disks with GPT failing to work on old MSDOS.

Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
to use it?

> Let me clarify: what exactly would the kernel option allow? When to use it?

Whether to use it if safe (on Linux). But whatever Linux does (after
this patch), access will go to the right area of the disk (as specified
by the RDB) so Linux won't any longer stomp on anything that would have
mattered to 32 bit disk drivers. So it really should be safe.

Cheers,

    Michael

Geert Uytterhoeven June 29, 2018, 8:51 a.m. UTC | #21

Hi Michael,

On Fri, Jun 29, 2018 at 10:43 AM Michael Schmitz <[e-mail]> wrote:
> Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
> >>> Do we really need the warning?
> >>> Once the parsing is fixed doing 64-bit math, it does not matter for
> >>> Linux anymore.
> >> Well, irony of this is: In my case the RDB has been created on a machine
> >> with a native OS. So Linux warns me about something I already did so on
> >> the native OS without any warning. In this case AmigaOS 4.0.
> > Exactly.
> >
> > So moving a disk partitioned under AmigaOS 4.0 to a system running an
> > older version of AmigaOS can fail miserably. Not a Linux issue.
> > Linux also doesn't warn about disks with GPT failing to work on old MSDOS.
>
> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> to use it?

No idea...

Probably some old Windows or MacOS versions will just suggest to
format your "new" disk ;-)

But it's up to the person (which is not Linux) formatting the disk to
not try to use
it on systems that cannot handle it, and may destroy it.

> > Let me clarify: what exactly would the kernel option allow? When to use it?
>
> Whether to use it if safe (on Linux). But whatever Linux does (after
> this patch), access will go to the right area of the disk (as specified
> by the RDB) so Linux won't any longer stomp on anything that would have
> mattered to 32 bit disk drivers. So it really should be safe.

Personally, I see no reason to depend on a kernel option, if it is safe to use.
Just use it.

Gr{oetje,eeting}s,

                        Geert

Michael Schmitz June 29, 2018, 8:58 a.m. UTC | #22

Hi Geert,


Am 28.06.18 um 18:45 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> wrote:
>> Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
>>>>> And as stated in my other reply to the patch:
>>>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
>>>>> like
>>>>> operating systems (NSD64, TD64 or SCSI direct)
>>>> I'd probably leave it at 'disk needs 64 bit disk device support on
>>>> native OS', and only print that warning once.
>>> This is fine with me.
>> OK, I'll go with that.
> Do we really need the warning?
> Once the parsing is fixed doing 64-bit math, it does not matter for Linux
> anymore.
>
> Won't it make more sense to have the warning in the tool that created the
> partition table in the first place?
Sure, but we've seen one case of this in the wild, and the tool
apparently did not issue a warning.

I agree it's not an issue for Linux. If  you think dropping a warning
about something not actually relevant to Linux in the log would be
confusing, or if convention is to limit warnings strictly to behaviour
relevant to Linux, I can live without the warning. Joanne scared me a
bit about Amigoids angry at data loss, but I suppose there can't be many
around my neck of the woods. 

>
>>> I would not name the kernel option "eat_my_rdb", but use a less
>>> dramatizing name.
>>>
>>> Maybe just: "allow_64bit_rdb" or something like that.
>> I don't expect to get away with that :-)
> I still fail to see what's the added value of the kernel option...
> Either the partition is usable, or not.

The question is - can writes to the disk cause any damage to data on the
disk, as seen by old OS versions? If the answer is no, we won't need the
option after all.

>>> How does the user come to know about this kernel option? Will you print
>>> its name in kernel log?
>> Depends on how easy we want to make it for users. If I put a BUG() trap
>> with the check, the resulting log section will point to a specific line
>> in block/partitions/amiga.c, from which the override option will be
>> obvious. But that might be a little too opaque for some...
> Please don't use BUG(), unless your goal is to attract attention (from
> Linus, who dislikes BUG()!).
I'd rather not abuse his patience. Thanks for the hint.
> Using BUG() would be a nice way to DoS someones machine by plugging in
> a USB stick with a malformed RDB.
I guess I deserved that. But BUG() doesn't panic now, does it? The ones
I saw did allow the kernel to happily carry on.

Cheers,

    Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Michael Schmitz June 29, 2018, 9:07 a.m. UTC | #23

Hi Geert,


Am 29.06.18 um 20:51 schrieb Geert Uytterhoeven:
> Hi Michael,
>
>> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
>> to use it?
> No idea...
>
> Probably some old Windows or MacOS versions will just suggest to
> format your "new" disk ;-)

Yep, that's what I'd expect. Windows even used to trash the LILO boot
code in the MBR if you just happened to boot to it by accident. Did we
complain?

>
> But it's up to the person (which is not Linux) formatting the disk to
> not try to use
> it on systems that cannot handle it, and may destroy it.
>
>>> Let me clarify: what exactly would the kernel option allow? When to use it?
>> Whether to use it if safe (on Linux). But whatever Linux does (after
>> this patch), access will go to the right area of the disk (as specified
>> by the RDB) so Linux won't any longer stomp on anything that would have
>> mattered to 32 bit disk drivers. So it really should be safe.
> Personally, I see no reason to depend on a kernel option, if it is safe to use.
> Just use it.

So to recap - someone partitions a disk on AmigaOS 4.x, taking advantage
of the large block device support there.
Using that disk on AmigaOS 3.1, data loss ensues. Whether or not Linux
(patched) ever touched the disk has no impact on that outcome.

Right?

Cheers,

    Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Geert Uytterhoeven June 29, 2018, 9:10 a.m. UTC | #24

Hi Michael,

On Fri, Jun 29, 2018 at 10:58 AM Michael Schmitz <[e-mail]> wrote:
> Am 28.06.18 um 18:45 schrieb Geert Uytterhoeven:
> > On Thu, Jun 28, 2018 at 6:59 AM Michael Schmitz <[e-mail]> wrote:
> >> Am 28.06.2018 um 09:20 schrieb Martin Steigerwald:
> >>>>> And as stated in my other reply to the patch:
> >>>>> partition needs 64 bit disk device support in AmigaOS or AmigaOS
> >>>>> like
> >>>>> operating systems (NSD64, TD64 or SCSI direct)
> >>>> I'd probably leave it at 'disk needs 64 bit disk device support on
> >>>> native OS', and only print that warning once.
> >>> This is fine with me.
> >> OK, I'll go with that.
> > Do we really need the warning?
> > Once the parsing is fixed doing 64-bit math, it does not matter for Linux
> > anymore.
> >
> > Won't it make more sense to have the warning in the tool that created the
> > partition table in the first place?
> Sure, but we've seen one case of this in the wild, and the tool
> apparently did not issue a warning.
>
> I agree it's not an issue for Linux. If  you think dropping a warning
> about something not actually relevant to Linux in the log would be
> confusing, or if convention is to limit warnings strictly to behaviour
> relevant to Linux, I can live without the warning. Joanne scared me a
> bit about Amigoids angry at data loss, but I suppose there can't be many
> around my neck of the woods.
>
> >
> >>> I would not name the kernel option "eat_my_rdb", but use a less
> >>> dramatizing name.
> >>>
> >>> Maybe just: "allow_64bit_rdb" or something like that.
> >> I don't expect to get away with that :-)
> > I still fail to see what's the added value of the kernel option...
> > Either the partition is usable, or not.
>
> The question is - can writes to the disk cause any damage to data on the
> disk, as seen by old OS versions? If the answer is no, we won't need the
> option after all.

You mean someone relying on the parameters of his RDB to overflow using
32-bit calculations, and still have valid offsets on the disk so it's usable?
I think that would need hand-crafting an RDB, if possible at all.
And writing to it on AmigaOS 4.0 or any other OS doing proper 64-bit
calculations would write to the wrong locations, too.
IMHO not something to consider.

> >>> How does the user come to know about this kernel option? Will you print
> >>> its name in kernel log?
> >> Depends on how easy we want to make it for users. If I put a BUG() trap
> >> with the check, the resulting log section will point to a specific line
> >> in block/partitions/amiga.c, from which the override option will be
> >> obvious. But that might be a little too opaque for some...
> > Please don't use BUG(), unless your goal is to attract attention (from
> > Linus, who dislikes BUG()!).
> I'd rather not abuse his patience. Thanks for the hint.
> > Using BUG() would be a nice way to DoS someones machine by plugging in
> > a USB stick with a malformed RDB.
> I guess I deserved that. But BUG() doesn't panic now, does it? The ones
> I saw did allow the kernel to happily carry on.

The one in asm-generic does call panic().
The m68k one calls __builtin_trap(), which may cause a trap (and panic?) or
do nothing, depending on gcc version, I think.

Gr{oetje,eeting}s,

                        Geert

Michael Schmitz June 29, 2018, 9:12 a.m. UTC | #25

Hi Geert,


Am 28.06.18 um 01:30 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> Thanks for your patch!
>
> On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
>> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
> ??
>
>> The Amiga RDB partition parser module uses int for partition sector
>> address and count, which will overflow for disks 2 TB and larger.
>>
>> Use sector_t as type for sector address and size (as expected by
>> put_partition) to allow using such disks without danger of data
>> corruption.
> Note that sector_t is not guaranteed to be 64-bit:
>
>     #ifdef CONFIG_LBDAF
>     typedef u64 sector_t;
>     typedef u64 blkcnt_t;
>     #else
>     typedef unsigned long sector_t;
>     typedef unsigned long blkcnt_t;
>     #endif
>
> And it seems CONFIG_LBDAF can still be disabled on 32-bit...
>

What are the ramifications of using a 2 TB disk on a kernel without
CONFIG_LBDAF? Are there any safeguards after the partition scan stage
that would prevent the kernel from using partitions on such a disk?

Trying to decide whether we absolutely have to bail if sector_t is 32
bit ...

Cheers,

    Michael

Geert Uytterhoeven June 29, 2018, 9:12 a.m. UTC | #26

Hi Michael,

On Fri, Jun 29, 2018 at 11:08 AM Michael Schmitz <[e-mail]> wrote:
> Am 29.06.18 um 20:51 schrieb Geert Uytterhoeven:
> >> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> >> to use it?
> > No idea...
> >
> > Probably some old Windows or MacOS versions will just suggest to
> > format your "new" disk ;-)
>
> Yep, that's what I'd expect. Windows even used to trash the LILO boot
> code in the MBR if you just happened to boot to it by accident. Did we
> complain?

Those days Linux users just knew not to use Windows ;-)

> > But it's up to the person (which is not Linux) formatting the disk to
> > not try to use
> > it on systems that cannot handle it, and may destroy it.
> >
> >>> Let me clarify: what exactly would the kernel option allow? When to use it?
> >> Whether to use it if safe (on Linux). But whatever Linux does (after
> >> this patch), access will go to the right area of the disk (as specified
> >> by the RDB) so Linux won't any longer stomp on anything that would have
> >> mattered to 32 bit disk drivers. So it really should be safe.
> > Personally, I see no reason to depend on a kernel option, if it is safe to use.
> > Just use it.
>
> So to recap - someone partitions a disk on AmigaOS 4.x, taking advantage
> of the large block device support there.
> Using that disk on AmigaOS 3.1, data loss ensues. Whether or not Linux
> (patched) ever touched the disk has no impact on that outcome.
>
> Right?

That's my understanding, too.

Gr{oetje,eeting}s,

                        Geert

Michael Schmitz June 29, 2018, 9:19 a.m. UTC | #27

Hi Geert,


Am 29.06.18 um 21:10 schrieb Geert Uytterhoeven:
>
>> The question is - can writes to the disk cause any damage to data on the
>> disk, as seen by old OS versions? If the answer is no, we won't need the
>> option after all.
> You mean someone relying on the parameters of his RDB to overflow using
> 32-bit calculations, and still have valid offsets on the disk so it's usable?
> I think that would need hand-crafting an RDB, if possible at all.
> And writing to it on AmigaOS 4.0 or any other OS doing proper 64-bit
> calculations would write to the wrong locations, too.
> IMHO not something to consider.

Something like that. But I haven't stopped for long enough to work out
if that was even possible.

>
>>>>> How does the user come to know about this kernel option? Will you print
>>>>> its name in kernel log?
>>>> Depends on how easy we want to make it for users. If I put a BUG() trap
>>>> with the check, the resulting log section will point to a specific line
>>>> in block/partitions/amiga.c, from which the override option will be
>>>> obvious. But that might be a little too opaque for some...
>>> Please don't use BUG(), unless your goal is to attract attention (from
>>> Linus, who dislikes BUG()!).
>> I'd rather not abuse his patience. Thanks for the hint.
>>> Using BUG() would be a nice way to DoS someones machine by plugging in
>>> a USB stick with a malformed RDB.
>> I guess I deserved that. But BUG() doesn't panic now, does it? The ones
>> I saw did allow the kernel to happily carry on.
> The one in asm-generic does call panic().

Ouch - that explains Linus's aversion.

> The m68k one calls __builtin_trap(), which may cause a trap (and panic?) or
> do nothing, depending on gcc version, I think.

The trap doesn't cause a panic for me. Might rely on the trap vector
being set up right (which we can rely on at partition scan time), and
the context though. But if m68k is the odd one out in gracefully
handling BUG(), better leave it.

Cheers,

    Michael
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Michael Schmitz June 29, 2018, 9:25 a.m. UTC | #28

Hi Geert,

Am 29.06.18 um 21:12 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Fri, Jun 29, 2018 at 11:08 AM Michael Schmitz <[e-mail]> wrote:
>> Am 29.06.18 um 20:51 schrieb Geert Uytterhoeven:
>>>> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
>>>> to use it?
>>> No idea...
>>>
>>> Probably some old Windows or MacOS versions will just suggest to
>>> format your "new" disk ;-)
>> Yep, that's what I'd expect. Windows even used to trash the LILO boot
>> code in the MBR if you just happened to boot to it by accident. Did we
>> complain?
> Those days Linux users just knew not to use Windows ;-)

For good reason. But that happened on a couple of instrument control PCs
I converted from Win2k. Never mount a scratch monkey... 

>>> Personally, I see no reason to depend on a kernel option, if it is safe to use.
>>> Just use it.
>> So to recap - someone partitions a disk on AmigaOS 4.x, taking advantage
>> of the large block device support there.
>> Using that disk on AmigaOS 3.1, data loss ensues. Whether or not Linux
>> (patched) ever touched the disk has no impact on that outcome.
>>
>> Right?
> That's my understanding, too.

OK, that decides it.

Cheers,

    Michael
> Gr{oetje,eeting}s,
>
>                         Geert
>

jdow June 29, 2018, 9:32 a.m. UTC | #29

On 20180629 01:42, Michael Schmitz wrote:
> Hi Geert,
> 
> 
> Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
>>
>>>> Do we really need the warning?
>>>> Once the parsing is fixed doing 64-bit math, it does not matter for
>>>> Linux anymore.
>>> Well, irony of this is: In my case the RDB has been created on a machine
>>> with a native OS. So Linux warns me about something I already did so on
>>> the native OS without any warning. In this case AmigaOS 4.0.
>> Exactly.
>>
>> So moving a disk partitioned under AmigaOS 4.0 to a system running an
>> older version of AmigaOS can fail miserably. Not a Linux issue.
>> Linux also doesn't warn about disks with GPT failing to work on old MSDOS.
> 
> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> to use it?

I believe it would fail to mount it and suggest formatting it. The key is to 
fail the mount and not risk damage to the data on it.

>> Let me clarify: what exactly would the kernel option allow? When to use it?
> 
> Whether to use it if safe (on Linux). But whatever Linux does (after
> this patch), access will go to the right area of the disk (as specified
> by the RDB) so Linux won't any longer stomp on anything that would have
> mattered to 32 bit disk drivers. So it really should be safe.

Martin pointed out something privately about RDBs I'd forgotten over the 
archeological eras gone past. The RDB values are all 32 bit unsigned longs. That 
means as long as they are properly promoted AND the file system does not barf 
from a gross overfeeding disks up to about 2^128 bytes can be supported. But you 
need a new uint128_t datatype. So for Amigas or Linux the real limitation is the 
data type and significance that flows between the RDB parser and the file 
system. That is the value that has to be considered. Then if the filesystem 
somewhere inside coughs up a hairball over some internal overflow or other and 
cannot accept say a full uint64_t maximum value of bytes with grace and polish 
it should error out when it sees an incoming disk positions off its internally 
acceptable range.

This means that any errors and failures to mount should lie with the RDB Parser, 
the filesystem, the device driver, and any intervening code between the three. 
The RDBs can express a value far greater than the number of atoms estimated to 
exist in the observable universe. So RDBs won't ever need a change for disk size 
considerations. The parser and filesystems probably and possibly, respectively, 
will.

Error out and don't mount disks or partitions that are outside the size range 
your element of the datapath from disk to user's program can handle at the first 
place this can be detected. It's the polite thing to do to a user's data rather 
than the Maxine Waters thing to do.

{^_^}

Andreas Schwab June 29, 2018, 12:44 p.m. UTC | #30

On Jun 29 2018, Michael Schmitz <[e-mail]> wrote:

> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> to use it?

GPT has the concept of a protective MBR which should prevent such errors.

Andreas.

Martin Steigerwald June 29, 2018, 9:10 p.m. UTC | #31

Hi Michael.

Michael Schmitz - 29.06.18, 10:42:
> Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
> >>> Do we really need the warning?
> >>> Once the parsing is fixed doing 64-bit math, it does not matter
> >>> for
> >>> Linux anymore.
> >> 
> >> Well, irony of this is: In my case the RDB has been created on a
> >> machine with a native OS. So Linux warns me about something I
> >> already did so on the native OS without any warning. In this case
> >> AmigaOS 4.0.> 
> > Exactly.
> > 
> > So moving a disk partitioned under AmigaOS 4.0 to a system running
> > an
> > older version of AmigaOS can fail miserably. Not a Linux issue.
> > Linux also doesn't warn about disks with GPT failing to work on old
> > MSDOS.
> Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> to use it?

As far as I know most GPT partitioning tools create a fake MBR with one 
large partition of a partition type for GPT to tell legacy applications 
to leave the disk alone.

Thanks,

Martin Steigerwald June 29, 2018, 9:17 p.m. UTC | #32

Hi,

Geert Uytterhoeven - 29.06.18, 10:51:
> On Fri, Jun 29, 2018 at 10:43 AM Michael Schmitz 
<[e-mail]> wrote:
> > Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
> > >>> Do we really need the warning?
> > >>> Once the parsing is fixed doing 64-bit math, it does not matter
> > >>> for
> > >>> Linux anymore.
> > >> 
> > >> Well, irony of this is: In my case the RDB has been created on a
> > >> machine with a native OS. So Linux warns me about something I
> > >> already did so on the native OS without any warning. In this
> > >> case AmigaOS 4.0.> > 
> > > Exactly.
> > > 
> > > So moving a disk partitioned under AmigaOS 4.0 to a system running
> > > an
> > > older version of AmigaOS can fail miserably. Not a Linux issue.
> > > Linux also doesn't warn about disks with GPT failing to work on
> > > old MSDOS.> 
> > Would MSDOS recognize the GPT partition as 'probably FAT', and
> > attempt to use it?
> 
> No idea...
> 
> Probably some old Windows or MacOS versions will just suggest to
> format your "new" disk ;-)

Heh… MacOS X last I saw it just offers to initialize any disk it does 
not know about. I had this with a friend who uses Mac quite some years 
ago with an external harddrive with Linux filesystems. I told him Mac OS 
X would not understand it, but he did not believe me. Well luckily 
enough I have been quick enough to unplug the USB cable before he could 
hit the initialize button. And yes, that is right, Mac OS X at that time 
did not even tell the user that initializing means *formatting the disk 
and making all data that was previously on it unavailable*. I did not 
even believe it that they had no stern warning about initializing in the 
dialog window. I hope they improved the wording meanwhile.

Thanks,

Martin Steigerwald June 29, 2018, 9:24 p.m. UTC | #33

Hi Michael.

Michael Schmitz - 29.06.18, 11:07:
> > But it's up to the person (which is not Linux) formatting the disk
> > to
> > not try to use
> > it on systems that cannot handle it, and may destroy it.
> > 
> >>> Let me clarify: what exactly would the kernel option allow? When
> >>> to use it?>> 
> >> Whether to use it if safe (on Linux). But whatever Linux does
> >> (after
> >> this patch), access will go to the right area of the disk (as
> >> specified by the RDB) so Linux won't any longer stomp on anything
> >> that would have mattered to 32 bit disk drivers. So it really
> >> should be safe.> 
> > Personally, I see no reason to depend on a kernel option, if it is
> > safe to use. Just use it.
> 
> So to recap - someone partitions a disk on AmigaOS 4.x, taking
> advantage of the large block device support there.
> Using that disk on AmigaOS 3.1, data loss ensues. Whether or not Linux
> (patched) ever touched the disk has no impact on that outcome.

I am not even completely sure about that. Frankly I have no idea what 
would happen when using such a disk on AmigaOS 3.1 *without* NSDPatch or 
TD64 support (I think you could patch AmigaOS 3.1 with 64 Bit support 
already and some 3rd party harddisk controllers by Phase 5 hat TD64 
support at that time already). Unless I try it, which I won´t at the 
moment, I´d say the behaviour is largely undefined.

But hey, undefined means it may just overwrite start overwriting from 
the beginning of the disk beyond 32 bit. And I think that is quite 
likely. It could also crash, but if its an overflow I don´t think why it 
would crash. Anyway, I never tried this out.

But in any way: This would happen or not happen no matter whether Linux 
parsed the RDB or not.

I still think that the native OS warning really does not hurt… but I´d 
spare myself the kernel option. Having the warning without the kernel 
option would be a compromise between being cautious and being bold :).

Thanks,

Martin Steigerwald June 29, 2018, 9:45 p.m. UTC | #34

Beware: Essay ahead which proofs it to the point that there is no 
overflow in RDB before 96 bits maximum value of sectors:

jdow - 29.06.18, 11:32:
> On 20180629 01:42, Michael Schmitz wrote:
> > Hi Geert,
> > 
> > Am 28.06.18 um 21:25 schrieb Geert Uytterhoeven:
> >>>> Do we really need the warning?
> >>>> Once the parsing is fixed doing 64-bit math, it does not matter
> >>>> for
> >>>> Linux anymore.
> >>> 
> >>> Well, irony of this is: In my case the RDB has been created on a
> >>> machine with a native OS. So Linux warns me about something I
> >>> already did so on the native OS without any warning. In this case
> >>> AmigaOS 4.0.>> 
> >> Exactly.
> >> 
> >> So moving a disk partitioned under AmigaOS 4.0 to a system running
> >> an
> >> older version of AmigaOS can fail miserably. Not a Linux issue.
> >> Linux also doesn't warn about disks with GPT failing to work on old
> >> MSDOS.> 
> > Would MSDOS recognize the GPT partition as 'probably FAT', and
> > attempt to use it?
> 
> I believe it would fail to mount it and suggest formatting it. The key
> is to fail the mount and not risk damage to the data on it.
> 
> >> Let me clarify: what exactly would the kernel option allow? When to
> >> use it?> 
> > Whether to use it if safe (on Linux). But whatever Linux does (after
> > this patch), access will go to the right area of the disk (as
> > specified by the RDB) so Linux won't any longer stomp on anything
> > that would have mattered to 32 bit disk drivers. So it really
> > should be safe.
> Martin pointed out something privately about RDBs I'd forgotten over
> the archeological eras gone past. The RDB values are all 32 bit
> unsigned longs. That means as long as they are properly promoted AND
> the file system does not barf from a gross overfeeding disks up to
> about 2^128 bytes can be supported. But you need a new uint128_t
> datatype. So for Amigas or Linux the real limitation is the data type
> and significance that flows between the RDB parser and the file
> system. That is the value that has to be considered. Then if the
> filesystem somewhere inside coughs up a hairball over some internal
> overflow or other and cannot accept say a full uint64_t maximum value
> of bytes with grace and polish it should error out when it sees an
> incoming disk positions off its internally acceptable range.

I think I pointed it out publically as well.

I did in the other thread "Re: Amiga RDB partition support for disks >= 
2 TB". Just for the record in an reply to your patch, Michael, I copy it 
over:

-------------------------------------------------------------------------
But yeah, as mentioned I researched the topic before. And I think there 
has not even been an overflow within the RDB:

> The raw, theoretical limit on the maximum device capacity is about
> 2^105 bytes:
>
> 32 bit rdb_Cylinders * 32 bit rdb_Heads * 32 bit rdb_Sectors * 512
> bytes/sector for the HD size in struct RigidDiskBlock

http://wiki.amigaos.net/wiki/RDB_(Amiga_Rigid_Disk_Block)

Confirmed by:

The .ADF (Amiga Disk File) format FAQ:
http://lclevy.free.fr/adflib/adf_info.html#p6

But what do I write, you know the RDB format 

So just do the calculation in 96 Bit and you all are set 

Now that is a reason for 128 Bit CPUs :).
-------------------------------------------------------------------------

> Here are the relevant pieces of the ADF format FAQ:
>  6.1 What is the Rigid Disk Block ?
> 
> * Rigid Disk block (256 bytes) must exist within the first 16 blocks
> […]
> 
> 	Physical drive caracteristics
> 
> 64/40	ulong 	1 	cylinders 	number of drive cylinder
> 68/44 	ulong 	1 	sectors 	sectors per track
> 72/48	ulong 	1 	heads 		number of drive heads
> […]

ULONG is 32-bit unsigned. See:

http://wiki.amigaos.net/wiki/Fundamental_Types

So we basically have 96 bits here + at least 9 bits (512 bytes sector 
size) if you calculate by bytes. And as noted by Joanne: In case you 
would use large sector sizes… you could address really even more 
*insane* amounts of capacity.

See why I still think RDB´s is quite brilliant?

Maybe using ULONG was just by accident, but well it made RDB´s future 
proof for accessing larger disks than anything that is in the market 
today and I bet for a long time to come :)

And for partitions this works as well:

>  6.3 How are partitions stored?
> 
> 164/a4 	ulong 	1 	LowCyl		first cylinder of a partition 
> (inclusive)
> 168/a8 	ulong 	1 	HighCyl		last cylinder of a partition 
> (inclusive)

So to summarize: I think RDB´s can compete with GPT regarding maximum 
capacity nicely enough. Despite Joanne invented RDB at least a decade 
before someone invented GPT. AmigaOS had and has a ton of flaws but it 
has been made by genius´es and it is a lot of brilliant ideas in it.

Okay, I think after this essay it should really be clear that the RDB 
itself can handle insane amounts of capacity in any case.

> This means that any errors and failures to mount should lie with the
> RDB Parser, the filesystem, the device driver, and any intervening
> code between the three. The RDBs can express a value far greater than
> the number of atoms estimated to exist in the observable universe. So
> RDBs won't ever need a change for disk size considerations. The
> parser and filesystems probably and possibly, respectively, will.

Exactly that.

Filesystems have other limits. And they need to bail out before mounting 
anything beyond their limits.

So the RDB parser is just fine with at least 64 bit calculations. Of 
course it should bail out big time if it ever comes across a RDB where 

32 bit rdb_Cylinders * 32 bit rdb_Heads * 32 bit rdb_Sectors

does not fit within 64 Bit. Unless it uses 96 or even 128 bit 
calculations :). Even tough I think it is unlikely that there ever would 
be such an RDB, I would rather play it very safe with anything regarding 
partitioning and addressing capacity on disks. Never ever overflow 
without bailing out. Or to have it with Joanne´s words:

> Error out and don't mount disks or partitions that are outside the
> size range your element of the datapath from disk to user's program
> can handle at the first place this can be detected. It's the polite
> thing to do to a user's data rather than the Maxine Waters thing to
> do.

Fully agreed.

Amen,

Michael Schmitz June 29, 2018, 11:24 p.m. UTC | #35

Martin,


Am 30.06.18 um 09:24 schrieb Martin Steigerwald:
> Hi Michael.
>
> Michael Schmitz - 29.06.18, 11:07:
>>> But it's up to the person (which is not Linux) formatting the disk
>>> to
>>> not try to use
>>> it on systems that cannot handle it, and may destroy it.
>>>
>>>>> Let me clarify: what exactly would the kernel option allow? When
>>>>> to use it?>> 
>>>> Whether to use it if safe (on Linux). But whatever Linux does
>>>> (after
>>>> this patch), access will go to the right area of the disk (as
>>>> specified by the RDB) so Linux won't any longer stomp on anything
>>>> that would have mattered to 32 bit disk drivers. So it really
>>>> should be safe.> 
>>> Personally, I see no reason to depend on a kernel option, if it is
>>> safe to use. Just use it.
>> So to recap - someone partitions a disk on AmigaOS 4.x, taking
>> advantage of the large block device support there.
>> Using that disk on AmigaOS 3.1, data loss ensues. Whether or not Linux
>> (patched) ever touched the disk has no impact on that outcome.
> I am not even completely sure about that. Frankly I have no idea what 
> would happen when using such a disk on AmigaOS 3.1 *without* NSDPatch or 
> TD64 support (I think you could patch AmigaOS 3.1 with 64 Bit support 
> already and some 3rd party harddisk controllers by Phase 5 hat TD64 
> support at that time already). Unless I try it, which I won´t at the 
> moment, I´d say the behaviour is largely undefined.
The problem that still remains is with unpatched legacy versions. RDB
does support large enough partitions out of the box, due to C/H/S all
using u32. We all agree there. The question is with file systems and
device drivers - someone, at some point, has to recast the block offset
at filesystem level to an offset expressed in the devices's native block
size (regardless of block size used by RDB and filesystem). There are
MOs that use 2k block sizes natively, but as far as I am aware, disks
are usually 512 byte block size (the last disk system I remember where
you could chose block size at the hardware level, at format time, was an
Emulex disk controller on  MicroVax II).

I've dug through a few specs to get a better handle on this. SCSI-2 used
a maximum of 4 bytes for block addresses. IDE used 6 bytes, so at the
time these systems were designed, there already was a way of requesting
data at offsets beyond what can be expressed in 32 bits. I would hope
that any system supporting IDE would have done offset calculations in 64
bits. Is that too optimistic?
> But hey, undefined means it may just overwrite start overwriting from 
> the beginning of the disk beyond 32 bit. And I think that is quite 
> likely. It could also crash, but if its an overflow I don´t think why it 
> would crash. Anyway, I never tried this out.
If offsets are calculated in 32 bits, excess bits are truncated (I don't
think integer overflows ever trapped?). That wraps the offset to
somewhere within the first 2 TB of the disk (maybe less if signed int
was used, like in our parser).
> But in any way: This would happen or not happen no matter whether Linux 
> parsed the RDB or not.
>
> I still think that the native OS warning really does not hurt… but I´d 
> spare myself the kernel option. Having the warning without the kernel 
> option would be a compromise between being cautious and being bold :).
Yep, I've come around to that conclusion, too. Just need to test it
properly. And I still need to check what will happen if we allow such a
large partition table to parse on a kernel without LBD support (i.e.,
are there any downstream checks in place to refuse partitions larger
than what can be handled by 32 bit).

Cheers,

    Michael


> Thanks,

jdow June 29, 2018, 11:24 p.m. UTC | #36

On 20180629 14:45, Martin Steigerwald wrote:
 > Beware: Essay ahead which proofs it to the point that there is no
 > overflow in RDB before 96 bits maximum value of sectors:

Time to go into more detail on RDBs. It isn't as simple as it started to appear.

extract from hardblocks.h RDSK block definition
===8<---
     ULONG   rdb_BlockBytes;	/* size of disk blocks */
...
     ULONG   rdb_Cylinders;	/* number of drive cylinders */
     ULONG   rdb_Sectors;	/* sectors per track */
     ULONG   rdb_Heads;		/* number of drive heads */
     ...
     ULONG   rdb_LoCylinder;	/* low cylinder of partitionable disk area */
     ULONG   rdb_HiCylinder;	/* high cylinder of partitionable data area */
     ULONG   rdb_CylBlocks;	/* number of blocks available per cylinder */
===8<---
This has the hard limit embodied within it, unfortunately.

The first four values above give you hope for 2^128 bytes. The next three may 
trash some of it when all three are considered.

Since a cylinder is sectors times heads we have 2^64 blocks capacity embodied in 
rdb_LoCylinder and rdb_HiCylinder. But, our hopes are deftly dashed by the last 
value rdb_CylBlocks which places a hard limit on the product of rdb_Heads and 
rdb_Sectors of 2^32. But, that still allows is a fairly large disk. 2^32-1 
blocks per cylinder times block size, rdb_BlockBytes, of 2^32, although the 
larger block sizes are um er sort of putrid to use. Similar limitations exist 
within dos.h in the InfoData and DosEnvec structure, among other likely places.

Approaches "exist" to allowing large partitions. Some of them are unattractive, 
probably all of them as a matter of fact.
1) For large disks move to GPT and be done with it.
2) "lie" and teach the filesystems to ignore rdb_CylBlocks and similar values 
elsewhere. Then the sky is the limit.
3) Invent a "PA64" 64 bit RDB entry and the other internal structures to make it 
work, InfoData64, DosEnvec64, and so on.

Solution 2 might be the least disruptive way to do it. BUT, a whole host of 
utilities like "info" will have to be tweaked to handle "rdb_CylBlocks" becoming 
meaningless.

So this is what happened with some simple includes mining while I am playing 
hooky from doing some real work.

Good luck, gentlemen.
{^_^}

Michael Schmitz June 30, 2018, 12:44 a.m. UTC | #37

Joanne,


Am 30.06.18 um 11:24 schrieb jdow:
>
> On 20180629 14:45, Martin Steigerwald wrote:
> > Beware: Essay ahead which proofs it to the point that there is no
> > overflow in RDB before 96 bits maximum value of sectors:
>
> Time to go into more detail on RDBs. It isn't as simple as it started
> to appear.
>
> extract from hardblocks.h RDSK block definition
> ===8<---
>     ULONG   rdb_BlockBytes;    /* size of disk blocks */
> ...
>     ULONG   rdb_Cylinders;    /* number of drive cylinders */
>     ULONG   rdb_Sectors;    /* sectors per track */
>     ULONG   rdb_Heads;        /* number of drive heads */
>     ...
>     ULONG   rdb_LoCylinder;    /* low cylinder of partitionable disk
> area */
>     ULONG   rdb_HiCylinder;    /* high cylinder of partitionable data
> area */
>     ULONG   rdb_CylBlocks;    /* number of blocks available per
> cylinder */
> ===8<---
> This has the hard limit embodied within it, unfortunately.
>
> The first four values above give you hope for 2^128 bytes. The next
> three may trash some of it when all three are considered.
>
> Since a cylinder is sectors times heads we have 2^64 blocks capacity
> embodied in rdb_LoCylinder and rdb_HiCylinder. But, our hopes are
> deftly dashed by the last value rdb_CylBlocks which places a hard
> limit on the product of rdb_Heads and rdb_Sectors of 2^32. But, that
> still allows is a fairly large disk. 2^32-1 blocks per cylinder times
> block size, rdb_BlockBytes, of 2^32, although the larger block sizes
> are um er sort of putrid to use. Similar limitations exist within
> dos.h in the InfoData and DosEnvec structure, among other likely places.
>

As far as Linux is concerned, rdb_CylBlocks is used nowhere, neither in
the RDB parser nor in the AFFS filesystem driver. Only the partition
list is parsed.

Should we use rdb_LoCylinder*rdbCylBlocks and
rdb_HiCylinder*rdbCylBlocks in the RDB parser to verify the detected
partitions are valid according to the RDB's own specified limits? Or can
we absolutely rely on the partitioning tool getting that right?

Any similar surprises in the partition list data structures? The header
I have in Linux is largely non-descriptive there:

struct PartitionBlock {
        __be32  pb_ID;
        __be32  pb_SummedLongs;
        __s32   pb_ChkSum;
        __u32   pb_HostID;
        __be32  pb_Next;
        __u32   pb_Flags;
        __u32   pb_Reserved1[2];
        __u32   pb_DevFlags;
        __u8    pb_DriveName[32];
        __u32   pb_Reserved2[15];
        __be32  pb_Environment[17];
        __u32   pb_EReserved[15];
};

As far as I can guess from the code, pb_Environment[3] (number of heads)
and pb_Environment[5] (number of sectors per cylinder) are abitrarily
chosen so the partition size can be expressed as a difference between
pb_Environment[9] and pb_Environment[10] (low and high cylinder
addresses), which places restrictions on both partition size and
alignment that depend on where on the disk a partition is placed?

Cheers,

    Michael

> Approaches "exist" to allowing large partitions. Some of them are
> unattractive, probably all of them as a matter of fact.
> 1) For large disks move to GPT and be done with it.
> 2) "lie" and teach the filesystems to ignore rdb_CylBlocks and similar
> values elsewhere. Then the sky is the limit.
> 3) Invent a "PA64" 64 bit RDB entry and the other internal structures
> to make it work, InfoData64, DosEnvec64, and so on.
>
> Solution 2 might be the least disruptive way to do it. BUT, a whole
> host of utilities like "info" will have to be tweaked to handle
> "rdb_CylBlocks" becoming meaningless.
>
> So this is what happened with some simple includes mining while I am
> playing hooky from doing some real work.
>
> Good luck, gentlemen.
> {^_^}

jdow June 30, 2018, 12:49 a.m. UTC | #38

On 20180629 16:24, Michael Schmitz wrote:
 > Martin,
 >
 >
...
 > The problem that still remains is with unpatched legacy versions. RDB
 > does support large enough partitions out of the box, due to C/H/S all
 > using u32. We all agree there. The question is with file systems and

Nope, I bothered to read the source code includes files. As long as anything in 
the OS uses the blocks per cylinder variables in various structures you're SOL. 
rdb_CylinderBlocks is just for starters. The GlobalVec entry also includes a 
CylinderBlocks entry. And it's only uint32_t size.

As I say, good luck gentlemen. Changes, big changes, probably need to be made to 
go beyond 2TB except by using larger block sizes. But, 64k block sizes are sort 
of "putrid" to say the least.

{^_^}

jdow June 30, 2018, 12:57 a.m. UTC | #39

On 20180629 17:44, Michael Schmitz wrote:

 > struct PartitionBlock {
 >          __be32  pb_ID;
 >          __be32  pb_SummedLongs;
 >          __s32   pb_ChkSum;
 >          __u32   pb_HostID;
 >          __be32  pb_Next;
 >          __u32   pb_Flags;
 >          __u32   pb_Reserved1[2];
 >          __u32   pb_DevFlags;
 >          __u8    pb_DriveName[32];
 >          __u32   pb_Reserved2[15];
 >          __be32  pb_Environment[17];
 >          __u32   pb_EReserved[15];
 > };
  pb_Environment = a struct DosEnvec and it is 20 ULONGs in size. I believe you 
are looking at some old include files. These got added to the end of the 
DosEnvec structure:
     ULONG de_Baud;	     /* Baud rate for serial handler */
     ULONG de_Control;	     /* Control word for handler/filesystem */
     ULONG de_BootBlocks;     /* Number of blocks containing boot code */

 > As far as I can guess from the code, pb_Environment[3] (number of heads)
 > and pb_Environment[5] (number of sectors per cylinder) are abitrarily
 > chosen so the partition size can be expressed as a difference between
 > pb_Environment[9] and pb_Environment[10] (low and high cylinder
 > addresses), which places restrictions on both partition size and
 > alignment that depend on where on the disk a partition is placed?
If you do not teach the OS to ignore Cylinder Blocks type entries and use some 
math on heads and blocks per track the disk size is relatively stuck modulo 
using large blocks.

{^_^}

On 20180629 17:44, Michael Schmitz wrote:
> Joanne,
> 
> 
> Am 30.06.18 um 11:24 schrieb jdow:
>>
>> On 20180629 14:45, Martin Steigerwald wrote:
>>> Beware: Essay ahead which proofs it to the point that there is no
>>> overflow in RDB before 96 bits maximum value of sectors:
>>
>> Time to go into more detail on RDBs. It isn't as simple as it started
>> to appear.
>>
>> extract from hardblocks.h RDSK block definition
>> ===8<---
>>      ULONG   rdb_BlockBytes;    /* size of disk blocks */
>> ...
>>      ULONG   rdb_Cylinders;    /* number of drive cylinders */
>>      ULONG   rdb_Sectors;    /* sectors per track */
>>      ULONG   rdb_Heads;        /* number of drive heads */
>>      ...
>>      ULONG   rdb_LoCylinder;    /* low cylinder of partitionable disk
>> area */
>>      ULONG   rdb_HiCylinder;    /* high cylinder of partitionable data
>> area */
>>      ULONG   rdb_CylBlocks;    /* number of blocks available per
>> cylinder */
>> ===8<---
>> This has the hard limit embodied within it, unfortunately.
>>
>> The first four values above give you hope for 2^128 bytes. The next
>> three may trash some of it when all three are considered.
>>
>> Since a cylinder is sectors times heads we have 2^64 blocks capacity
>> embodied in rdb_LoCylinder and rdb_HiCylinder. But, our hopes are
>> deftly dashed by the last value rdb_CylBlocks which places a hard
>> limit on the product of rdb_Heads and rdb_Sectors of 2^32. But, that
>> still allows is a fairly large disk. 2^32-1 blocks per cylinder times
>> block size, rdb_BlockBytes, of 2^32, although the larger block sizes
>> are um er sort of putrid to use. Similar limitations exist within
>> dos.h in the InfoData and DosEnvec structure, among other likely places.
>>
> 
> As far as Linux is concerned, rdb_CylBlocks is used nowhere, neither in
> the RDB parser nor in the AFFS filesystem driver. Only the partition
> list is parsed.
> 
> Should we use rdb_LoCylinder*rdbCylBlocks and
> rdb_HiCylinder*rdbCylBlocks in the RDB parser to verify the detected
> partitions are valid according to the RDB's own specified limits? Or can
> we absolutely rely on the partitioning tool getting that right?
> 
> Any similar surprises in the partition list data structures? The header
> I have in Linux is largely non-descriptive there:
> 
> struct PartitionBlock {
>          __be32  pb_ID;
>          __be32  pb_SummedLongs;
>          __s32   pb_ChkSum;
>          __u32   pb_HostID;
>          __be32  pb_Next;
>          __u32   pb_Flags;
>          __u32   pb_Reserved1[2];
>          __u32   pb_DevFlags;
>          __u8    pb_DriveName[32];
>          __u32   pb_Reserved2[15];
>          __be32  pb_Environment[17];
>          __u32   pb_EReserved[15];
> };
> 
> As far as I can guess from the code, pb_Environment[3] (number of heads)
> and pb_Environment[5] (number of sectors per cylinder) are abitrarily
> chosen so the partition size can be expressed as a difference between
> pb_Environment[9] and pb_Environment[10] (low and high cylinder
> addresses), which places restrictions on both partition size and
> alignment that depend on where on the disk a partition is placed?
> 
> Cheers,
> 
>      Michael
> 
>> Approaches "exist" to allowing large partitions. Some of them are
>> unattractive, probably all of them as a matter of fact.
>> 1) For large disks move to GPT and be done with it.
>> 2) "lie" and teach the filesystems to ignore rdb_CylBlocks and similar
>> values elsewhere. Then the sky is the limit.
>> 3) Invent a "PA64" 64 bit RDB entry and the other internal structures
>> to make it work, InfoData64, DosEnvec64, and so on.
>>
>> Solution 2 might be the least disruptive way to do it. BUT, a whole
>> host of utilities like "info" will have to be tweaked to handle
>> "rdb_CylBlocks" becoming meaningless.
>>
>> So this is what happened with some simple includes mining while I am
>> playing hooky from doing some real work.
>>
>> Good luck, gentlemen.
>> {^_^}
>

Michael Schmitz June 30, 2018, 1:31 a.m. UTC | #40

Joanne,


Am 30.06.18 um 12:57 schrieb jdow:
> On 20180629 17:44, Michael Schmitz wrote:
>
> > struct PartitionBlock {
> >          __be32  pb_ID;
> >          __be32  pb_SummedLongs;
> >          __s32   pb_ChkSum;
> >          __u32   pb_HostID;
> >          __be32  pb_Next;
> >          __u32   pb_Flags;
> >          __u32   pb_Reserved1[2];
> >          __u32   pb_DevFlags;
> >          __u8    pb_DriveName[32];
> >          __u32   pb_Reserved2[15];
> >          __be32  pb_Environment[17];
> >          __u32   pb_EReserved[15];
> > };
>  pb_Environment = a struct DosEnvec and it is 20 ULONGs in size. I
> believe you are looking at some old include files.

Without looking at ancient git history, I'd say between 1993 and 1996.

> These got added to the end of the DosEnvec structure:
>     ULONG de_Baud;         /* Baud rate for serial handler */
>     ULONG de_Control;         /* Control word for handler/filesystem */
>     ULONG de_BootBlocks;     /* Number of blocks containing boot code */
>
> > As far as I can guess from the code, pb_Environment[3] (number of
> heads)
> > and pb_Environment[5] (number of sectors per cylinder) are abitrarily
> > chosen so the partition size can be expressed as a difference between
> > pb_Environment[9] and pb_Environment[10] (low and high cylinder
> > addresses), which places restrictions on both partition size and
> > alignment that depend on where on the disk a partition is placed?
> If you do not teach the OS to ignore Cylinder Blocks type entries and
> use some math on heads and blocks per track the disk size is
> relatively stuck modulo using large blocks.

As long as AmigaOS and Linux agree on how to express start and end
offset for the partitions, that's fine.

But I read your other mail to mean that we're stuck to 2 TB disks for
now. I don't follow that - we can have partitions of 2 TB each by maxing
out rdb_CylBlocks as long as we use 512 bytes per block (since the
product of cylinders and blocks per cylinder is limited to 32 bits) and
using one cylinder per partition (32 bits available there as well)?

But the rdb_CylBlocks limit also means we're safe with 64 bit sector_t
in Linux. Best add a check in the parser to warn us if the product of
head count and sectors per cylinder overflows 32 bit though.

Cheers,

    Michael
>
> {^_^}
>
> On 20180629 17:44, Michael Schmitz wrote:
>> Joanne,
>>
>>
>> Am 30.06.18 um 11:24 schrieb jdow:
>>>
>>> On 20180629 14:45, Martin Steigerwald wrote:
>>>> Beware: Essay ahead which proofs it to the point that there is no
>>>> overflow in RDB before 96 bits maximum value of sectors:
>>>
>>> Time to go into more detail on RDBs. It isn't as simple as it started
>>> to appear.
>>>
>>> extract from hardblocks.h RDSK block definition
>>> ===8<---
>>>      ULONG   rdb_BlockBytes;    /* size of disk blocks */
>>> ...
>>>      ULONG   rdb_Cylinders;    /* number of drive cylinders */
>>>      ULONG   rdb_Sectors;    /* sectors per track */
>>>      ULONG   rdb_Heads;        /* number of drive heads */
>>>      ...
>>>      ULONG   rdb_LoCylinder;    /* low cylinder of partitionable disk
>>> area */
>>>      ULONG   rdb_HiCylinder;    /* high cylinder of partitionable data
>>> area */
>>>      ULONG   rdb_CylBlocks;    /* number of blocks available per
>>> cylinder */
>>> ===8<---
>>> This has the hard limit embodied within it, unfortunately.
>>>
>>> The first four values above give you hope for 2^128 bytes. The next
>>> three may trash some of it when all three are considered.
>>>
>>> Since a cylinder is sectors times heads we have 2^64 blocks capacity
>>> embodied in rdb_LoCylinder and rdb_HiCylinder. But, our hopes are
>>> deftly dashed by the last value rdb_CylBlocks which places a hard
>>> limit on the product of rdb_Heads and rdb_Sectors of 2^32. But, that
>>> still allows is a fairly large disk. 2^32-1 blocks per cylinder times
>>> block size, rdb_BlockBytes, of 2^32, although the larger block sizes
>>> are um er sort of putrid to use. Similar limitations exist within
>>> dos.h in the InfoData and DosEnvec structure, among other likely
>>> places.
>>>
>>
>> As far as Linux is concerned, rdb_CylBlocks is used nowhere, neither in
>> the RDB parser nor in the AFFS filesystem driver. Only the partition
>> list is parsed.
>>
>> Should we use rdb_LoCylinder*rdbCylBlocks and
>> rdb_HiCylinder*rdbCylBlocks in the RDB parser to verify the detected
>> partitions are valid according to the RDB's own specified limits? Or can
>> we absolutely rely on the partitioning tool getting that right?
>>
>> Any similar surprises in the partition list data structures? The header
>> I have in Linux is largely non-descriptive there:
>>
>> struct PartitionBlock {
>>          __be32  pb_ID;
>>          __be32  pb_SummedLongs;
>>          __s32   pb_ChkSum;
>>          __u32   pb_HostID;
>>          __be32  pb_Next;
>>          __u32   pb_Flags;
>>          __u32   pb_Reserved1[2];
>>          __u32   pb_DevFlags;
>>          __u8    pb_DriveName[32];
>>          __u32   pb_Reserved2[15];
>>          __be32  pb_Environment[17];
>>          __u32   pb_EReserved[15];
>> };
>>
>> As far as I can guess from the code, pb_Environment[3] (number of heads)
>> and pb_Environment[5] (number of sectors per cylinder) are abitrarily
>> chosen so the partition size can be expressed as a difference between
>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
>> addresses), which places restrictions on both partition size and
>> alignment that depend on where on the disk a partition is placed?
>>
>> Cheers,
>>
>>      Michael
>>
>>> Approaches "exist" to allowing large partitions. Some of them are
>>> unattractive, probably all of them as a matter of fact.
>>> 1) For large disks move to GPT and be done with it.
>>> 2) "lie" and teach the filesystems to ignore rdb_CylBlocks and similar
>>> values elsewhere. Then the sky is the limit.
>>> 3) Invent a "PA64" 64 bit RDB entry and the other internal structures
>>> to make it work, InfoData64, DosEnvec64, and so on.
>>>
>>> Solution 2 might be the least disruptive way to do it. BUT, a whole
>>> host of utilities like "info" will have to be tweaked to handle
>>> "rdb_CylBlocks" becoming meaningless.
>>>
>>> So this is what happened with some simple includes mining while I am
>>> playing hooky from doing some real work.
>>>
>>> Good luck, gentlemen.
>>> {^_^}
>>

jdow June 30, 2018, 3:56 a.m. UTC | #41

On 20180629 18:31, Michael Schmitz wrote:> Joanne,
 >
 >
 > Am 30.06.18 um 12:57 schrieb jdow:
 >> On 20180629 17:44, Michael Schmitz wrote:
 >>
 >>> struct PartitionBlock {
 >>>            __be32  pb_ID;
 >>>            __be32  pb_SummedLongs;
 >>>            __s32   pb_ChkSum;
 >>>            __u32   pb_HostID;
 >>>            __be32  pb_Next;
 >>>            __u32   pb_Flags;
 >>>            __u32   pb_Reserved1[2];
 >>>            __u32   pb_DevFlags;
 >>>            __u8    pb_DriveName[32];
 >>>            __u32   pb_Reserved2[15];
 >>>            __be32  pb_Environment[17];
 >>>            __u32   pb_EReserved[15];
 >>> };
 >>   pb_Environment = a struct DosEnvec and it is 20 ULONGs in size. I
 >> believe you are looking at some old include files.
 >
 > Without looking at ancient git history, I'd say between 1993 and 1996.
 >
 >> These got added to the end of the DosEnvec structure:
 >>      ULONG de_Baud;         /* Baud rate for serial handler */
 >>      ULONG de_Control;         /* Control word for handler/filesystem */
 >>      ULONG de_BootBlocks;     /* Number of blocks containing boot code */
 >>
 >>> As far as I can guess from the code, pb_Environment[3] (number of
 >> heads)
 >>> and pb_Environment[5] (number of sectors per cylinder) are abitrarily
 >>> chosen so the partition size can be expressed as a difference between
 >>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
 >>> addresses), which places restrictions on both partition size and
 >>> alignment that depend on where on the disk a partition is placed?
 >> If you do not teach the OS to ignore Cylinder Blocks type entries and
 >> use some math on heads and blocks per track the disk size is
 >> relatively stuck modulo using large blocks.
 >
 > As long as AmigaOS and Linux agree on how to express start and end
 > offset for the partitions, that's fine.
 >
 > But I read your other mail to mean that we're stuck to 2 TB disks for
 > now. I don't follow that - we can have partitions of 2 TB each by maxing
 > out rdb_CylBlocks as long as we use 512 bytes per block (since the
 > product of cylinders and blocks per cylinder is limited to 32 bits) and
 > using one cylinder per partition (32 bits available there as well)?
 >
 > But the rdb_CylBlocks limit also means we're safe with 64 bit sector_t
 > in Linux. Best add a check in the parser to warn us if the product of
 > head count and sectors per cylinder overflows 32 bit though.
 >
 > Cheers,
 >
 >      Michael
How long did it tale s to get to 10 TB disks from 2 TB disks. And a new SD Card 
spec allows for 128 TB disks. Block sizes get sort of ridiculous as you get past 
about 8k bytes or about 32 TB or about two years from now.

Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as far as I 
know, makes heavy use of Cylinder Blocks values. It calculating Cylinder Blocks 
overflows when creating the disk's RDBs the user MUST be informed it is unsafe 
to put on a real Amiga. (I'd also suggest teaching Linux to understand RDSL, 
which would be RDSK++ sort of. Then use that if Cylinder Blocks overflows.) The 
value you will not be able to fill in the DosEnvec structure is:
	ULONG de_HighCyl;	     /* max cylinder. drive specific */

So accessing larger disks once you hit 2 TB means you must increase the logical 
block size. And eventually that will waste HUGE amounts of files when small 
files are being stored.

Any solution will require action on the part of the people developing AmigaDOS 
follow-ons. You might want to get them motivated, somehow, and proceed from 
there with a request to be informed of any RDB changes. I'd suggest to them that 
removing sensitivity to Cylinder Blocks sorts of values from the entire system 
probably would be painful but the simplest solution.

{^_^}

Michael Schmitz June 30, 2018, 5:26 a.m. UTC | #42

Joanne,


Am 30.06.18 um 15:56 schrieb jdow:
>
> >>> As far as I can guess from the code, pb_Environment[3] (number of
> >> heads)
> >>> and pb_Environment[5] (number of sectors per cylinder) are abitrarily
> >>> chosen so the partition size can be expressed as a difference between
> >>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
> >>> addresses), which places restrictions on both partition size and
> >>> alignment that depend on where on the disk a partition is placed?
> >> If you do not teach the OS to ignore Cylinder Blocks type entries and
> >> use some math on heads and blocks per track the disk size is
> >> relatively stuck modulo using large blocks.
> >
> > As long as AmigaOS and Linux agree on how to express start and end
> > offset for the partitions, that's fine.
> >
> > But I read your other mail to mean that we're stuck to 2 TB disks for
> > now. I don't follow that - we can have partitions of 2 TB each by
> maxing
> > out rdb_CylBlocks as long as we use 512 bytes per block (since the
> > product of cylinders and blocks per cylinder is limited to 32 bits) and
> > using one cylinder per partition (32 bits available there as well)?
> >
> > But the rdb_CylBlocks limit also means we're safe with 64 bit sector_t
> > in Linux. Best add a check in the parser to warn us if the product of
> > head count and sectors per cylinder overflows 32 bit though.
> >
> > Cheers,
> >
> >      Michael
> How long did it tale s to get to 10 TB disks from 2 TB disks. And a
> new SD Card spec allows for 128 TB disks. Block sizes get sort of
> ridiculous as you get past about 8k bytes or about 32 TB or about two
> years from now.

I get that - I just don't get why 32 bits for cylinders plus 32 bits for
blocks per cylinder equals 2 TB (4G of 512 byte blocks). But I don't
know what other limits exist that may restrict the total number of
blocks to 32 bits.

>
> Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
> far as I know, makes heavy use of Cylinder Blocks values. It
> calculating Cylinder Blocks overflows when creating the disk's RDBs
> the user MUST be informed it is 

I'm not at all planning to create disks for AmigaDOS. I just need to
know what combinations of cylinders, heads and sectors are possible to
encounter on disks that have been created with native tools. Well,
assuming sufficient amounts of braindamage in the corresponding Linux
tools, knowing the absolute outer limits of what these tools could do
would be nice as well, but someone using amiga-fdisk to create a RDSK
block for a 10 TB disk fully deserves any punishment that invites.

(Actually, I lied there. I do plan to create a RDSK block for a 2 TB
disk image where cylinder, head and sector counts all approach the 32
bit limit, just to see that my overflow checks work as intended. But
that's strictly for Linux testing).

> unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
> understand RDSL, which would be RDSK++ sort of. Then use that if
> Cylinder Blocks overflows.) The value you will not be able to fill in
> the DosEnvec structure is:
>     ULONG de_HighCyl;         /* max cylinder. drive specific */

OK, so Cylinder Blocks overflowing is a red flag, and requires to abort
parsing the partition table right away? And HighCyl really means the
max. number of logical blocks, not cylinders (which would have
nr_heads*nr_sects many blocks)? That's probably the cause for my confusion.

>
> So accessing larger disks once you hit 2 TB means you must increase
> the logical block size. And eventually that will waste HUGE amounts of
> files when small files are being stored.
Just like small inodes wastes huge amounts of space for metadata. It's a
tradeoff, and AFFS on a RDSK format disk probably isn't the right choice
for huge disks. Never mind that - if someone _does_ go that way, we need
to make sure we can parse the RDSK information correctly. And if such a
disk causes the 64 bit sector_t in Linux to overflow, I'd like the
parser to spot that, too.

Thanks for your immense patience in explaining all these subtleties to me.

Cheers,

    Michael

>
> Any solution will require action on the part of the people developing
> AmigaDOS follow-ons. You might want to get them motivated, somehow,
> and proceed from there with a request to be informed of any RDB
> changes. I'd suggest to them that removing sensitivity to Cylinder
> Blocks sorts of values from the entire system probably would be
> painful but the simplest solution.
>
> {^_^}
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

jdow June 30, 2018, 6:47 a.m. UTC | #43

Let's get everybody:

On 20180629 22:26, Michael Schmitz wrote:
 > Joanne,
 >
 >
 > Am 30.06.18 um 15:56 schrieb jdow:
 >>
 >>>>> As far as I can guess from the code, pb_Environment[3] (number of
 >>>> heads)
 >>>>> and pb_Environment[5] (number of sectors per cylinder) are abitrarily
 >>>>> chosen so the partition size can be expressed as a difference between
 >>>>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
 >>>>> addresses), which places restrictions on both partition size and
 >>>>> alignment that depend on where on the disk a partition is placed?
 >>>> If you do not teach the OS to ignore Cylinder Blocks type entries and
 >>>> use some math on heads and blocks per track the disk size is
 >>>> relatively stuck modulo using large blocks.
 >>>
 >>> As long as AmigaOS and Linux agree on how to express start and end
 >>> offset for the partitions, that's fine.
 >>>
 >>> But I read your other mail to mean that we're stuck to 2 TB disks for
 >>> now. I don't follow that - we can have partitions of 2 TB each by
 >> maxing
 >>> out rdb_CylBlocks as long as we use 512 bytes per block (since the
 >>> product of cylinders and blocks per cylinder is limited to 32 bits) and
 >>> using one cylinder per partition (32 bits available there as well)?
 >>>
 >>> But the rdb_CylBlocks limit also means we're safe with 64 bit sector_t
 >>> in Linux. Best add a check in the parser to warn us if the product of
 >>> head count and sectors per cylinder overflows 32 bit though.
 >>>
 >>> Cheers,
 >>>
 >>>        Michael
 >> How long did it tale s to get to 10 TB disks from 2 TB disks. And a
 >> new SD Card spec allows for 128 TB disks. Block sizes get sort of
 >> ridiculous as you get past about 8k bytes or about 32 TB or about two
 >> years from now.
 >
 > I get that - I just don't get why 32 bits for cylinders plus 32 bits for
 > blocks per cylinder equals 2 TB (4G of 512 byte blocks). But I don't
 > know what other limits exist that may restrict the total number of
 > blocks to 32 bits.

It overflows uint32_t cylinder blocks aka blocks per cylinder. Linux doesn't 
care. AmigaDOS surely does. If YOU make partitions really large for yourself 
that's OK. If Joe Amigoid does it the potential for an angry red turning to 
purple face is high.

 >> Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
 >> far as I know, makes heavy use of Cylinder Blocks values. It
 >> calculating Cylinder Blocks overflows when creating the disk's RDBs
 >> the user MUST be informed it is
 >
 > I'm not at all planning to create disks for AmigaDOS. I just need to
 > know what combinations of cylinders, heads and sectors are possible to
 > encounter on disks that have been created with native tools. Well,
 > assuming sufficient amounts of braindamage in the corresponding Linux
 > tools, knowing the absolute outer limits of what these tools could do
 > would be nice as well, but someone using amiga-fdisk to create a RDSK
 > block for a 10 TB disk fully deserves any punishment that invites.

Native AmigaDOS tools SHOULD NOT be able to create something that overflows 
CylinderBlocks values. However, if it can that creates an interesting test case 
to see what various tools, like the AmigaDOS "info" command, do when they they 
are run on such a disk. I don't have OS source to perform searches. And I am not 
setup to feed the system something obscene.

 > (Actually, I lied there. I do plan to create a RDSK block for a 2 TB
 > disk image where cylinder, head and sector counts all approach the 32
 > bit limit, just to see that my overflow checks work as intended. But
 > that's strictly for Linux testing).
 >
 >> unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
 >> understand RDSL, which would be RDSK++ sort of. Then use that if
 >> Cylinder Blocks overflows.) The value you will not be able to fill in
 >> the DosEnvec structure is:
 >>      ULONG de_HighCyl;         /* max cylinder. drive specific */
 >
 > OK, so Cylinder Blocks overflowing is a red flag, and requires to abort
 > parsing the partition table right away? And HighCyl really means the
 > max. number of logical blocks, not cylinders (which would have
 > nr_heads*nr_sects many blocks)? That's probably the cause for my confusion.

I think I picked the wrong value. In RDSK itself this value is what overflows:
     ULONG   rdb_CylBlocks;    /* number of blocks available per cylinder */
And I think that floats around the system in many places with different names. 
As mentioned the "info" command is one item to test. If no crashes are found 
then AmigaDOS may be clean up to obscene sizes. At the moment I do not remember 
what hdwrench.library does with that value other than pass it along as read. Nor 
am I sure what it generates as any suggested values. I don't at this time have a 
disk I can mount as a disk on WinUAE that is more than 2TB. And my Amigas speak 
SCSI so I have no disk for them, either, even if they still boot.

 >
 >>
 >> So accessing larger disks once you hit 2 TB means you must increase
 >> the logical block size. And eventually that will waste HUGE amounts of
 >> files when small files are being stored.
 > Just like small inodes wastes huge amounts of space for metadata. It's a
 > tradeoff, and AFFS on a RDSK format disk probably isn't the right choice
 > for huge disks. Never mind that - if someone _does_ go that way, we need
 > to make sure we can parse the RDSK information correctly. And if such a
 > disk causes the 64 bit sector_t in Linux to overflow, I'd like the
 > parser to spot that, too.
 >
 > Thanks for your immense patience in explaining all these subtleties to me.
 >
 > Cheers,
 >
 >      Michael

And I'm rushing too much so I'm sorry I am making errors. This stuff is 25 years 
in the past since I last looked at it seriously.

{^_^}

Martin Steigerwald June 30, 2018, 7:49 a.m. UTC | #44

Whoa, my summary essay triggered digging even more accurately into that 
matter. For some obscure reason I am even enjoying this. :)

jdow - 30.06.18, 05:56:
> On 20180629 18:31, Michael Schmitz wrote:> Joanne,
> 
>  > Am 30.06.18 um 12:57 schrieb jdow:
>  >> On 20180629 17:44, Michael Schmitz wrote:
>  >>> struct PartitionBlock {
>  >>> 
>  >>>            __be32  pb_ID;
>  >>>            __be32  pb_SummedLongs;
>  >>>            __s32   pb_ChkSum;
>  >>>            __u32   pb_HostID;
>  >>>            __be32  pb_Next;
>  >>>            __u32   pb_Flags;
>  >>>            __u32   pb_Reserved1[2];
>  >>>            __u32   pb_DevFlags;
>  >>>            __u8    pb_DriveName[32];
>  >>>            __u32   pb_Reserved2[15];
>  >>>            __be32  pb_Environment[17];
>  >>>            __u32   pb_EReserved[15];
>  >>> 
>  >>> };
>  >>> 
>  >>   pb_Environment = a struct DosEnvec and it is 20 ULONGs in size.
>  >>   I
>  >> 
>  >> believe you are looking at some old include files.
>  > 
>  > Without looking at ancient git history, I'd say between 1993 and
>  > 1996. > 
>  >> These got added to the end of the DosEnvec structure:
>  >>      ULONG de_Baud;         /* Baud rate for serial handler */
>  >>      ULONG de_Control;         /* Control word for
>  >>      handler/filesystem */
>  >>      ULONG de_BootBlocks;     /* Number of blocks containing boot
>  >>      code */
>  >>> 
>  >>> As far as I can guess from the code, pb_Environment[3] (number of
>  >> 
>  >> heads)
>  >> 
>  >>> and pb_Environment[5] (number of sectors per cylinder) are
>  >>> abitrarily
>  >>> chosen so the partition size can be expressed as a difference
>  >>> between
>  >>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
>  >>> addresses), which places restrictions on both partition size and
>  >>> alignment that depend on where on the disk a partition is placed?
>  >> 
>  >> If you do not teach the OS to ignore Cylinder Blocks type entries
>  >> and
>  >> use some math on heads and blocks per track the disk size is
>  >> relatively stuck modulo using large blocks.
>  > 
>  > As long as AmigaOS and Linux agree on how to express start and end
>  > offset for the partitions, that's fine.
>  > 
>  > But I read your other mail to mean that we're stuck to 2 TB disks
>  > for
>  > now. I don't follow that - we can have partitions of 2 TB each by
>  > maxing out rdb_CylBlocks as long as we use 512 bytes per block
>  > (since the product of cylinders and blocks per cylinder is limited
>  > to 32 bits) and using one cylinder per partition (32 bits
>  > available there as well)?
>  > 
>  > But the rdb_CylBlocks limit also means we're safe with 64 bit
>  > sector_t in Linux. Best add a check in the parser to warn us if
>  > the product of head count and sectors per cylinder overflows 32
>  > bit though.
>  > 
>  > Cheers,
>  > 
>  >      Michael
> 
> How long did it tale s to get to 10 TB disks from 2 TB disks. And a
> new SD Card spec allows for 128 TB disks. Block sizes get sort of
> ridiculous as you get past about 8k bytes or about 32 TB or about two
> years from now.
> 
> Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
> far as I know, makes heavy use of Cylinder Blocks values. It
> calculating Cylinder Blocks overflows when creating the disk's RDBs
> the user MUST be informed it is unsafe to put on a real Amiga. (I'd

Joanne, if you are sure on this… I´d say at least warn if not bail out 
on Cylinder Blocks overflow.

But given what you say here, no partitioning tool on AmigaOS or AmigaOS 
like operating system would create such an overflow.

Can you verify whether that is the case with the RDB that I attached to 
the bug report?

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

https://bugzilla.kernel.org/attachment.cgi?id=73771

> also suggest teaching Linux to understand RDSL, which would be RDSK++
> sort of. Then use that if Cylinder Blocks overflows.) The value you
> will not be able to fill in the DosEnvec structure is: ULONG
> de_HighCyl;	     /* max cylinder. drive specific */
>
> 
> So accessing larger disks once you hit 2 TB means you must increase
> the logical block size. And eventually that will waste HUGE amounts
> of files when small files are being stored.

As far as I am aware, AmigaOS 4.1 still only supports 512 byte sectors.
 
> Any solution will require action on the part of the people developing
> AmigaDOS follow-ons. You might want to get them motivated, somehow,
> and proceed from there with a request to be informed of any RDB
> changes. I'd suggest to them that removing sensitivity to Cylinder
> Blocks sorts of values from the entire system probably would be
> painful but the simplest solution.

I think for this patch it is important to focus on the *current* 
situation and make the best out of it.

I am really inclined to point some AmigaOS 4 developers to this 
discussion and just looked for an archive. Unfortunately there does not 
appear to be a working one. The one mentioned on

http://www.linux-m68k.org/mail.html

http://aire.ncl.ac.uk/Atari/Mailing-Lists/Linux-680x0-vger-List.index.html

does not send an answer within the HTTP / TCP timeout limit.

I also did not find any archive for linux-block mailing list.

And lore.kernel.org only seems to archive LKML itself which is patch and 
the discussion we have here is not CC´d to.

Any advice?

Thanks,

Martin Steigerwald June 30, 2018, 8:48 a.m. UTC | #45

Michael. Joanne.

I do think this discussion is slightly getting out of hand… so I suggest 
to focus on what its up to the kernel to do and what is not. And to 
focus only on what is up to the RDB parser, cause the patch is on 
changing that. The RDB parser is not responsible for what any file 
system may do. Securing AFFS would be a different, important, topic.

With that mail I am probably out as this discussion took already quite a 
bit of time.

But for details, read on:

Michael Schmitz - 30.06.18, 07:26:
> Am 30.06.18 um 15:56 schrieb jdow:
> > >>> As far as I can guess from the code, pb_Environment[3] (number
> > >>> of
> > >> 
> > >> heads)
> > >> 
> > >>> and pb_Environment[5] (number of sectors per cylinder) are
> > >>> abitrarily
> > >>> chosen so the partition size can be expressed as a difference
> > >>> between
> > >>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
> > >>> addresses), which places restrictions on both partition size and
> > >>> alignment that depend on where on the disk a partition is
> > >>> placed?
> > >> 
> > >> If you do not teach the OS to ignore Cylinder Blocks type entries
> > >> and
> > >> use some math on heads and blocks per track the disk size is
> > >> relatively stuck modulo using large blocks.
> > > 
> > > As long as AmigaOS and Linux agree on how to express start and end
> > > offset for the partitions, that's fine.
> > > 
> > > But I read your other mail to mean that we're stuck to 2 TB disks
> > > for
> > > now. I don't follow that - we can have partitions of 2 TB each by
> > 
> > maxing
> > 
> > > out rdb_CylBlocks as long as we use 512 bytes per block (since the
> > > product of cylinders and blocks per cylinder is limited to 32
> > > bits) and using one cylinder per partition (32 bits available
> > > there as well)?
> > > 
> > > But the rdb_CylBlocks limit also means we're safe with 64 bit
> > > sector_t in Linux. Best add a check in the parser to warn us if
> > > the product of head count and sectors per cylinder overflows 32
> > > bit though.
> > > 
> > > Cheers,
> > >
> > >      Michael
> > 
> > How long did it tale s to get to 10 TB disks from 2 TB disks. And a
> > new SD Card spec allows for 128 TB disks. Block sizes get sort of
> > ridiculous as you get past about 8k bytes or about 32 TB or about
> > two
> > years from now.
> 
> I get that - I just don't get why 32 bits for cylinders plus 32 bits
> for blocks per cylinder equals 2 TB (4G of 512 byte blocks). But I
> don't know what other limits exist that may restrict the total number
> of blocks to 32 bits.

I think for the total device size:

> The raw, theoretical limit on the maximum device capacity is about
> 2^105 bytes:
> 
> 32 bit rdb_Cylinders * 32 bit rdb_Heads * 32 bit rdb_Sectors * 512
> bytes/sector for the HD size in struct RigidDiskBlock

is correct. Do you agree, Joanne?

http://wiki.amigaos.net/wiki/RDB

If so, we can remove that limit from the discussion and focus on what 
remains.


As for the partition sizes, how about

> Partition size
> 
> For the partitions the maximum size is:
> 32 bit (de_HighCyl + 1 - de_LowCyl) (to get the partition size) * 32
> bit de_Surfaces * 32 bit de_SectorsPerTrack * 512 bytes/sector in
> struct DosEnvec (=pb_Environment[]) in struct PartitionBlock.
>
> That's from the physical drive part, the actual disk size limit for > 
> the partitions may be much smaller depending on the partitioning
> software, if it's only using the logical sizes instead, which is
> likely the case, it's only 8 ZiB with 512 bytes/sector: 32 bit
> rdb_HiCylinder * 32 bit rdb_CylBlocks * 512 bytes/sector = 2^73
> bytes. For using the logical sizes using simple uint64 calculations
> (with some overflow checks) should be enough, for more a math library
> with support for larger integers needs to be used which probably no
> partitioning software does.

taken from the same wiki page?

I however do not get what it means with "logical sizes".

Joanne, what is your feedback on this?

> > Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
> > far as I know, makes heavy use of Cylinder Blocks values. It
> > calculating Cylinder Blocks overflows when creating the disk's RDBs
> > the user MUST be informed it is
> 
> I'm not at all planning to create disks for AmigaDOS. I just need to
> know what combinations of cylinders, heads and sectors are possible to
> encounter on disks that have been created with native tools. Well,

Do we really need to add checks to those possible combinations values, 
Joanne? Cause, if we do, how do we find out?

Again, the Linux RDB parser just *reads* the RDB.

And already on the Amiga side it can happen that one partitioning tool 
does not understand the other. That Rigid Disk Block is not quite as 
rigid as the name would suggest, you already said its very flexible and 
that is not always a good thing. I have seen a Phase 5 RDB partitioning 
tool *crush* an RDB created by HDToolBox cause one calculated from 0 and 
one from zero.

I do think is neither the duty nor the responsibility of the Linux 
kernel to check for such crap.

Especially as, as I will point out further down in the mail, it is 
difficult to impossible to actually know for sure which combinations any 
of the partitioning tools on native os´s allowed.

Even if we´d find out the possible combinations of the official tools, 
there are RDB tools like RDBSalv and what not.

I´d say it is important to avoid over complicating things. We cannot 
fulfill and it is not our responsibility to save the user from any brain 
damage any partitioning tool on either native OS or Linux created 
*within* the kernel.

So I´d recommend: 

- Make the calculations bail out on overflows and refuse to load the RDB 
if any calculation overflows.

- Ideally also secure AFFS this way.

- Have the warning about 64 bit disk support on native OS

and *be done with it*. For the kernel.

> assuming sufficient amounts of braindamage in the corresponding Linux
> tools, knowing the absolute outer limits of what these tools could do
> would be nice as well, but someone using amiga-fdisk to create a RDSK
> block for a 10 TB disk fully deserves any punishment that invites.

If there is any warning due to using limits that exceed what HDToolBox 
(AmigaOS upto 3.1), HDToolbox using hdwrench.library (AmigaOS 3.5 
onwards), Media Toolbox using SP_engine (AmigaOS 4 onwards) as well as 
all the tools in Morphos, AROS and on AmiNet woul use, it would be up to 
amiga-fdisk and parted to issue such a warning.

As for the RDB parser, I´d go with robustness principle:

Be conservative in what you send, be liberal in what you accept.

https://en.wikipedia.org/wiki/Robustness_principle

So what ever crap or non crap any partitioning tool creates, check for 
any overflow in calculations and only refuse to accept the result in 
case there have been an overflow in calculations.

If a value is an ULONG, accept the full 32 bit unsigned integer. Cause 
if you don´t, it could mean the user is not able to access the disk with 
that RDB.

Partitioning tools however can impose any they think make sense.

As for limits with in native OS partitioning tools, for HDToolbox with 
and without hdwrench.library you probably remember, Joanne. As for Media 
Toolbox we´d have to ask AmigaOS developers. Remember AmigaOS and 
Morphos development are still closed source. So there is nothing except 
the SDKs and their Documentation which we can use to know for sure.

The latest AmigaOS 4.1 development kit appears to be version 53.30, 
found at. It is for AmigaOS 4.1 Final Edition:

https://www.hyperion-entertainment.biz/index.php/downloads?
view=files&parent=30

(you need to click the link to arrive an an extra page and click the 
link there)

base.lha in SDK_Install contains the documentation. I was able to unpack 
it with lha x on Linux. However I have no idea where that lha command on 
my Debian GNU/Sid setup came from:

% LANG=C dpkg -S /usr/bin/lha
dpkg-query: no path found matching pattern /usr/bin/lha

I bet there is some Linux lha downloadable somewhere, if need be I can 
dig for it.

However I used grep and find on various variations of Media Toolbox, 
SP_Engine and RDB as well as Rigid Disk Block and found nothing of 
interest for our topic.

So it appears that the workings of SP_Engine are buried in source code 
we don´t have access to. And well I won´t base any limit checking on 
assumptions. So I won´t set any additional limits than the check for 
overflowing calculations.

The AmigaOS 3.9 NDK still appears to be available on the website of my 
former employee HAAGE&PARTNER GmbH:

https://www.haage-partner.de/download/AmigaOS/NDK39.lha

This one included the hdwrench.library autodocs and releasenotes. I do 
not know whether they have something about limits. However, it does not 
appear so:

> […] NDK_3.9/Documentation% grep limit Autodocs/hdwrench.doc
> 
>     This function is intentionally limited writing only to the lower
>     block

This is a limit about writing the data section of a BootBlock, so 
unrelated to our topic.

> […] NDK_3.9/Documentation% grep limit
> Releasenotes/hdwrench_lib_relnotes

Michael:
> (Actually, I lied there. I do plan to create a RDSK block for a 2 TB
> disk image where cylinder, head and sector counts all approach the 32
> bit limit, just to see that my overflow checks work as intended. But
> that's strictly for Linux testing).

I think that is a good test for the patch. I am impressed you are 
willing to put the effort for that into this work.

Okay, that is what I was able to dig out on what is officially 
available. It appears that the official developer documentation for 
AmigaOS is pretty sparse on any of that. For anything else I´d need to 
contact current AmigaOS developers. I am no longer a member of the 
AmigaOS team.

Maybe the ADF Format FAQ I mentioned has a bit more, but I did not find 
anything obvious on a short glance

http://lclevy.free.fr/adflib/adf_info.html

Also it is a third party source.

> > unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
> > understand RDSL, which would be RDSK++ sort of. Then use that if
> > Cylinder Blocks overflows.) The value you will not be able to fill
> > in
> > the DosEnvec structure is:
> >     ULONG de_HighCyl;         /* max cylinder. drive specific */
> 
> OK, so Cylinder Blocks overflowing is a red flag, and requires to
> abort parsing the partition table right away? And HighCyl really
> means the max. number of logical blocks, not cylinders (which would
> have nr_heads*nr_sects many blocks)? That's probably the cause for my
> confusion.

When can they actually overflow? How are they calculated?

If there is no calculation to calculate it and it is just a static value 
within the RDB, I´d accept what ever the value is in the RDB, unless a 
calculation based on it overflows.

Again, keep it simple.

I think it does not make sense for the Linux kernel to try to outsmart 
the user or any of the partitioning tools out there.

The only question is:

Can the partitions be calculated in a way that it is safe to access 
them, i.e. that their calculated start and end is really at the location 
the start and end is on the disk? Then accept it.

For the file systems: Can the file system safely access this? Then 
accept. If not, then decline. *In the filesystem*. This is nothing for 
the RDB parser to check so out of scope for the discussion of this 
patch.

> > So accessing larger disks once you hit 2 TB means you must increase
> > the logical block size. And eventually that will waste HUGE amounts
> > of files when small files are being stored.
> 
> Just like small inodes wastes huge amounts of space for metadata. It's
> a tradeoff, and AFFS on a RDSK format disk probably isn't the right
> choice for huge disks. Never mind that - if someone _does_ go that
> way, we need to make sure we can parse the RDSK information
> correctly. And if such a disk causes the 64 bit sector_t in Linux to
> overflow, I'd like the parser to spot that, too.
> 
> Thanks for your immense patience in explaining all these subtleties to
> me.
>
> Cheers,
> 
>     Michael
> 
> > Any solution will require action on the part of the people
> > developing
> > AmigaDOS follow-ons. You might want to get them motivated, somehow,
> > and proceed from there with a request to be informed of any RDB
> > changes. I'd suggest to them that removing sensitivity to Cylinder
> > Blocks sorts of values from the entire system probably would be
> > painful but the simplest solution.
> > 
> > {^_^}
[…]

Martin Steigerwald June 30, 2018, 9:07 a.m. UTC | #46

jdow - 30.06.18, 08:47:
> Let's get everybody:
> 
> On 20180629 22:26, Michael Schmitz wrote:
>  > Joanne,
>  > 
>  > Am 30.06.18 um 15:56 schrieb jdow:
>  >>>>> As far as I can guess from the code, pb_Environment[3] (number
>  >>>>> of
>  >>>> 
>  >>>> heads)
>  >>>> 
>  >>>>> and pb_Environment[5] (number of sectors per cylinder) are
>  >>>>> abitrarily
>  >>>>> chosen so the partition size can be expressed as a difference
>  >>>>> between
>  >>>>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
>  >>>>> addresses), which places restrictions on both partition size
>  >>>>> and
>  >>>>> alignment that depend on where on the disk a partition is
>  >>>>> placed?
>  >>>> 
>  >>>> If you do not teach the OS to ignore Cylinder Blocks type
>  >>>> entries and
>  >>>> use some math on heads and blocks per track the disk size is
>  >>>> relatively stuck modulo using large blocks.
>  >>> 
>  >>> As long as AmigaOS and Linux agree on how to express start and
>  >>> end
>  >>> offset for the partitions, that's fine.
>  >>> 
>  >>> But I read your other mail to mean that we're stuck to 2 TB disks
>  >>> for
>  >>> now. I don't follow that - we can have partitions of 2 TB each by
>  >> 
>  >> maxing
>  >> 
>  >>> out rdb_CylBlocks as long as we use 512 bytes per block (since
>  >>> the
>  >>> product of cylinders and blocks per cylinder is limited to 32
>  >>> bits) and using one cylinder per partition (32 bits available
>  >>> there as well)?
>  >>> 
>  >>> But the rdb_CylBlocks limit also means we're safe with 64 bit
>  >>> sector_t in Linux. Best add a check in the parser to warn us if
>  >>> the product of head count and sectors per cylinder overflows 32
>  >>> bit though.
>  >>> 
>  >>> Cheers,
>  >>> 
>  >>>        Michael
>  >> 
>  >> How long did it tale s to get to 10 TB disks from 2 TB disks. And
>  >> a
>  >> new SD Card spec allows for 128 TB disks. Block sizes get sort of
>  >> ridiculous as you get past about 8k bytes or about 32 TB or about
>  >> two
>  >> years from now.
>  > 
>  > I get that - I just don't get why 32 bits for cylinders plus 32
>  > bits for blocks per cylinder equals 2 TB (4G of 512 byte blocks).
>  > But I don't know what other limits exist that may restrict the
>  > total number of blocks to 32 bits.
> 
> It overflows uint32_t cylinder blocks aka blocks per cylinder. Linux
> doesn't care. AmigaDOS surely does. If YOU make partitions really
> large for yourself that's OK. If Joe Amigoid does it the potential
> for an angry red turning to purple face is high.

Ok, let get this straight:

Do you think that is the responsibility of the RDB parser within the 
Linux kernel to protect the user from anything whatever partitioning 
tool has created?

If so, how would you make sure the Linux kernel knows about whatever any 
partitioning tool used by Amiga users can come up with? 

I´d say: Don´t bother. It is not the job of the RDB parser to impose 
limits on what partitioning tools can create. If native OS tools don´t 
create such thing, you don´t need to check for it. If someone managed to 
create it with amiga-fdisk or parted, the tool needs to be fixed. *Not* 
the kernel.

Anyway, that 2 TB disk that started all this *worked* on AmigaOS 4. And 
I am pretty sure while I cannot proof it, that even a larger disk would 
work. There is a limit for the boot partition on AmigaOS 4 Classic, 
which uses AmigaOS 3.1 to bootstrap AmigaOS 4 on Classic Amiga computers 
like an Amiga 1200 or Amiga 4000 with PowerPC extension card. But 
according to

Hard drive setup for AmigaOS 4.1 Classic
http://blog.hyperion-entertainment.biz/?p=210

AmigaOS classic (i.e. < 4) would crash.

>  >> Do you want to create disks that will fail on AmigaDOS? AmigaDOS,
>  >> as
>  >> far as I know, makes heavy use of Cylinder Blocks values. It
>  >> calculating Cylinder Blocks overflows when creating the disk's
>  >> RDBs
>  >> the user MUST be informed it is
>  > 
>  > I'm not at all planning to create disks for AmigaDOS. I just need
>  > to
>  > know what combinations of cylinders, heads and sectors are possible
>  > to encounter on disks that have been created with native tools.
>  > Well, assuming sufficient amounts of braindamage in the
>  > corresponding Linux tools, knowing the absolute outer limits of
>  > what these tools could do would be nice as well, but someone using
>  > amiga-fdisk to create a RDSK block for a 10 TB disk fully deserves
>  > any punishment that invites.
> Native AmigaDOS tools SHOULD NOT be able to create something that
> overflows CylinderBlocks values. However, if it can that creates an

There you have it.

Then *why* bother, Joanne?

> interesting test case to see what various tools, like the AmigaDOS
> "info" command, do when they they are run on such a disk. I don't
> have OS source to perform searches. And I am not setup to feed the
> system something obscene.
> 
>  > (Actually, I lied there. I do plan to create a RDSK block for a 2
>  > TB
>  > disk image where cylinder, head and sector counts all approach the
>  > 32
>  > bit limit, just to see that my overflow checks work as intended.
>  > But
>  > that's strictly for Linux testing).
>  > 
>  >> unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
>  >> understand RDSL, which would be RDSK++ sort of. Then use that if
>  >> Cylinder Blocks overflows.) The value you will not be able to fill
>  >> in
>  >> 
>  >> the DosEnvec structure is:
>  >>      ULONG de_HighCyl;         /* max cylinder. drive specific */
>  > 
>  > OK, so Cylinder Blocks overflowing is a red flag, and requires to
>  > abort parsing the partition table right away? And HighCyl really
>  > means the max. number of logical blocks, not cylinders (which
>  > would have nr_heads*nr_sects many blocks)? That's probably the
>  > cause for my confusion.
> I think I picked the wrong value. In RDSK itself this value is what
> overflows: ULONG   rdb_CylBlocks;    /* number of blocks available
> per cylinder */ And I think that floats around the system in many
> places with different names. As mentioned the "info" command is one
> item to test. If no crashes are found then AmigaDOS may be clean up
> to obscene sizes. At the moment I do not remember what
> hdwrench.library does with that value other than pass it along as
> read. Nor am I sure what it generates as any suggested values. I
> don't at this time have a disk I can mount as a disk on WinUAE that
> is more than 2TB. And my Amigas speak SCSI so I have no disk for
> them, either, even if they still boot.
>  >> So accessing larger disks once you hit 2 TB means you must
>  >> increase
>  >> the logical block size. And eventually that will waste HUGE
>  >> amounts of files when small files are being stored.
>  > 
>  > Just like small inodes wastes huge amounts of space for metadata.
>  > It's a tradeoff, and AFFS on a RDSK format disk probably isn't the
>  > right choice for huge disks. Never mind that - if someone _does_
>  > go that way, we need to make sure we can parse the RDSK
>  > information correctly. And if such a disk causes the 64 bit
>  > sector_t in Linux to overflow, I'd like the parser to spot that,
>  > too.
>  > 
>  > Thanks for your immense patience in explaining all these subtleties
>  > to me.
[…]
>  >      Michael
> 
> And I'm rushing too much so I'm sorry I am making errors. This stuff
> is 25 years in the past since I last looked at it seriously.

I think its important to focus on what can overflow can happen within 
calculations the RDB parser (and as a second step the AFFS file system) 
in the kernel kernel in order to keep this discussion to a manageable 
size. Be conservative about overflows, but otherwise accept. With a 
warning if a calculated exceed 32 bit.

As for values in the RDB. If its there, accept it. Some tool has written 
it there. We don´t know whether it did this right or wrong. We don´t 
know what the developer of the tool thought when writing it, well except 
for hdwrench.library I´d say as far as you remember. :) And it is not 
our job within the kernel to check that.

There is a ton of more or less legacy software out there on native OS 
which does something to or with RDBs. I´d say it is impossible to say 
what RDB a user may come up with.

Thanks,

jdow June 30, 2018, 9:28 a.m. UTC | #47

For Linux:
1) Make a change to the Linux RDB parser so that the product of the "CHS" values 
goes to at least a 64 bit entity, It may need to go to a 128 bit entity when we 
are encoding data in DNA, crystal lattices, or something else super dense. The 
parser simply feeds data to the OS. No warnings are needed. No errors are 
expected but if they happen do not mount the disk and post an error message the 
user can use to diagnose the problem. "It's broken," is insufficient.
2) if there is an mkfs-(amiga file system of some sort) utility audit it for 
generating values as proper as possible. For the interim a warning of heads 
times sectors overflows uint32_t size storage.

For AmigaDOS:
1) Be aware that there is a potential for problems in the AmigaDOS utilities, 
filesystem, and device driver interfaces. This should be audited for overflow 
issues and to see of the rdb_CylinderBlocks value propagates through the system 
or an equivalent is created as a uint32_t size value. This is a potential 
problem. And fix them.
2) If 4.1 cannot handle block sizes other than 512 bytes it is broken and should 
be fixed. This goes for all tools and utilities as well. Incompatible utilities 
that are found should be flagged to users. This might include old versions of 
AmigaDOS partitioning tools as well as most anything else found to use the "CHS" 
and "block size" values. The size of this audit is unknown. If source code is in 
one place it can be searched for the use of these values as well as 
rdb_CylinderBlocks itself.

Regardless of how you fiddle the values the limit is beyond the size needed to 
have each atom in the visible universe individually addressed by several orders 
of magnitude. The ultimate limit is 2^128. Your value is too small by a few 
orders of magnitude. It's academic, though. This is all predicted on auditing 
the system's use of the "CHS" and block size values and repairing errors found.

Logical block sizes come from using say 16 sectors as one block, or in MS 
parlance "cluster". Call it an space allocation unit.

"Even if we´d find out the possible combinations of the official tools,
there are RDB tools like RDBSalv and what not." "All" means "All." If no 
maintainer exists then flag the tool as broken, and for God's sake don't 
distribute it with the OS.

Re hdwrench.library, I have all the source code. I've wasted enough time that I 
am HIGHLY unmotivated to dig into this morass any further.

On a side note, if HAAGE&PARTNER developed 3.whatever which included 
hdwrench.library as a work for hire for you then that source code can be 
located, including a fairly complete history if its development. Once located it 
can be posted to you if you cannot find it. It's copyrighted to H&P. I did it as 
a consultant for them. If you hired them to do the work then the copyright is 
misattributed and can be rectified.

Regarding hard block writing limitations - THAT must be near the front of super 
large disks. On an 18 gigabyte disk you can even get away with putting the RDBs 
at the end of the disk except for the RDSK block which must be in the first 16 
physical sectors. Everything else is uint32_t block number pointers. {^_-} Use 
your imagination, Luke! {^_-}

Any code that uses the "CHS" values to calculate a position or size of a disk is 
potentially broken. Hopefully there are only a very few. But I do not know and 
am a known paranoid.

At one time I had the impression that filesystems worked based on block numbers 
on the disk, first block and last block of the partition with no particular 
attention being paid to the "CHS" values. Of course the filesystem must think 
properly in terms of cluster size aka logical block size aka N * sector size. 
Erm, usually N is expected to be a power of 2. Non-powers of 2 MIGHT work in 
some cases. The filesystem gets an fssm message. That includes a DosEnvec. 
DosEnvec includes disk information for block size and "CHS" but at the moment I 
don't see a CylinderBlocks value hiding in it. I thought I had seen such at one 
time so in my hurry I may be going past it.

Somebody ****ELSE**** has some work to do now.

{^_^}

On 20180630 01:48, Martin Steigerwald wrote:
> Michael. Joanne.
> 
> I do think this discussion is slightly getting out of hand… so I suggest
> to focus on what its up to the kernel to do and what is not. And to
> focus only on what is up to the RDB parser, cause the patch is on
> changing that. The RDB parser is not responsible for what any file
> system may do. Securing AFFS would be a different, important, topic.
> 
> With that mail I am probably out as this discussion took already quite a
> bit of time.
> 
> But for details, read on:
> 
> Michael Schmitz - 30.06.18, 07:26:
>> Am 30.06.18 um 15:56 schrieb jdow:
>>>>>> As far as I can guess from the code, pb_Environment[3] (number
>>>>>> of
>>>>>
>>>>> heads)
>>>>>
>>>>>> and pb_Environment[5] (number of sectors per cylinder) are
>>>>>> abitrarily
>>>>>> chosen so the partition size can be expressed as a difference
>>>>>> between
>>>>>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
>>>>>> addresses), which places restrictions on both partition size and
>>>>>> alignment that depend on where on the disk a partition is
>>>>>> placed?
>>>>>
>>>>> If you do not teach the OS to ignore Cylinder Blocks type entries
>>>>> and
>>>>> use some math on heads and blocks per track the disk size is
>>>>> relatively stuck modulo using large blocks.
>>>>
>>>> As long as AmigaOS and Linux agree on how to express start and end
>>>> offset for the partitions, that's fine.
>>>>
>>>> But I read your other mail to mean that we're stuck to 2 TB disks
>>>> for
>>>> now. I don't follow that - we can have partitions of 2 TB each by
>>>
>>> maxing
>>>
>>>> out rdb_CylBlocks as long as we use 512 bytes per block (since the
>>>> product of cylinders and blocks per cylinder is limited to 32
>>>> bits) and using one cylinder per partition (32 bits available
>>>> there as well)?
>>>>
>>>> But the rdb_CylBlocks limit also means we're safe with 64 bit
>>>> sector_t in Linux. Best add a check in the parser to warn us if
>>>> the product of head count and sectors per cylinder overflows 32
>>>> bit though.
>>>>
>>>> Cheers,
>>>>
>>>>       Michael
>>>
>>> How long did it tale s to get to 10 TB disks from 2 TB disks. And a
>>> new SD Card spec allows for 128 TB disks. Block sizes get sort of
>>> ridiculous as you get past about 8k bytes or about 32 TB or about
>>> two
>>> years from now.
>>
>> I get that - I just don't get why 32 bits for cylinders plus 32 bits
>> for blocks per cylinder equals 2 TB (4G of 512 byte blocks). But I
>> don't know what other limits exist that may restrict the total number
>> of blocks to 32 bits.
> 
> I think for the total device size:
> 
>> The raw, theoretical limit on the maximum device capacity is about
>> 2^105 bytes:
>>
>> 32 bit rdb_Cylinders * 32 bit rdb_Heads * 32 bit rdb_Sectors * 512
>> bytes/sector for the HD size in struct RigidDiskBlock
> 
> is correct. Do you agree, Joanne?
> 
> http://wiki.amigaos.net/wiki/RDB
> 
> If so, we can remove that limit from the discussion and focus on what
> remains.
> 
> 
> As for the partition sizes, how about
> 
>> Partition size
>>
>> For the partitions the maximum size is:32 bit (de_HighCyl + 1 - de_LowCyl) (to get the partition size) * 32
>> bit de_Surfaces * 32 bit de_SectorsPerTrack * 512 bytes/sector in
>> struct DosEnvec (=pb_Environment[]) in struct PartitionBlock.
>> That's from the physical drive part, the actual disk size limit for >
>> the partitions may be much smaller depending on the partitioning
>> software, if it's only using the logical sizes instead, which is
>> likely the case, it's only 8 ZiB with 512 bytes/sector: 32 bit
>> rdb_HiCylinder * 32 bit rdb_CylBlocks * 512 bytes/sector = 2^73
>> bytes. For using the logical sizes using simple uint64 calculations
>> (with some overflow checks) should be enough, for more a math library
>> with support for larger integers needs to be used which probably no
>> partitioning software does.
> 
> taken from the same wiki page?
> 
> I however do not get what it means with "logical sizes".
> 
> Joanne, what is your feedback on this?
> 
>>> Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
>>> far as I know, makes heavy use of Cylinder Blocks values. It
>>> calculating Cylinder Blocks overflows when creating the disk's RDBs
>>> the user MUST be informed it is
>>
>> I'm not at all planning to create disks for AmigaDOS. I just need to
>> know what combinations of cylinders, heads and sectors are possible to
>> encounter on disks that have been created with native tools. Well,
> 
> Do we really need to add checks to those possible combinations values,
> Joanne? Cause, if we do, how do we find out?
> 
> Again, the Linux RDB parser just *reads* the RDB.
> 
> And already on the Amiga side it can happen that one partitioning tool
> does not understand the other. That Rigid Disk Block is not quite as
> rigid as the name would suggest, you already said its very flexible and
> that is not always a good thing. I have seen a Phase 5 RDB partitioning
> tool *crush* an RDB created by HDToolBox cause one calculated from 0 and
> one from zero.
> 
> I do think is neither the duty nor the responsibility of the Linux
> kernel to check for such crap.
> 
> Especially as, as I will point out further down in the mail, it is
> difficult to impossible to actually know for sure which combinations any
> of the partitioning tools on native os´s allowed.
> 
> Even if we´d find out the possible combinations of the official tools,
> there are RDB tools like RDBSalv and what not.
> 
> I´d say it is important to avoid over complicating things. We cannot
> fulfill and it is not our responsibility to save the user from any brain
> damage any partitioning tool on either native OS or Linux created
> *within* the kernel.
> 
> So I´d recommend:
> 
> - Make the calculations bail out on overflows and refuse to load the RDB
> if any calculation overflows.
> 
> - Ideally also secure AFFS this way.
> 
> - Have the warning about 64 bit disk support on native OS
> 
> and *be done with it*. For the kernel.
> 
>> assuming sufficient amounts of braindamage in the corresponding Linux
>> tools, knowing the absolute outer limits of what these tools could do
>> would be nice as well, but someone using amiga-fdisk to create a RDSK
>> block for a 10 TB disk fully deserves any punishment that invites.
> 
> If there is any warning due to using limits that exceed what HDToolBox
> (AmigaOS upto 3.1), HDToolbox using hdwrench.library (AmigaOS 3.5
> onwards), Media Toolbox using SP_engine (AmigaOS 4 onwards) as well as
> all the tools in Morphos, AROS and on AmiNet woul use, it would be up to
> amiga-fdisk and parted to issue such a warning.
> 
> As for the RDB parser, I´d go with robustness principle:
> 
> Be conservative in what you send, be liberal in what you accept.
> 
> https://en.wikipedia.org/wiki/Robustness_principle
> 
> So what ever crap or non crap any partitioning tool creates, check for
> any overflow in calculations and only refuse to accept the result in
> case there have been an overflow in calculations.
> 
> If a value is an ULONG, accept the full 32 bit unsigned integer. Cause
> if you don´t, it could mean the user is not able to access the disk with
> that RDB.
> 
> Partitioning tools however can impose any they think make sense.
> 
> As for limits with in native OS partitioning tools, for HDToolbox with
> and without hdwrench.library you probably remember, Joanne. As for Media
> Toolbox we´d have to ask AmigaOS developers. Remember AmigaOS and
> Morphos development are still closed source. So there is nothing except
> the SDKs and their Documentation which we can use to know for sure.
> 
> The latest AmigaOS 4.1 development kit appears to be version 53.30,
> found at. It is for AmigaOS 4.1 Final Edition:
> 
> https://www.hyperion-entertainment.biz/index.php/downloads?
> view=files&parent=30
> 
> (you need to click the link to arrive an an extra page and click the
> link there)
> 
> base.lha in SDK_Install contains the documentation. I was able to unpack
> it with lha x on Linux. However I have no idea where that lha command on
> my Debian GNU/Sid setup came from:
> 
> % LANG=C dpkg -S /usr/bin/lha
> dpkg-query: no path found matching pattern /usr/bin/lha
> 
> I bet there is some Linux lha downloadable somewhere, if need be I can
> dig for it.
> 
> However I used grep and find on various variations of Media Toolbox,
> SP_Engine and RDB as well as Rigid Disk Block and found nothing of
> interest for our topic.
> 
> So it appears that the workings of SP_Engine are buried in source code
> we don´t have access to. And well I won´t base any limit checking on
> assumptions. So I won´t set any additional limits than the check for
> overflowing calculations.
> 
> The AmigaOS 3.9 NDK still appears to be available on the website of my
> former employee HAAGE&PARTNER GmbH:
> 
> https://www.haage-partner.de/download/AmigaOS/NDK39.lha
> 
> This one included the hdwrench.library autodocs and releasenotes. I do
> not know whether they have something about limits. However, it does not
> appear so:
> 
>> […] NDK_3.9/Documentation% grep limit Autodocs/hdwrench.doc
>>
>>      This function is intentionally limited writing only to the lower
>>      block
> 
> This is a limit about writing the data section of a BootBlock, so
> unrelated to our topic.
> 
>> […] NDK_3.9/Documentation% grep limit
>> Releasenotes/hdwrench_lib_relnotes
> 
> Michael:
>> (Actually, I lied there. I do plan to create a RDSK block for a 2 TB
>> disk image where cylinder, head and sector counts all approach the 32
>> bit limit, just to see that my overflow checks work as intended. But
>> that's strictly for Linux testing).
> 
> I think that is a good test for the patch. I am impressed you are
> willing to put the effort for that into this work.
> 
> Okay, that is what I was able to dig out on what is officially
> available. It appears that the official developer documentation for
> AmigaOS is pretty sparse on any of that. For anything else I´d need to
> contact current AmigaOS developers. I am no longer a member of the
> AmigaOS team.
> 
> Maybe the ADF Format FAQ I mentioned has a bit more, but I did not find
> anything obvious on a short glance
> 
> http://lclevy.free.fr/adflib/adf_info.html
> 
> Also it is a third party source.
> 
>>> unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
>>> understand RDSL, which would be RDSK++ sort of. Then use that if
>>> Cylinder Blocks overflows.) The value you will not be able to fill
>>> in
>>> the DosEnvec structure is:
>>>      ULONG de_HighCyl;         /* max cylinder. drive specific */
>>
>> OK, so Cylinder Blocks overflowing is a red flag, and requires to
>> abort parsing the partition table right away? And HighCyl really
>> means the max. number of logical blocks, not cylinders (which would
>> have nr_heads*nr_sects many blocks)? That's probably the cause for my
>> confusion.
> 
> When can they actually overflow? How are they calculated?
> 
> If there is no calculation to calculate it and it is just a static value
> within the RDB, I´d accept what ever the value is in the RDB, unless a
> calculation based on it overflows.
> 
> Again, keep it simple.
> 
> I think it does not make sense for the Linux kernel to try to outsmart
> the user or any of the partitioning tools out there.
> 
> The only question is:
> 
> Can the partitions be calculated in a way that it is safe to access
> them, i.e. that their calculated start and end is really at the location
> the start and end is on the disk? Then accept it.
> 
> For the file systems: Can the file system safely access this? Then
> accept. If not, then decline. *In the filesystem*. This is nothing for
> the RDB parser to check so out of scope for the discussion of this
> patch.
> 
>>> So accessing larger disks once you hit 2 TB means you must increase
>>> the logical block size. And eventually that will waste HUGE amounts
>>> of files when small files are being stored.
>>
>> Just like small inodes wastes huge amounts of space for metadata. It's
>> a tradeoff, and AFFS on a RDSK format disk probably isn't the right
>> choice for huge disks. Never mind that - if someone _does_ go that
>> way, we need to make sure we can parse the RDSK information
>> correctly. And if such a disk causes the 64 bit sector_t in Linux to
>> overflow, I'd like the parser to spot that, too.
>>
>> Thanks for your immense patience in explaining all these subtleties to
>> me.
>>
>> Cheers,
>>
>>      Michael
>>
>>> Any solution will require action on the part of the people
>>> developing
>>> AmigaDOS follow-ons. You might want to get them motivated, somehow,
>>> and proceed from there with a request to be informed of any RDB
>>> changes. I'd suggest to them that removing sensitivity to Cylinder
>>> Blocks sorts of values from the entire system probably would be
>>> painful but the simplest solution.
>>>
>>> {^_^}
> […]
>

jdow June 30, 2018, 9:36 a.m. UTC | #48

Get everybody....

On 20180630 00:49, Martin Steigerwald wrote:
 > Whoa, my summary essay triggered digging even more accurately into that
 > matter. For some obscure reason I am even enjoying this. :)
 >
 > jdow - 30.06.18, 05:56:
 >> On 20180629 18:31, Michael Schmitz wrote:> Joanne,
 >>
 >>   > Am 30.06.18 um 12:57 schrieb jdow:
 >>   >> On 20180629 17:44, Michael Schmitz wrote:
 >>   >>> struct PartitionBlock {
 >>   >>>
 >>   >>>            __be32  pb_ID;
 >>   >>>            __be32  pb_SummedLongs;
 >>   >>>            __s32   pb_ChkSum;
 >>   >>>            __u32   pb_HostID;
 >>   >>>            __be32  pb_Next;
 >>   >>>            __u32   pb_Flags;
 >>   >>>            __u32   pb_Reserved1[2];
 >>   >>>            __u32   pb_DevFlags;
 >>   >>>            __u8    pb_DriveName[32];
 >>   >>>            __u32   pb_Reserved2[15];
 >>   >>>            __be32  pb_Environment[17];
 >>   >>>            __u32   pb_EReserved[15];
 >>   >>>
 >>   >>> };
 >>   >>>
 >>   >>   pb_Environment = a struct DosEnvec and it is 20 ULONGs in size.
 >>   >>   I
 >>   >>
 >>   >> believe you are looking at some old include files.
 >>   >
 >>   > Without looking at ancient git history, I'd say between 1993 and
 >>   > 1996. >
 >>   >> These got added to the end of the DosEnvec structure:
 >>   >>      ULONG de_Baud;         /* Baud rate for serial handler */
 >>   >>      ULONG de_Control;         /* Control word for
 >>   >>      handler/filesystem */
 >>   >>      ULONG de_BootBlocks;     /* Number of blocks containing boot
 >>   >>      code */
 >>   >>>
 >>   >>> As far as I can guess from the code, pb_Environment[3] (number of
 >>   >>
 >>   >> heads)
 >>   >>
 >>   >>> and pb_Environment[5] (number of sectors per cylinder) are
 >>   >>> abitrarily
 >>   >>> chosen so the partition size can be expressed as a difference
 >>   >>> between
 >>   >>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
 >>   >>> addresses), which places restrictions on both partition size and
 >>   >>> alignment that depend on where on the disk a partition is placed?
 >>   >>
 >>   >> If you do not teach the OS to ignore Cylinder Blocks type entries
 >>   >> and
 >>   >> use some math on heads and blocks per track the disk size is
 >>   >> relatively stuck modulo using large blocks.
 >>   >
 >>   > As long as AmigaOS and Linux agree on how to express start and end
 >>   > offset for the partitions, that's fine.
 >>   >
 >>   > But I read your other mail to mean that we're stuck to 2 TB disks
 >>   > for
 >>   > now. I don't follow that - we can have partitions of 2 TB each by
 >>   > maxing out rdb_CylBlocks as long as we use 512 bytes per block
 >>   > (since the product of cylinders and blocks per cylinder is limited
 >>   > to 32 bits) and using one cylinder per partition (32 bits
 >>   > available there as well)?
 >>   >
 >>   > But the rdb_CylBlocks limit also means we're safe with 64 bit
 >>   > sector_t in Linux. Best add a check in the parser to warn us if
 >>   > the product of head count and sectors per cylinder overflows 32
 >>   > bit though.
 >>   >
 >>   > Cheers,
 >>   >
 >>   >      Michael
 >>
 >> How long did it tale s to get to 10 TB disks from 2 TB disks. And a
 >> new SD Card spec allows for 128 TB disks. Block sizes get sort of
 >> ridiculous as you get past about 8k bytes or about 32 TB or about two
 >> years from now.
 >>
 >> Do you want to create disks that will fail on AmigaDOS? AmigaDOS, as
 >> far as I know, makes heavy use of Cylinder Blocks values. It
 >> calculating Cylinder Blocks overflows when creating the disk's RDBs
 >> the user MUST be informed it is unsafe to put on a real Amiga. (I'd
 >
 > Joanne, if you are sure on this… I´d say at least warn if not bail out
 > on Cylinder Blocks overflow.

I am not sure how far into the rest of the system rdb_CylinderBlocks propagates. 
It MAY only affect repartitioning the disk. It may affect the "info" command. An 
audit is called for.

 > But given what you say here, no partitioning tool on AmigaOS or AmigaOS
 > like operating system would create such an overflow.
 >
 > Can you verify whether that is the case with the RDB that I attached to
 > the bug report?

No - frankly don't want to take the time to wander through it.

 > Bug 43511 - Partitions: Amiga RDB partition on 2 TB disk way too big,
 > while OK in AmigaOS 4.1

Did ALL the Amiga disk utilities (partitioning tools perhaps excepted) operate 
properly? If they did then the rdb_CylinderBlocks value might not propagate 
through the system.

 > https://bugzilla.kernel.org/show_bug.cgi?id=43511
 >
 > https://bugzilla.kernel.org/attachment.cgi?id=73771
 >
 >> also suggest teaching Linux to understand RDSL, which would be RDSK++
 >> sort of. Then use that if Cylinder Blocks overflows.) The value you
 >> will not be able to fill in the DosEnvec structure is: ULONG
 >> de_HighCyl;         /* max cylinder. drive specific */
 >>
 >>
 >> So accessing larger disks once you hit 2 TB means you must increase
 >> the logical block size. And eventually that will waste HUGE amounts
 >> of files when small files are being stored.
 >
 > As far as I am aware, AmigaOS 4.1 still only supports 512 byte sectors.

If that is the case I had several disks it would have barfed on, some of which 
had no option other than 2k * N block sizes as they were Fujitsu hard sector 
magneto-optical disks. This is something 4.1 MUST fix, IMAO. Is it based on 3.x 
code or is it a complete rewrite?

 >> Any solution will require action on the part of the people developing
 >> AmigaDOS follow-ons. You might want to get them motivated, somehow,
 >> and proceed from there with a request to be informed of any RDB
 >> changes. I'd suggest to them that removing sensitivity to Cylinder
 >> Blocks sorts of values from the entire system probably would be
 >> painful but the simplest solution.
 >
 > I think for this patch it is important to focus on the *current*
 > situation and make the best out of it.
 >
 > I am really inclined to point some AmigaOS 4 developers to this
 > discussion and just looked for an archive. Unfortunately there does not
 > appear to be a working one. The one mentioned on

If nothing else this discussion should prompt them to audit the OS code to see 
just what works and what does not. That will inform the Linux fix. I suggest the 
Linux fix go in and keep an open "potential" bug on the Amiga partitioning tool 
for Linux, presuming there is one. If there isn't, "What, ME worry?"

 > http://www.linux-m68k.org/mail.html
 >
 > http://aire.ncl.ac.uk/Atari/Mailing-Lists/Linux-680x0-vger-List.index.html
 >
 > does not send an answer within the HTTP / TCP timeout limit.
 >
 > I also did not find any archive for linux-block mailing list.
 >
 > And lore.kernel.org only seems to archive LKML itself which is patch and
 > the discussion we have here is not CC´d to.
 >
 > Any advice?

Get the AmigaDOS developers, all of 'em if there is a split with a group trying 
to recreate AmigaDOS open sores.

{^_^}

jdow June 30, 2018, 9:39 a.m. UTC | #49

As software is discovered to be "broken" at it to the appropriate incompatible list.

Otherwise permanently limit AmigaDOS to 2TB.
{^_^}

On 20180630 02:07, Martin Steigerwald wrote:
> jdow - 30.06.18, 08:47:
>> Let's get everybody:
>>
>> On 20180629 22:26, Michael Schmitz wrote:
>>   > Joanne,
>>   >
>>   > Am 30.06.18 um 15:56 schrieb jdow:
>>   >>>>> As far as I can guess from the code, pb_Environment[3] (number
>>   >>>>> of
>>   >>>>
>>   >>>> heads)
>>   >>>>
>>   >>>>> and pb_Environment[5] (number of sectors per cylinder) are
>>   >>>>> abitrarily
>>   >>>>> chosen so the partition size can be expressed as a difference
>>   >>>>> between
>>   >>>>> pb_Environment[9] and pb_Environment[10] (low and high cylinder
>>   >>>>> addresses), which places restrictions on both partition size
>>   >>>>> and
>>   >>>>> alignment that depend on where on the disk a partition is
>>   >>>>> placed?
>>   >>>>
>>   >>>> If you do not teach the OS to ignore Cylinder Blocks type
>>   >>>> entries and
>>   >>>> use some math on heads and blocks per track the disk size is
>>   >>>> relatively stuck modulo using large blocks.
>>   >>>
>>   >>> As long as AmigaOS and Linux agree on how to express start and
>>   >>> end
>>   >>> offset for the partitions, that's fine.
>>   >>>
>>   >>> But I read your other mail to mean that we're stuck to 2 TB disks
>>   >>> for
>>   >>> now. I don't follow that - we can have partitions of 2 TB each by
>>   >>
>>   >> maxing
>>   >>
>>   >>> out rdb_CylBlocks as long as we use 512 bytes per block (since
>>   >>> the
>>   >>> product of cylinders and blocks per cylinder is limited to 32
>>   >>> bits) and using one cylinder per partition (32 bits available
>>   >>> there as well)?
>>   >>>
>>   >>> But the rdb_CylBlocks limit also means we're safe with 64 bit
>>   >>> sector_t in Linux. Best add a check in the parser to warn us if
>>   >>> the product of head count and sectors per cylinder overflows 32
>>   >>> bit though.
>>   >>>
>>   >>> Cheers,
>>   >>>
>>   >>>        Michael
>>   >>
>>   >> How long did it tale s to get to 10 TB disks from 2 TB disks. And
>>   >> a
>>   >> new SD Card spec allows for 128 TB disks. Block sizes get sort of
>>   >> ridiculous as you get past about 8k bytes or about 32 TB or about
>>   >> two
>>   >> years from now.
>>   >
>>   > I get that - I just don't get why 32 bits for cylinders plus 32
>>   > bits for blocks per cylinder equals 2 TB (4G of 512 byte blocks).
>>   > But I don't know what other limits exist that may restrict the
>>   > total number of blocks to 32 bits.
>>
>> It overflows uint32_t cylinder blocks aka blocks per cylinder. Linux
>> doesn't care. AmigaDOS surely does. If YOU make partitions really
>> large for yourself that's OK. If Joe Amigoid does it the potential
>> for an angry red turning to purple face is high.
> 
> Ok, let get this straight:
> 
> Do you think that is the responsibility of the RDB parser within the
> Linux kernel to protect the user from anything whatever partitioning
> tool has created?
> 
> If so, how would you make sure the Linux kernel knows about whatever any
> partitioning tool used by Amiga users can come up with?
> 
> I´d say: Don´t bother. It is not the job of the RDB parser to impose
> limits on what partitioning tools can create. If native OS tools don´t
> create such thing, you don´t need to check for it. If someone managed to
> create it with amiga-fdisk or parted, the tool needs to be fixed. *Not*
> the kernel.
> 
> Anyway, that 2 TB disk that started all this *worked* on AmigaOS 4. And
> I am pretty sure while I cannot proof it, that even a larger disk would
> work. There is a limit for the boot partition on AmigaOS 4 Classic,
> which uses AmigaOS 3.1 to bootstrap AmigaOS 4 on Classic Amiga computers
> like an Amiga 1200 or Amiga 4000 with PowerPC extension card. But
> according to
> 
> Hard drive setup for AmigaOS 4.1 Classic
> http://blog.hyperion-entertainment.biz/?p=210
> 
> AmigaOS classic (i.e. < 4) would crash.
> 
>>   >> Do you want to create disks that will fail on AmigaDOS? AmigaDOS,
>>   >> as
>>   >> far as I know, makes heavy use of Cylinder Blocks values. It
>>   >> calculating Cylinder Blocks overflows when creating the disk's
>>   >> RDBs
>>   >> the user MUST be informed it is
>>   >
>>   > I'm not at all planning to create disks for AmigaDOS. I just need
>>   > to
>>   > know what combinations of cylinders, heads and sectors are possible
>>   > to encounter on disks that have been created with native tools.
>>   > Well, assuming sufficient amounts of braindamage in the
>>   > corresponding Linux tools, knowing the absolute outer limits of
>>   > what these tools could do would be nice as well, but someone using
>>   > amiga-fdisk to create a RDSK block for a 10 TB disk fully deserves
>>   > any punishment that invites.
>> Native AmigaDOS tools SHOULD NOT be able to create something that
>> overflows CylinderBlocks values. However, if it can that creates an
> 
> There you have it.
> 
> Then *why* bother, Joanne?
> 
>> interesting test case to see what various tools, like the AmigaDOS
>> "info" command, do when they they are run on such a disk. I don't
>> have OS source to perform searches. And I am not setup to feed the
>> system something obscene.
>>
>>   > (Actually, I lied there. I do plan to create a RDSK block for a 2
>>   > TB
>>   > disk image where cylinder, head and sector counts all approach the
>>   > 32
>>   > bit limit, just to see that my overflow checks work as intended.
>>   > But
>>   > that's strictly for Linux testing).
>>   >
>>   >> unsafe to put on a real Amiga. (I'd also suggest teaching Linux to
>>   >> understand RDSL, which would be RDSK++ sort of. Then use that if
>>   >> Cylinder Blocks overflows.) The value you will not be able to fill
>>   >> in
>>   >>
>>   >> the DosEnvec structure is:
>>   >>      ULONG de_HighCyl;         /* max cylinder. drive specific */
>>   >
>>   > OK, so Cylinder Blocks overflowing is a red flag, and requires to
>>   > abort parsing the partition table right away? And HighCyl really
>>   > means the max. number of logical blocks, not cylinders (which
>>   > would have nr_heads*nr_sects many blocks)? That's probably the
>>   > cause for my confusion.
>> I think I picked the wrong value. In RDSK itself this value is what
>> overflows: ULONG   rdb_CylBlocks;    /* number of blocks available
>> per cylinder */ And I think that floats around the system in many
>> places with different names. As mentioned the "info" command is one
>> item to test. If no crashes are found then AmigaDOS may be clean up
>> to obscene sizes. At the moment I do not remember what
>> hdwrench.library does with that value other than pass it along as
>> read. Nor am I sure what it generates as any suggested values. I
>> don't at this time have a disk I can mount as a disk on WinUAE that
>> is more than 2TB. And my Amigas speak SCSI so I have no disk for
>> them, either, even if they still boot.
>>   >> So accessing larger disks once you hit 2 TB means you must
>>   >> increase
>>   >> the logical block size. And eventually that will waste HUGE
>>   >> amounts of files when small files are being stored.
>>   >
>>   > Just like small inodes wastes huge amounts of space for metadata.
>>   > It's a tradeoff, and AFFS on a RDSK format disk probably isn't the
>>   > right choice for huge disks. Never mind that - if someone _does_
>>   > go that way, we need to make sure we can parse the RDSK
>>   > information correctly. And if such a disk causes the 64 bit
>>   > sector_t in Linux to overflow, I'd like the parser to spot that,
>>   > too.
>>   >
>>   > Thanks for your immense patience in explaining all these subtleties
>>   > to me.
> […]
>>   >      Michael
>>
>> And I'm rushing too much so I'm sorry I am making errors. This stuff
>> is 25 years in the past since I last looked at it seriously.
> 
> I think its important to focus on what can overflow can happen within
> calculations the RDB parser (and as a second step the AFFS file system)
> in the kernel kernel in order to keep this discussion to a manageable
> size. Be conservative about overflows, but otherwise accept. With a
> warning if a calculated exceed 32 bit.
> 
> As for values in the RDB. If its there, accept it. Some tool has written
> it there. We don´t know whether it did this right or wrong. We don´t
> know what the developer of the tool thought when writing it, well except
> for hdwrench.library I´d say as far as you remember. :) And it is not
> our job within the kernel to check that.
> 
> There is a ton of more or less legacy software out there on native OS
> which does something to or with RDBs. I´d say it is impossible to say
> what RDB a user may come up with.
> 
> Thanks,
>

Geert Uytterhoeven June 30, 2018, 9:10 p.m. UTC | #50

Hi Michael,

On Fri, Jun 29, 2018 at 11:12 AM Michael Schmitz <[e-mail]> wrote:
> Am 28.06.18 um 01:30 schrieb Geert Uytterhoeven:
> > On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
> >> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
> > ??
> >
> >> The Amiga RDB partition parser module uses int for partition sector
> >> address and count, which will overflow for disks 2 TB and larger.
> >>
> >> Use sector_t as type for sector address and size (as expected by
> >> put_partition) to allow using such disks without danger of data
> >> corruption.
> > Note that sector_t is not guaranteed to be 64-bit:
> >
> >     #ifdef CONFIG_LBDAF
> >     typedef u64 sector_t;
> >     typedef u64 blkcnt_t;
> >     #else
> >     typedef unsigned long sector_t;
> >     typedef unsigned long blkcnt_t;
> >     #endif
> >
> > And it seems CONFIG_LBDAF can still be disabled on 32-bit...
> >
>
> What are the ramifications of using a 2 TB disk on a kernel without
> CONFIG_LBDAF? Are there any safeguards after the partition scan stage
> that would prevent the kernel from using partitions on such a disk?
>
> Trying to decide whether we absolutely have to bail if sector_t is 32
> bit ...

Sorry, I don't know.

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven June 30, 2018, 9:21 p.m. UTC | #51

Hi Andreas,

On Fri, Jun 29, 2018 at 3:26 PM Andreas Schwab <[e-mail]> wrote:
> On Jun 29 2018, Michael Schmitz <[e-mail]> wrote:
> > Would MSDOS recognize the GPT partition as 'probably FAT', and attempt
> > to use it?
>
> GPT has the concept of a protective MBR which should prevent such errors.

Thanks, good to know.

So it looks like GPT and RDSK can coexist on the same disk: an RDSK header
in block 0 (pointing to PART blocks located after the GPT), and GPT in
blocks 1-33.

That means for very large disks, you can have an RDSK describing the
partitions that fit in the first part of the disk that can be accessed by
e.g. AmigaOS 3.0, and a GPT that describes all partitions on the disk,
right?

Gr{oetje,eeting}s,

                        Geert

Michael Schmitz June 30, 2018, 9:26 p.m. UTC | #52

Hi Geert,

Am 01.07.2018 um 09:10 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Fri, Jun 29, 2018 at 11:12 AM Michael Schmitz <[e-mail]> wrote:
>> Am 28.06.18 um 01:30 schrieb Geert Uytterhoeven:
>>> On Wed, Jun 27, 2018 at 4:47 AM <[e-mail]> wrote:
>>>> From 5299e0e64dfb33ac3a1f3137b42178734ce20087 Mon Sep 17 00:00:00 2001
>>> ??
>>>
>>>> The Amiga RDB partition parser module uses int for partition sector
>>>> address and count, which will overflow for disks 2 TB and larger.
>>>>
>>>> Use sector_t as type for sector address and size (as expected by
>>>> put_partition) to allow using such disks without danger of data
>>>> corruption.
>>> Note that sector_t is not guaranteed to be 64-bit:
>>>
>>>     #ifdef CONFIG_LBDAF
>>>     typedef u64 sector_t;
>>>     typedef u64 blkcnt_t;
>>>     #else
>>>     typedef unsigned long sector_t;
>>>     typedef unsigned long blkcnt_t;
>>>     #endif
>>>
>>> And it seems CONFIG_LBDAF can still be disabled on 32-bit...
>>>
>>
>> What are the ramifications of using a 2 TB disk on a kernel without
>> CONFIG_LBDAF? Are there any safeguards after the partition scan stage
>> that would prevent the kernel from using partitions on such a disk?
>>
>> Trying to decide whether we absolutely have to bail if sector_t is 32
>> bit ...
>
> Sorry, I don't know.

Not to worry - I'll find out one way or another. I've added some more 
checks to spot both 32 bit and 64 bit overflows following what we 
discussed in the past days (though the latter really shouldn't happen). 
I'm quite sure that will throw everyone on linux-block in a laughing 
fit, but I'm sure someone will point out a better way of doing this 
during review (or prove it won't be necessary).

Pending final test, I'll submit the result today or tomorrow.

Cheers,

	Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Michael Schmitz July 1, 2018, 2:43 a.m. UTC | #53

Martin,

Am 30.06.18 um 19:49 schrieb Martin Steigerwald:
> I am really inclined to point some AmigaOS 4 developers to this
> discussion and just looked for an archive. Unfortunately there does not 
> appear to be a working one. The one mentioned on
>
> http://www.linux-m68k.org/mail.html
>
> http://aire.ncl.ac.uk/Atari/Mailing-Lists/Linux-680x0-vger-List.index.html
>
> does not send an answer within the HTTP / TCP timeout limit.

Most of the information found online on m68k will be several years out
of date, I'd expect. The page you appear to have found will be several
decades out of date (any mention of the FTP server at Uni Saarbruecken
for list archives is a dead giveaway). The internet doesn't forget, but
that can be a curse rather than a blessing at times.

Anyway, vger sez:

https://marc.info/?l=linux-m68k&r=1&w=2
and
https://www.spinics.net/lists/linux-m68k/

which work for me.
>
> I also did not find any archive for linux-block mailing list

https://marc.info/?l=linux-block works for me.

> .
>
> And lore.kernel.org only seems to archive LKML itself which is patch and 
> the discussion we have here is not CC´d to.

And I'm glad we didn't.

Cheers,

    Michael

>
> Any advice?
>
> Thanks,

jdow July 1, 2018, 4:36 a.m. UTC | #54

FWIW on the other side this appears to be a good source of Amiga software data.

http://amigadev.elowar.com/
{^_^}

On 20180630 19:43, Michael Schmitz wrote:
> Martin,
> 
> Am 30.06.18 um 19:49 schrieb Martin Steigerwald:
>> I am really inclined to point some AmigaOS 4 developers to this
>> discussion and just looked for an archive. Unfortunately there does not
>> appear to be a working one. The one mentioned on
>>
>> http://www.linux-m68k.org/mail.html
>>
>> http://aire.ncl.ac.uk/Atari/Mailing-Lists/Linux-680x0-vger-List.index.html
>>
>> does not send an answer within the HTTP / TCP timeout limit.
> 
> Most of the information found online on m68k will be several years out
> of date, I'd expect. The page you appear to have found will be several
> decades out of date (any mention of the FTP server at Uni Saarbruecken
> for list archives is a dead giveaway). The internet doesn't forget, but
> that can be a curse rather than a blessing at times.
> 
> Anyway, vger sez:
> 
> https://marc.info/?l=linux-m68k&r=1&w=2
> and
> https://www.spinics.net/lists/linux-m68k/
> 
> which work for me.
>>
>> I also did not find any archive for linux-block mailing list
> 
> https://marc.info/?l=linux-block works for me.
> 
>> .
>>
>> And lore.kernel.org only seems to archive LKML itself which is patch and
>> the discussion we have here is not CC´d to.
> 
> And I'm glad we didn't.
> 
> Cheers,
> 
>      Michael
> 
>>
>> Any advice?
>>
>> Thanks,
>

Martin Steigerwald July 1, 2018, 12:26 p.m. UTC | #55

Michael.

Michael Schmitz - 01.07.18, 04:43:
> Am 30.06.18 um 19:49 schrieb Martin Steigerwald:
> > I am really inclined to point some AmigaOS 4 developers to this
> > discussion and just looked for an archive. Unfortunately there does
> > not appear to be a working one. The one mentioned on
> > 
> > http://www.linux-m68k.org/mail.html
> > 
> > http://aire.ncl.ac.uk/Atari/Mailing-Lists/Linux-680x0-vger-List.inde
> > x.html
> > 
> > does not send an answer within the HTTP / TCP timeout limit.
> 
> Most of the information found online on m68k will be several years out
> of date, I'd expect. The page you appear to have found will be
> several decades out of date (any mention of the FTP server at Uni
> Saarbruecken for list archives is a dead giveaway). The internet
> doesn't forget, but that can be a curse rather than a blessing at
> times.
> 
> Anyway, vger sez:
> 
> https://marc.info/?l=linux-m68k&r=1&w=2
> and
> https://www.spinics.net/lists/linux-m68k/
> 
> which work for me.
> 
> > I also did not find any archive for linux-block mailing list
> 
> https://marc.info/?l=linux-block works for me.

Ah, I thought marc.info was down, but well now I remember that someone 
took over or something like that.

Did not think of spinics.net.

Well I wrote a mail to some AmigaOS 4 developers. Whether I get a 
response remains to be seen. However, in any case I´d continue with 
developing the patch. I´d I not make the Linux fix dependent on this.

If they share something with me to forward it regarding the Linux patch, 
I´d share that with you.

> > And lore.kernel.org only seems to archive LKML itself which is patch
> > and the discussion we have here is not CC´d to.
> 
> And I'm glad we didn't.

:)

Thanks,

Patch

diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 5609366..42c3f38 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -32,7 +32,8 @@  int amiga_partition(struct parsed_partitions *state)
 	unsigned char *data;
 	struct RigidDiskBlock *rdb;
 	struct PartitionBlock *pb;
-	int start_sect, nr_sects, blk, part, res = 0;
+	sector_t start_sect, nr_sects;
+	int blk, part, res = 0;
 	int blksize = 1;	/* Multiplier for disk block size */
 	int slot = 1;
 	char b[BDEVNAME_SIZE];
@@ -111,6 +112,16 @@  int amiga_partition(struct parsed_partitions *state)
 			     be32_to_cpu(pb->pb_Environment[3]) *
 			     be32_to_cpu(pb->pb_Environment[5]) *
 			     blksize;
+		if (start_sect > INT_MAX || nr_sects > INT_MAX
+			|| (start_sect + nr_sects) > INT_MAX) {
+			pr_err("%s: Warning: RDB partition overflow!\n",
+				bdevname(state->bdev, b));
+			pr_err("%s: start 0x%llX size 0x%llX\n",
+				bdevname(state->bdev, b), start_sect,
+				nr_sects);
+			pr_err("%s: partition incompatible with 32 bit OS\n",
+				bdevname(state->bdev, b));
+		}
 		put_partition(state,slot++,start_sect,nr_sects);
 		{
 			/* Be even more informative to aid mounting */