| Message ID | 20200218184756.242904-1-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Suppress warning in __kvm_gfn_to_hva_cache_init | expand |
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 >
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 > >
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 > > >
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
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
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
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!
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
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;
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(-)