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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home