Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range

[Steven Rostedt <srostedt@xxxxxxxxxx>]

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