I get the following
In file included from arch/x86/kvm/mmu.c:2856:
arch/x86/kvm/paging_tmpl.h: In function ‘paging32_walk_addr_generic’:
arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
In file included from arch/x86/kvm/mmu.c:2852:
arch/x86/kvm/paging_tmpl.h: In function ‘paging64_walk_addr_generic’:
arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
when building -rc1. It looks like it is caused by
6e2ca7d1802bf8ed9908435e34daa116662e7790 and sticking uninitialized_var() around
the ptep_user declaration looks like the easiest solution. But the code should
still be audited by someone who's familiar with it whether shutting up the
compiler doesn't cause an actual bug.
Thanks.
--
Regards/Gruss,
Boris.
On Mon, 30 May 2011 11:46:04 +0200
Borislav Petkov <[email protected]> wrote:
> I get the following
>
> In file included from arch/x86/kvm/mmu.c:2856:
> arch/x86/kvm/paging_tmpl.h: In function $B!F(Bpaging32_walk_addr_generic$B!G(B:
> arch/x86/kvm/paging_tmpl.h:124: warning: $B!F(Bptep_user$B!G(B may be used uninitialized in this function
> In file included from arch/x86/kvm/mmu.c:2852:
> arch/x86/kvm/paging_tmpl.h: In function $B!F(Bpaging64_walk_addr_generic$B!G(B:
> arch/x86/kvm/paging_tmpl.h:124: warning: $B!F(Bptep_user$B!G(B may be used uninitialized in this function
>
> when building -rc1. It looks like it is caused by
> 6e2ca7d1802bf8ed9908435e34daa116662e7790 and sticking uninitialized_var() around
> the ptep_user declaration looks like the easiest solution. But the code should
> still be audited by someone who's familiar with it whether shutting up the
> compiler doesn't cause an actual bug.
Sorry, it is my commit.
I think the logic guarantees that ptep_user won't be used until it is
assigned some value.
It seems to be safe, IIUC.
Takuya
On Mon, May 30, 2011 at 07:14:26PM +0900, Takuya Yoshikawa wrote:
> On Mon, 30 May 2011 11:46:04 +0200
> Borislav Petkov <[email protected]> wrote:
>
> > I get the following
> >
> > In file included from arch/x86/kvm/mmu.c:2856:
> > arch/x86/kvm/paging_tmpl.h: In function ‘paging32_walk_addr_generic’:
> > arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
> > In file included from arch/x86/kvm/mmu.c:2852:
> > arch/x86/kvm/paging_tmpl.h: In function ‘paging64_walk_addr_generic’:
> > arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
> >
> > when building -rc1. It looks like it is caused by
> > 6e2ca7d1802bf8ed9908435e34daa116662e7790 and sticking uninitialized_var() around
> > the ptep_user declaration looks like the easiest solution. But the code should
> > still be audited by someone who's familiar with it whether shutting up the
> > compiler doesn't cause an actual bug.
>
> Sorry, it is my commit.
>
> I think the logic guarantees that ptep_user won't be used until it is
> assigned some value.
>
> It seems to be safe, IIUC.
Ok, thanks for confirming. I'll send a fix soon if no one beats me to
it.
--
Regards/Gruss,
Boris.
On 3.0-rc1 I get
In file included from arch/x86/kvm/mmu.c:2856:
arch/x86/kvm/paging_tmpl.h: In function ‘paging32_walk_addr_generic’:
arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
In file included from arch/x86/kvm/mmu.c:2852:
arch/x86/kvm/paging_tmpl.h: In function ‘paging64_walk_addr_generic’:
arch/x86/kvm/paging_tmpl.h:124: warning: ‘ptep_user’ may be used uninitialized in this function
caused by 6e2ca7d1802bf8ed9908435e34daa116662e7790. According to Takuya
Yoshikawa, ptep_user won't be used uninitialized so shut up gcc.
Cc: Takuya Yoshikawa <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kvm/paging_tmpl.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 6c4dc01..9d03ad4 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ 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);
gfn_t table_gfn;
unsigned index, pt_access, uninitialized_var(pte_access);
gpa_t pte_gpa;
--
1.7.5.rc1.16.g9db1
* Borislav Petkov <[email protected]> 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
On Tue, 31 May 2011 09:38:24 +0200
Ingo Molnar <[email protected]> wrote:
>
> * Borislav Petkov <[email protected]> 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.
Thank you for your advice.
Borislav, would you like to do the fix suggested here?
As the person who introduced this warning, if these are too many
for you, I will try some of these.
Takuya
On 05/31/2011 10:38 AM, Ingo Molnar wrote:
> * Borislav Petkov<[email protected]> 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.
>
>
<snip lots of good advice>
> Please fix it instead.
s/instead/in addition/; while all those changes are good, they are much
too large for 3.0. Let's push the simple fix for 3.0 and queue the
bigger refactoring to 3.1.
--
error compiling committee.c: too many arguments to function
On Tue, May 31, 2011 at 11:20:55AM +0300, Avi Kivity wrote:
> On 05/31/2011 10:38 AM, Ingo Molnar wrote:
> >* Borislav Petkov<[email protected]> 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.
> >
> >
>
> <snip lots of good advice>
>
> >Please fix it instead.
>
> s/instead/in addition/; while all those changes are good, they are
> much too large for 3.0. Let's push the simple fix for 3.0 and queue
> the bigger refactoring to 3.1.
Just to clarify: Hell, it is _not_ I who's fixing this! Virtualization
folks are crazy anyway and I'm not touching their code except for
trivial fixes :-).
The story: I saw the humongous function and being lazier than Ingo, I
just wanted to shut up the warning. Knowing that uninitialized_var()
is a dangerous thing to use, I asked whether people who know the code
can guarantee that ptep_user is not going to be used uninitialized and
Takuya confirmed.
But yes, it'll be much better if kvm people could take a good hard look
at it and try to simplify it. Also, while they're at it, I'd suggest
they actually trace whether that unlikely() annotation actually brings
any performance speedup - if it doesn't, out the door with it and here's
more simplification right there.
Thanks.
--
Regards/Gruss,
Boris.
* Avi Kivity <[email protected]> wrote:
> On 05/31/2011 10:38 AM, Ingo Molnar wrote:
> >* Borislav Petkov<[email protected]> 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.
> >
> >
>
> <snip lots of good advice>
>
> > Please fix it instead.
>
> s/instead/in addition/; while all those changes are good, they are
> much too large for 3.0. Let's push the simple fix for 3.0 and
> queue the bigger refactoring to 3.1.
Yeah, that's probably wise, this is a tricky function.
Thanks,
Ingo
On Tue, May 31, 2011 at 12:26:55PM +0200, Ingo Molnar wrote:
>
> * Avi Kivity <[email protected]> wrote:
>
> > On 05/31/2011 10:38 AM, Ingo Molnar wrote:
> > >* Borislav Petkov<[email protected]> 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.
> > >
> > >
> >
> > <snip lots of good advice>
> >
> > > Please fix it instead.
> >
> > s/instead/in addition/; while all those changes are good, they are
> > much too large for 3.0. Let's push the simple fix for 3.0 and
> > queue the bigger refactoring to 3.1.
>
> Yeah, that's probably wise, this is a tricky function.
So, any progress on this front? Warning is still there in -rc2.
Thanks.
--
Regards/Gruss,
Boris.
On 06/07/2011 10:28 AM, Borislav Petkov wrote:
> So, any progress on this front? Warning is still there in -rc2.
>
Thanks for the reminder, applied and queued.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.