On 18.10.2023 12:07, Federico Serafini wrote:
> On 16/10/23 17:26, Jan Beulich wrote:
>> On 03.10.2023 17:24, Federico Serafini wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s, unsigned
>>> long e)
>>> * a problem.
>>> */
>>> void init_or_livepatch modify_xen_mappings_lite(
>>> - unsigned long s, unsigned long e, unsigned int _nf)
>>> + unsigned long s, unsigned long e, unsigned int nf)
>>> {
>>> - unsigned long v = s, fm, nf;
>>> + unsigned long v = s, fm, flags;
>>
>> While it looks correct, I consider this an unacceptably dangerous
>> change: What if by the time this is to be committed some new use of
>> the local "nf" appears, without resulting in fuzz while applying the
>> patch? Imo this needs doing in two steps: First nf -> flags, then
>> _nf -> nf.
>>
>> Furthermore since you alter the local variable, is there any reason
>> not to also change it to be "unsigned int", matching the function
>> argument it's set from?
>
> If you are referring to the local variable 'flags', it is set using the
> value returned from put_pte_flags() which is an intpte_t (typedef for
> u64). Am I missing something?
Oh, I'm sorry, I didn't look there carefully enough. Which means using
_nf and nf here was probably not a good idea in the first place, when
both are of different type and nature.
Jan
