From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Rostedt Subject: Re: [PATCH] wrong accounting in direct_remap_pfn_range Date: Wed, 30 Aug 2006 20:17:39 -0400 Message-ID: <44F62AA3.7050604@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-devel@lists.xensource.com, quintela@redhat.com List-Id: xen-devel@lists.xenproject.org Keir Fraser wrote: > I think you're confused. I usually am :) But not on this one :P > The only time we execute the test-and-fixup code is Actually, "fixup" was the wrong name of the variable, so it was confusing. Perhaps I should have called it "finish", as in finish processing. > if we exit the for loop in the normal way. All exceptional exits from the > loop are done by 'goto out', which skips the test-and-fixup code. So we can > only reach the test-and-fixup code after executing a whole number of > iterations of the for loop, so I don't think that there is a problem. I wasn't talking about exceptions. Just fixing up what wasn't finished. But the "fixup" name made it confusing. The problem is here: let me cut and paste the code to show you. for (i = 0; i < size; i += PAGE_SIZE) { ** we are looping here to cover each value in v->val that walks up "u" if ((v - u) == (PAGE_SIZE / sizeof(mmu_update_t))) { ** we enter this condition when we have filled up "u". /* Fill in the PTE pointers. */ rc = apply_to_page_range(mm, start_address, address - start_address, direct_remap_area_pte_fn, &w); if (rc) goto out; w = u; rc = -EFAULT; if (HYPERVISOR_mmu_update(u, v - u, NULL, domid) < 0) goto out; ** here we assign v to u and if this was our last address. That is ** we break out of the loop, you think there's nothing more to be done. ** Right?? v = u; start_address = address; ** But we continue the loop. ** Note: This could also be solved by putting in: * if (i == size) * break; } /* * Fill in the machine address: PTE ptr is done later by * __direct_remap_area_pages(). */ ** we set v->val to the next mfn, even if we are done (because we filled ** up u already). ** But that's ok, since we are done, we don't need to do anything with ** "u" anymore, right? v->val = pte_val_ma(pfn_pte_ma(mfn, prot)); mfn++; address += PAGE_SIZE; ** oooh, we increment v here, so v != u, although we finished and are ** done, and we now have a bad value in u. v++; } ** Uh Oh, v != u so we go into this if statement. if (v != u) { /* get the ptep's filled in */ ** Now we "apply_to_page_range" with a bad value in "u". rc = apply_to_page_range(mm, start_address, address - start_address, direct_remap_area_pte_fn, &w); if (rc) goto out; rc = -EFAULT; if (unlikely(HYPERVISOR_mmu_update(u, v - u, NULL, domid) < 0)) goto out; } So "fixup" was a bad choice for a variable name. Should I resend the patch with the variable "done" or "finished"? Or do you prefer the "if (i == size) break;" better? This bug probably was never hit, since the chances of sending in just enough pages to map that fills up "u" but no more is very very slim. But it _can_ happen, and I'd hate to be the one who is assigned that bugzilla! This bug was found by me just going over the code and seeing how things tick. I didn't actually run into the bug. But if you tell me that this isn't a bug, then you might as well remove that if statement and just do that apply_to_page_range every time, or tell me what I'm really missing. So, I'm not confused here, but sorry for confusing you :) -- Steve