Re: [PATCH v7 2/2] block: add overflow checks for Amiga partition support

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Martin,

Am 14.06.2023 um 19:19 schrieb Martin Steigerwald:
> Hi Michael, hi Joanne.
>
> @Joanne: I am cc'ing you in this patch as I bet you might be able to
> confirm whether the rdb_CylBlocks value in Amiga RDB is big endian. I
> hope you do not mind. I would assume that everything in Amiga RDB is big
> endian, but I bet you know for certain.
>
> Michael Schmitz - 14.06.23, 00:11:45 CEST:
>> On 13/06/23 22:57, Martin Steigerwald wrote:
>>> Michael Schmitz - 13.06.23, 10:18:24 CEST:
>>>> Am 13.06.2023 um 19:25 schrieb Martin Steigerwald:
>>>>> Hi Michael, hi Jens, Hi Geert.
>>>>>
>>>>> Michael Schmitz - 22.08.22, 22:56:10 CEST:
>>>>>> On 23/08/22 08:41, Jens Axboe wrote:
>>>>>>> On 8/22/22 2:39 PM, Michael Schmitz wrote:
>>>>>>>> Hi Jens,
>>>>>>>>
>>>>>>>> will do - just waiting to hear back what needs to be done
>>>>>>>> regarding
>>>>>>>> backporting issues raised by Geert.
>>>>>>>
>>>>>>> It needs to go upstream first before it can go to stable. Just
>>>>>>> mark
>>>>>>> it with the right Fixes tags and that will happen automatically.
>>>>>
>>>>> […]
>>>>>
>>>>>> thanks - the Fixes tag in my patches refers to Martin's bug
>>>>>> report
>>>>>> and won't be useful to decide how far back this must be applied.
>>>>>>
>>>>>> Now the bug pre-dates git, making the commit to 'fix'
>>>>>> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ... That one's a bit
>>>>>> special, please yell if you want me to lie about this and use a
>>>>>> later commit specific to the partition parser code.
>>>>>
>>>>> After this discussion happened I thought the patch went in.
>>>>> However…
>>>>> as John Paul Adrian asked in "Status of affs support in the kernel
>>>>> and affstools" thread on linux-m68k and debian-68k mailing list, I
>>>>> searched for the patch in git history but did not find it.
>>>>
>>>> I may have messed that one up, as it turns out. Last version was v9
>>>> which I had to resend twice, and depending on what Jens uses to
>>>> keep
>>>> track of patches, the resends may not have shown up in his tool. I
>>>> should have bumped the version number instead.
>>>>
>>>> I'll see if my latest version still applies cleanly ...
>>>
>>> Many thanks!
>>>
>>> Would be nice to see it finally go in.
>>
>> My last version (v9) still applies, but that one still threw a sparse
>> warning for patch 2:
>>
>> Link:https://lore.kernel.org/all/202208231319.Ng5RTzzg-lkp@intel.com
>>
>> Not sure how to treat that one - rdb_CylBlocks is not declared as big
>> endian so the warning is correct, but as far as I can see, for all
>> practical purposes rdb_CylBlocks would be expected to be in big endian
>> order (partition usually prepared on a big endian system)?
>
> While I did not specifically check myself I would be highly surprised in
> case anything in RDB data structure would be little endian. Amiga is a
> big endian platform. I'd expect little endian only to be used where
> there was need to interface with little endian platforms – like in PC
> emulators.

That's what I thought - mind you, rdb_CylBlocks is not declared little 
endian, just __u32 which can be either.

The reason why only rdb_SummedLongs, rdb_BlockBytes and 
rdb_PartitionList are explicitly declared big endian is probably quite 
simple - nothing else was used by the Linux RDB parser. My patch adds 
rdb_CylBlocks to that list, so that ought to be changed to big endian, too.

affs_hardblocks.h is a UAPI header - what are the rules and 
ramifications around changes to those? Might not be worth the hassle in 
the end.

> It may not be easy to find any confirmation in developer documentation,
> I'd assume that wherever it would not have been stated differently, big
> endian is assumed for AmigaOS.
>
>> I can drop the be32_to_cpu conversion (and would expect to see a
>> warning printed on little endian systems), or force the cast to
>> __be32. Or rather drop that consistency check outright...
>
> So "be32_to_cpu" converts big to little endian in case the CPU
> architecture Linux runs on is little endian?

Correct - in order to use RDB partitions on little endian systems, all 
of the data used by the Linux kernel must be converted to host byte 
order before using them in calculations.

>
> Well again… I'd say it is safe to assume that anything in Amiga RDB is
> big endian.

Let's do that then. Unless someone feels strongly otherwise, I'll drop 
the rdb_CylBlocks check.

Cheers,

	Michael

>
> Best,
>