Re: [PATCH] kvm: Fix build warnings

Ingo Molnar
2011-05-31T07:38:24Z

* Borislav Petkov <bp@alien8.de> wrote:

> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -121,7 +121,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
> gva_t addr, u32 access)
> {
> pt_element_t pte;
> - pt_element_t __user *ptep_user;
> + pt_element_t __user *uninitialized_var(ptep_user);

Note that doing this is actually actively dangerous for two reasons.

Firstly, it also shuts down the warning when it turns into a *real*
warning. For example this function will not produce a warning:

int test(int a)
{
int uninitialized_var(b);

return b;
}

Secondly, if the *compiler* cannot understand the flow then the code
is obviously rather complex for humans to review. So if there's an
initialization bug in the future, the risk of a human not seeing it
and the risk of uninitialized_var() hiding it is larger.

So the recommended thing is to simplify the flow there to make it
easier for the compiler to see through it.

A quick look suggests that walk_addr_generic() is *horrible*: it has
amassed a large number of classic code structure mistakes, and while
it's clearly a performance critical function, needless code ugliness
often goes at the *expense* of good performance.

I found a handful of problems during a quick review of it:

- There's ugly repeated patterns of:

if (unlikely(condition)) {
present = false;
break;
}

which is then handled outside the main loop with:

if (unlikely(!present || ...))
goto error;

It would be a lot cleaner, not to mention faster as well to do
this via:

if (condition)
goto error_not_present;

That way the 'present' bool does not clog up the code flow (and
register allocations).

- rsvd_fault shows similar mismanagement:

if (unlikely(condition)) {
rsvd_fault = true;
break;
}

if (!eperm && !rsvd_fault && ...) {
...
}
}

if (unlikely(!present || eperm || rsvd_fault))
goto error;

This obfuscation complicated (and potentially slowed down) the
middle condition: it's rather clear that the code flow cannot get
there with rsvd == true ...

It should be done via a more natural:

if (condition)
goto error_rsvd_fault;

- eperm setting:

if (unlikely(write_fault && !is_writable_pte(pte)
&& (user_fault || is_write_protection(vcpu))))
eperm = true;

if (unlikely(user_fault && !(pte & PT_USER_MASK)))
eperm = true;

#if PTTYPE == 64
if (unlikely(fetch_fault && (pte & PT64_NX_MASK)))
eperm = true;
#endif

is idempotent so is an obvious candidate to be factored out into a
helper inline. If you already know how eperm is calculated why
should a code reader be forced to go through those lines again and
again, every time this function is reviewed?

- In fact, once the unnecessary rsvd_fault complication has been
factored out, the heart of the function, marking the pte
accessed/dirty connects very nicely to the eperm calculating
inline:

eperm = gpte_eperm(vcpu, pte, access);

[ NOTE: we should probably pass in 'access' explicitly because for
code generation it's better to keep such variables in a single
register and check it via the obvious bitmask and TESTL, not via
the separate write_fault, user_fault, fetch_fault variables. ]

- The 'access' attribute seems somewhat mismanaged as well. There
are unnecessary seeming complexities like:

write_fault = access & PFERR_WRITE_MASK;
user_fault = access & PFERR_USER_MASK;
fetch_fault = access & PFERR_FETCH_MASK;

ac = write_fault | fetch_fault | user_fault;

real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(gfn),
ac);

So ... we first split the 'access' attribute into 3 separate
bools, then we *combine* them again and pass the result to
translate_gpa()? Will the compiler figure out that this is equivalent
to access & ~(PFERR_WRITE_MASK|PFERR_USER_MASK|PFERR_FETCH_MASK)?

Even if it does, wouldnt it be safe to pass 'access' to ->translate_gpa()
as-is? If it's not safe to pass it as-is then a comment would be handy
about this non-obvious looking fact.

- Variables are not marked 'const' where they should be - the above
*_fault attributes for example but there are other examples as
well. Since GCC very obviously has trouble seeing through this
monster of a function, not helping it out with 'const' can hurt
code generation quality. Reviewers are also helped: i had to spend
a minute figuring out that none of these are ever modified within
the function.

- What the heck is up with ASSERT() usage in the Linux kernel?
arch/x86/kvm/ uses about 50% of BUG_ON()s and 50% of inverted
logic ASSERT()s. If the goal was to confuse the reviewer then it's
a full success! :-)

- Litte details like:

if (unlikely(kvm_is_error_hva(host_addr))) {

The name already suggests that kvm_is_error_hva() is a rare
exception mechanism. The unlikely() could be propagated *into*
kvm_is_error_hva() and thus call sites would be less cluttered.

- Data type choicese are sometimes unnatural and lead to unnecessary casts.
For example:

unsigned long host_addr;

host_addr = gfn_to_hva(vcpu->kvm, real_gfn);
if (unlikely(kvm_is_error_hva(host_addr))) {

ptep_user = (pt_element_t __user *)((void *)host_addr + offset);

It's a host virtual address, so eventual usage ends up being a
void * variant. Other usages of kvm_is_error_hva() show
similar patterns:

unsigned long addr;
addr = gfn_to_hva(vcpu->kvm, data >>
HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
if (kvm_is_error_hva(addr))
return 1;
if (clear_user((void __user *)addr, PAGE_SIZE))
return 1;

So if this type was changed to void __user *host_addr, and
gfn_to_hva() and kvm_is_error_hva() was changed to operate on void *
then the code would look much cleaner:

void __user *host_addr;

host_addr = gfn_to_hva(vcpu->kvm, real_gfn);
if (kvm_is_error_hva(host_addr)) {

ptep_user = host_addr + offset;

And note that we also lost a fragile type cast.

- Please factor out horrible conditions like:

if ((walker->level == PT_PAGE_TABLE_LEVEL) ||
((walker->level == PT_DIRECTORY_LEVEL) &&
is_large_pte(pte) &&
(PTTYPE == 64 || is_pse(vcpu))) ||
((walker->level == PT_PDPE_LEVEL) &&
is_large_pte(pte) &&
mmu->root_level == PT64_ROOT_LEVEL)) {

into helper inlines as well, with descriptive names.

- Code like this:

if (PTTYPE == 32 &&
walker->level == PT_DIRECTORY_LEVEL &&
is_cpuid_PSE36())

is clearly hurting from too deep indentation caused by over-inlining.

- Label names like 'walk:' are actively misleading. Of course it
'walks', but that's not the main function of the label: the main
function is that it *retries* a page table walk.

So 'retry_walk:' would be a lot more informative and would make
code like this:

ret = FNAME(cmpxchg_gpte)(vcpu, mmu, ptep_user, index,
pte, pte|PT_ACCESSED_MASK);
if (unlikely(ret < 0)) {
present = false;
break;
} else if (ret)
goto retry_walk;

a lot more clearer as well. Small details like this add up.

- I'd suggest splitting the iterator of the loop out into a helper inline
and only leave the loop / retry and error logic in walk_addr_generic().
Maybe even factor out the initialization and error logic - only leaving
the main retry logic in walk_addr_generic() itself.

All in one, having spent a few minutes with this code i am not
surprised *at all* that it has grown its *second* dangerous
uninitialized_var() annotation ...

Please fix it instead.

Thanks,

Ingo