On Tue, Feb 18, 2020 at 01:34:33PM -0800, Oliver Upton wrote:
> Hey Sean,
>
> On Tue, Feb 18, 2020 at 11:07:29AM -0800, Sean Christopherson wrote:
> > On Tue, Feb 18, 2020 at 10:47:56AM -0800, Oliver Upton wrote:
> > > Particularly draconian compilers warn of a possible uninitialized use of
> > > the nr_pages_avail variable. Silence this warning by initializing it to
> > > zero.
> >
> > Can you check if the warning still exists with commit 6ad1e29fe0ab ("KVM:
> > Clean up __kvm_gfn_to_hva_cache_init() and its callers")? I'm guessing
> > (hoping?) the suppression is no longer necessary.
>
> Hmm. I rebased this patch right before sending out + it seems that it is
> required (at least for me) to silence the compiler warning. For good
> measure, I ran git branch --contains to ensure I had your change. Looks
> like my topic branch did in fact have your fix.
Bummer. By "particularly draconian compilers", do you mean "clang"? Not
that it really matters, but it'd explain why no one else is complaining.
Anyways, comment on the code below...
> --
> Oliver
>
> > commit 6ad1e29fe0aba843dfffc714fced0ef6a2e19502
> > Author: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > Date: Thu Jan 9 14:58:55 2020 -0500
> >
> > KVM: Clean up __kvm_gfn_to_hva_cache_init() and its callers
> >
> > Barret reported a (technically benign) bug where nr_pages_avail can be
> > accessed without being initialized if gfn_to_hva_many() fails.
> >
> > virt/kvm/kvm_main.c:2193:13: warning: 'nr_pages_avail' may be
> > used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > Rather than simply squashing the warning by initializing nr_pages_avail,
> > fix the underlying issues by reworking __kvm_gfn_to_hva_cache_init() to
> > return immediately instead of continuing on. Now that all callers check
> > the result and/or bail immediately on a bad hva, there's no need to
> > explicitly nullify the memslot on error.
> >
> > Reported-by: Barret Rhoden <brho@xxxxxxxxxx>
> > Fixes: f1b9dd5eb86c ("kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init")
> > Cc: Jim Mattson <jmattson@xxxxxxxxxx>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> >
> >
> > > Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
> > > ---
> > > virt/kvm/kvm_main.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 70f03ce0e5c1..dc8a67ad082d 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -2219,7 +2219,7 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
> > > gfn_t start_gfn = gpa >> PAGE_SHIFT;
> > > gfn_t end_gfn = (gpa + len - 1) >> PAGE_SHIFT;
> > > gfn_t nr_pages_needed = end_gfn - start_gfn + 1;
> > > - gfn_t nr_pages_avail;
> > > + gfn_t nr_pages_avail = 0;
Since the warning is technically bogus, uninitialized_var() can be used,
which will hopefully make it more obvious (well, less misleading), that the
loop's post-condition isn't broken.
gfn uninitialized_var(nr_pages_avail);
> > >
> > > /* Update ghc->generation before performing any error checks. */
> > > ghc->generation = slots->generation;
> > > --
> > > 2.25.0.265.gbab2e86ba0-goog
> > >
