On Wed, Jan 9, 2019 at 2:50 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Jim Mattson,
>
> The patch f1b9dd5eb86c: "kvm: Disallow wraparound in
> kvm_gfn_to_hva_cache_init" from Dec 17, 2018, leads to the following
> static checker warning:
>
> arch/x86/kvm/../../../virt/kvm/kvm_main.c:2024 __kvm_gfn_to_hva_cache_init()
> error: uninitialized symbol 'nr_pages_avail'.
>
> arch/x86/kvm/../../../virt/kvm/kvm_main.c
> 1998 static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
> 1999 struct gfn_to_hva_cache *ghc,
> 2000 gpa_t gpa, unsigned long len)
> 2001 {
> 2002 int offset = offset_in_page(gpa);
> 2003 gfn_t start_gfn = gpa >> PAGE_SHIFT;
> 2004 gfn_t end_gfn = (gpa + len - 1) >> PAGE_SHIFT;
> 2005 gfn_t nr_pages_needed = end_gfn - start_gfn + 1;
> 2006 gfn_t nr_pages_avail;
> 2007 int r = start_gfn <= end_gfn ? 0 : -EINVAL;
> 2008
> 2009 ghc->gpa = gpa;
> 2010 ghc->generation = slots->generation;
> 2011 ghc->len = len;
> 2012 ghc->hva = KVM_HVA_ERR_BAD;
> 2013
> 2014 /*
> 2015 * If the requested region crosses two memslots, we still
> 2016 * verify that the entire region is valid here.
> 2017 */
> 2018 while (!r && start_gfn <= end_gfn) {
> 2019 ghc->memslot = __gfn_to_memslot(slots, start_gfn);
> 2020 ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn,
> 2021 &nr_pages_avail);
> 2022 if (kvm_is_error_hva(ghc->hva))
> 2023 r = -EFAULT;
> --> 2024 start_gfn += nr_pages_avail;
>
> Setting start_gfn to garbage is harmless, but adding a break statement
> would make the static checker happy.
No objection here.
> 2025 }
> 2026
> 2027 /* Use the slow path for cross page reads and writes. */
> 2028 if (!r && nr_pages_needed == 1)
> 2029 ghc->hva += offset;
> 2030 else
> 2031 ghc->memslot = NULL;
>
> This chunk is really confusing. It would be more clear to write it
> like this:
>
> if (r) {
> ghc->memslot = NULL;
> return r;
> }
>
> if (end_gfn == start_gfn) {
> ghc->hva += offset;
> return 0;
> }
>
> ghc->memslot = NULL;
> return 0;
>
> But even though the code is now clear, I'm still a bit confused why
> we're setting ->memslot to NULL on the success path.
That was the original behavior, before my change. This is to force
"the slow path for cross page reads and writes," as indicated in the
comment above.
>
> 2032
> 2033 return r;
> 2034 }
>
> regards,
> dan carpenter
