Re: [PATCH v2] x86: Ensure input to pfn_to_kaddr() is treated as a 64-bit type

[H. Peter Anvin]

On November 15, 2023 5:42:31 PM EST, Michael Roth <michael.roth@xxxxxxx> wrote:
>On Wed, Nov 15, 2023 at 12:48:58PM -0800, Dave Hansen wrote:
>> On 11/15/23 12:14, Michael Roth wrote:
>> > While it might be argued that the issue is on the caller side, other
>> > archs/macros have taken similar approaches to deal with instances like
>> > this, such as commit e48866647b48 ("ARM: 8396/1: use phys_addr_t in
>> > pfn_to_kaddr()").
>> 
>> Gah, I really hope nobody is arguing that for real, or is even thinking
>> about this as a valid argument.
>
>Not that I'm aware, but I did have my own doubts initially, which is
>why I thought it warranted a note in the commit just in case it came up
>from someone else.
>
>> 
>> The helper should, well, help the caller.  It makes zero sense to me
>> that every single call site would need to know if the argument's type
>> was big enough to hold the _return_ value.  This nonsense can only even
>> happen with macros.  Type promotion would just do the right thing for
>> any sanely declared actual helper function.
>
>My thought was that it is easier to expect developers to know the pitfalls
>of bit-field types, since it is universally applicable to all C code,
>whereas expecting developers to anticipate such issues when writing similar
>macros is potentially harder to enforce/audit and could lead to similar
>issues popping up as things are refactored over time and new macros get
>added that don't take such usages into account.
>
>But neither argument seems to hold up in reality. Experienced developers
>obviously do fall victim to the subtleties of of bit-field types, and
>kernel devs obviously do tend to address these instances in more robust
>ways based on the various pfn-related macros I looked through.
>
>-Mike

Now, if you are doing a cast, you are making the macro unusable for assembly anyway; any reason not to make it an inline function at that point?