[lkml]   [2011]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] kvm: Fix build warnings

    * Borislav Petkov <> 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;

    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;

    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;

    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

    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),

    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

    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 >>
    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 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;
    } 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.



     \ /
      Last update: 2011-05-31 09:41    [W:0.032 / U:37.016 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site