This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range
From: Steven Rostedt <srostedt@xxxxxxxxxx>
Date: Wed, 30 Aug 2006 20:17:39 -0400
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, quintela@xxxxxxxxxx
Delivery-date: Wed, 30 Aug 2006 17:17:18 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <C11BC1E2.1894%Keir.Fraser@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <C11BC1E2.1894%Keir.Fraser@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird (X11/20060614)
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));

                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.


** 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