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?
Yet then - can't we just delete "nf" and rename "_nf" to "nf"? The
function parameter is only used in
nf = put_pte_flags(_nf & FLAGS_MASK);
Jan
I thought about it but it will introduce a violation of Rule 17.8:
"A function parameter should not be modified".
It is an advisory rule that is not currently accepted but one day
it may be.
--
Federico Serafini, M.Sc.
Software Engineer, BUGSENG (http://bugseng.com)
