[PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Oliver Upton]

Particularly draconian compilers warn of a possible uninitialized use of
the nr_pages_avail variable. Silence this warning by initializing it to
zero.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 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;
 
 	/* Update ghc->generation before performing any error checks. */
 	ghc->generation = slots->generation;
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Sean Christopherson]

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.

commit 6ad1e29fe0aba843dfffc714fced0ef6a2e19502
Author: Sean Christopherson <sean.j.christopherson@intel.com>
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@google.com>
    Fixes: f1b9dd5eb86c ("kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init")
    Cc: Jim Mattson <jmattson@google.com>
    Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  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;
>  
>  	/* Update ghc->generation before performing any error checks. */
>  	ghc->generation = slots->generation;
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Oliver Upton]

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.

--
Oliver

> commit 6ad1e29fe0aba843dfffc714fced0ef6a2e19502
> Author: Sean Christopherson <sean.j.christopherson@intel.com>
> 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@google.com>
>     Fixes: f1b9dd5eb86c ("kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init")
>     Cc: Jim Mattson <jmattson@google.com>
>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> 
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  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;
> >  
> >  	/* Update ghc->generation before performing any error checks. */
> >  	ghc->generation = slots->generation;
> > -- 
> > 2.25.0.265.gbab2e86ba0-goog
> > 

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Sean Christopherson]

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@intel.com>
> > 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@google.com>
> >     Fixes: f1b9dd5eb86c ("kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init")
> >     Cc: Jim Mattson <jmattson@google.com>
> >     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > 
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  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
> > > 

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Paolo Bonzini]

On 18/02/20 20:07, 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.

What if __gfn_to_hva_many and gfn_to_hva_many are marked __always_inline?

Thanks,

Paolo

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Oliver Upton]

On Thu, Feb 20, 2020 at 3:23 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 18/02/20 20:07, 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.
>
> What if __gfn_to_hva_many and gfn_to_hva_many are marked __always_inline?
>
> Thanks,
>
> Paolo
>

Even with this suggestion the compiler is ill-convinced :/

in re to Sean: what do I mean by "draconian compiler"

Well, the public answer is that both Barret and I use the same
compiler. Nothing particularly interesting about it, but idk what our
toolchain folks' stance is on divulging details.

I'll instead use Sean's suggested fix (which reads much better) and resend.

--
Thanks,
Oliver

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Paolo Bonzini]

On 21/02/20 05:32, Oliver Upton wrote:
> On Thu, Feb 20, 2020 at 3:23 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 18/02/20 20:07, 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.
>>
>> What if __gfn_to_hva_many and gfn_to_hva_many are marked __always_inline?
>>
>> Thanks,
>>
>> Paolo
>>
> 
> Even with this suggestion the compiler is ill-convinced :/
> 
> in re to Sean: what do I mean by "draconian compiler"
> 
> Well, the public answer is that both Barret and I use the same
> compiler. Nothing particularly interesting about it, but idk what our
> toolchain folks' stance is on divulging details.
> 
> I'll instead use Sean's suggested fix (which reads much better) and resend.

Can you instead look into fixing that compiler?  After inlining, it is
trivial to realize that the first two returns imply
kvm_is_error_hva(ghc->hva).  I'm asking this because even for GCC
-Wuninitialized *used to be* total crap, but these days it's quite
reliable and even basic data flow should be able to thread through the
jumps.

I'm more than willing to help the compiler with __always_inline hints,
but an "uninitialized_var()" adds load on the code and I'm not sure it
makes sense to do that for the sake of some proprietary, or at least
unnamed, software.

Paolo

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Oliver Upton]

On Fri, Feb 21, 2020 at 12:25 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 21/02/20 05:32, Oliver Upton wrote:
> > On Thu, Feb 20, 2020 at 3:23 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 18/02/20 20:07, 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.
> >>
> >> What if __gfn_to_hva_many and gfn_to_hva_many are marked __always_inline?
> >>
> >> Thanks,
> >>
> >> Paolo
> >>
> >
> > Even with this suggestion the compiler is ill-convinced :/
> >
> > in re to Sean: what do I mean by "draconian compiler"
> >
> > Well, the public answer is that both Barret and I use the same
> > compiler. Nothing particularly interesting about it, but idk what our
> > toolchain folks' stance is on divulging details.
> >
> > I'll instead use Sean's suggested fix (which reads much better) and resend.
>
> Can you instead look into fixing that compiler?  After inlining, it is
> trivial to realize that the first two returns imply
> kvm_is_error_hva(ghc->hva).  I'm asking this because even for GCC
> -Wuninitialized *used to be* total crap, but these days it's quite
> reliable and even basic data flow should be able to thread through the
> jumps.

Yeah, at this point I completely agree. I'll move onto a mainline
toolchain going forward. This was just a glaring change when I rebased
some work on top of the -Werror change (as it absolutely should).

>
> I'm more than willing to help the compiler with __always_inline hints,
> but an "uninitialized_var()" adds load on the code and I'm not sure it
> makes sense to do that for the sake of some proprietary, or at least
> unnamed, software.

Absolutely. I thought it sensible to send out the fix in case of other
toolchains out in the wild. But if nobody else other than us has
complained it's quite obvious where the problem lies.

Thanks!

Re: [PATCH] KVM: Suppress warning in __kvm_gfn_to_hva_cache_init

[Paolo Bonzini]

On 21/02/20 09:34, Oliver Upton wrote:
> Absolutely. I thought it sensible to send out the fix in case of other
> toolchains out in the wild. But if nobody else other than us has
> complained it's quite obvious where the problem lies.

Here is another plausible (and untested) way to fix it, in case it's
the alias analysis that is throwing off the compiler (plus possibly
__always_inline).

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 027259af883e..63c7dcd7c57f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2218,6 +2218,8 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
 	gfn_t end_gfn = (gpa + len - 1) >> PAGE_SHIFT;
 	gfn_t nr_pages_needed = end_gfn - start_gfn + 1;
 	gfn_t nr_pages_avail;
+	unsigned long hva;
+	struct kvm_memslot *memslot;
 
 	/* Update ghc->generation before performing any error checks. */
 	ghc->generation = slots->generation;
@@ -2231,19 +2233,22 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
 	 * If the requested region crosses two memslots, we still
 	 * verify that the entire region is valid here.
 	 */
-	for ( ; start_gfn <= end_gfn; start_gfn += nr_pages_avail) {
-		ghc->memslot = __gfn_to_memslot(slots, start_gfn);
-		ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn,
-					   &nr_pages_avail);
-		if (kvm_is_error_hva(ghc->hva))
+	do {
+		memslot = __gfn_to_memslot(slots, start_gfn);
+		hva = gfn_to_hva_many(memslot, start_gfn, &nr_pages_avail);
+		if (kvm_is_error_hva(hva))
 			return -EFAULT;
-	}
+		start_gfn += nr_pages_avail;
+	} while (start_gfn <= end_gfn);
 
 	/* Use the slow path for cross page reads and writes. */
-	if (nr_pages_needed == 1)
-		ghc->hva += offset;
-	else
+	if (nr_pages_needed == 1) {
+		ghc->hva = hva + offset;
+		ghc->memslot = memslot;
+	} else {
+		ghc->hva = 0;
 		ghc->memslot = NULL;
+	}
 
 	ghc->gpa = gpa;
 	ghc->len = len;

Paolo