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

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

On 14/06/23 12:07, Finn Thain wrote:
> On Wed, 14 Jun 2023, Michael Schmitz wrote:
>
>> 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)?
>>
>> 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...
>>
> The new warning comes from this new code:
>
> 		if (cylblk > be32_to_cpu((__be32)rdb->rdb_CylBlocks)) {
> 			pr_warn("Dev %s: cylblk %u > rdb_CylBlocks %u!\n",
> 				state->disk->disk_name, cylblk,
> 				be32_to_cpu(rdb->rdb_CylBlocks));
> 		}
>
> The __be32 cast appears in the first line but not the fourth, which is an
> inconsistency you might want to tidy up. However, both lines produce the
> same sparse warning here.

Thanks for checking that - the cast is redundant as-is (be32_to_cpu() 
contains the same cast already). Does use of (__force __be32) instead 
make the warning go away? (I haven't managed to get sparse working for 
me, so I have no way of checking.)

> The inconsistent use of big-endian and native-endian members in struct
> RigidDiskBlock looks like the root cause to me but I know nothing about
> Amiga partition maps so I'm not going to guess.

The check appeared in the first version of the patch, after discussion 
around the RFC version at length. Going over that thread again, I 
haven't found why that check was added. It's probably been out of an 
overabundance of caution (as I know little about RDB, too) and can 
probably be removed.

Cheers,

     Michael