[PATCH 0/4] XSA-276 follow-up

[PATCH 0/4] XSA-276 follow-up

[Jan Beulich]

A few things I had run into while working on that issue:

1: mm: disallow MEMF_no_refcount to be passed for domain-owned allocations
2: x86: correct instances of PGC_allocated clearing
3: x86: reduce code duplication in guest_remove_page()
4: make domain_adjust_tot_pages() __must_check

They don't depend on one another, they're grouped together
just because of their origin.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/4] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Jan Beulich]

When such pages get assigned to domains (and hence their ->tot_pages
not incremented accordingly) we would otherwise also need to suppress
decrementing the count when freeing those pages.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2303,6 +2303,8 @@ struct page_info *alloc_domheap_pages(
 
     if ( memflags & MEMF_no_owner )
         memflags |= MEMF_no_refcount;
+    else if ( (memflags & MEMF_no_refcount) && d )
+        return NULL;
 
     if ( dma_bitsize && ((dma_zone = bits_to_zone(dma_bitsize)) < zone_hi) )
         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Andrew Cooper]

On 20/11/2018 16:17, Jan Beulich wrote:
> When such pages get assigned to domains (and hence their ->tot_pages
> not incremented accordingly) we would otherwise also need to suppress
> decrementing the count when freeing those pages.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2303,6 +2303,8 @@ struct page_info *alloc_domheap_pages(
>  
>      if ( memflags & MEMF_no_owner )
>          memflags |= MEMF_no_refcount;
> +    else if ( (memflags & MEMF_no_refcount) && d )
> +        return NULL;

Perhaps ASSERT_UNREACHABLE() in here as well?

As we've learnt the hard way, this is definitely a programming error, so
providing a stack trace is liable to be more helpful to someone
debugging why their memory allocations are suddenly failing.

~Andrew

>  
>      if ( dma_bitsize && ((dma_zone = bits_to_zone(dma_bitsize)) < zone_hi) )
>          pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Jan Beulich]

When such pages get assigned to domains (and hence their ->tot_pages
not incremented accordingly) we would otherwise also need to suppress
decrementing the count when freeing those pages.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add ASSERT_UNREACHABLE().

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2303,6 +2303,11 @@ struct page_info *alloc_domheap_pages(
 
     if ( memflags & MEMF_no_owner )
         memflags |= MEMF_no_refcount;
+    else if ( (memflags & MEMF_no_refcount) && d )
+    {
+        ASSERT_UNREACHABLE();
+        return NULL;
+    }
 
     if ( dma_bitsize && ((dma_zone = bits_to_zone(dma_bitsize)) < zone_hi) )
         pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Andrew Cooper]

On 23/11/2018 09:45, Jan Beulich wrote:
> When such pages get assigned to domains (and hence their ->tot_pages
> not incremented accordingly) we would otherwise also need to suppress
> decrementing the count when freeing those pages.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add ASSERT_UNREACHABLE().
>
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2303,6 +2303,11 @@ struct page_info *alloc_domheap_pages(
>  
>      if ( memflags & MEMF_no_owner )
>          memflags |= MEMF_no_refcount;
> +    else if ( (memflags & MEMF_no_refcount) && d )
> +    {
> +        ASSERT_UNREACHABLE();

Sorry to do this, but on second thoughts, this path isn't actually
unreachable.

Could I talk you into using ASSERT(!"Assigned domheap pages must be
refcounted") instead, to give a slightly more clear error to developers
who manage to hit it?

If you're happy with something along those lines, Reviewed-by: Andrew
Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Jan Beulich]

>>> On 23.11.18 at 11:28, <andrew.cooper3@citrix.com> wrote:
> On 23/11/2018 09:45, Jan Beulich wrote:
>> When such pages get assigned to domains (and hence their ->tot_pages
>> not incremented accordingly) we would otherwise also need to suppress
>> decrementing the count when freeing those pages.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Add ASSERT_UNREACHABLE().
>>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2303,6 +2303,11 @@ struct page_info *alloc_domheap_pages(
>>  
>>      if ( memflags & MEMF_no_owner )
>>          memflags |= MEMF_no_refcount;
>> +    else if ( (memflags & MEMF_no_refcount) && d )
>> +    {
>> +        ASSERT_UNREACHABLE();
> 
> Sorry to do this, but on second thoughts, this path isn't actually
> unreachable.
> 
> Could I talk you into using ASSERT(!"Assigned domheap pages must be
> refcounted") instead, to give a slightly more clear error to developers
> who manage to hit it?

I think there are other places where we use ASSERT_UNREACHABLE()
when the path is reachable in the sense that one could construct a
suitable path, but with how things are (supposed to be) it cannot be
reached. I'm unconvinced the added string literal would be of overly
much help - once you see the line number, it is pretty easy to figure
out whats wrong.

But if you insist, I'll switch to the alternative way of expressing it
(although I'd then perhaps use
ASSERT(!(memflags & MEMF_no_refcount)) instead). Just let me
know.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

[Andrew Cooper]

On 23/11/2018 10:49, Jan Beulich wrote:
>>>> On 23.11.18 at 11:28, <andrew.cooper3@citrix.com> wrote:
>> On 23/11/2018 09:45, Jan Beulich wrote:
>>> When such pages get assigned to domains (and hence their ->tot_pages
>>> not incremented accordingly) we would otherwise also need to suppress
>>> decrementing the count when freeing those pages.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v2: Add ASSERT_UNREACHABLE().
>>>
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2303,6 +2303,11 @@ struct page_info *alloc_domheap_pages(
>>>  
>>>      if ( memflags & MEMF_no_owner )
>>>          memflags |= MEMF_no_refcount;
>>> +    else if ( (memflags & MEMF_no_refcount) && d )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>> Sorry to do this, but on second thoughts, this path isn't actually
>> unreachable.
>>
>> Could I talk you into using ASSERT(!"Assigned domheap pages must be
>> refcounted") instead, to give a slightly more clear error to developers
>> who manage to hit it?
> I think there are other places where we use ASSERT_UNREACHABLE()
> when the path is reachable in the sense that one could construct a
> suitable path, but with how things are (supposed to be) it cannot be
> reached. I'm unconvinced the added string literal would be of overly
> much help - once you see the line number, it is pretty easy to figure
> out whats wrong.
>
> But if you insist, I'll switch to the alternative way of expressing it
> (although I'd then perhaps use
> ASSERT(!(memflags & MEMF_no_refcount)) instead). Just let me
> know.

That's fine as well.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/4] x86: correct instances of PGC_allocated clearing

[Jan Beulich]

For domain heap pages assigned to a domain dropping the page reference
tied to PGC_allocated may not drop the last reference, as otherwise the
test_and_clear_bit() might already act on an unowned page.

Work around this where possible, but the need to acquire extra page
references is a fair hint that references should have been acquired in
other places instead.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Compile tested only, as I have neither a mem-sharing nor a mem-paging
environment set up ready to be used for such testing.

--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -964,6 +964,15 @@ static int share_pages(struct domain *sd
         goto err_out;
     }
 
+    /* Acquire an extra reference, for the freeing below to be safe. */
+    if ( !get_page(cpage, cd) )
+    {
+        ret = -EOVERFLOW;
+        mem_sharing_page_unlock(secondpg);
+        mem_sharing_page_unlock(firstpg);
+        goto err_out;
+    }
+
     /* Merge the lists together */
     rmap_seed_iterator(cpage, &ri);
     while ( (gfn = rmap_iterate(cpage, &ri)) != NULL)
@@ -993,6 +1002,7 @@ static int share_pages(struct domain *sd
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
+    put_page(cpage);
 
     /* We managed to free a domain page. */
     atomic_dec(&nr_shared_mfns);
@@ -1066,9 +1076,16 @@ int mem_sharing_add_to_physmap(struct do
             if ( mfn_valid(cmfn) )
             {
                 struct page_info *cpage = mfn_to_page(cmfn);
-                ASSERT(cpage != NULL);
+
+                if ( !get_page(cpage, cd) )
+                {
+                    domain_crash(cd);
+                    ret = -EOVERFLOW;
+                    goto err_unlock;
+                }
                 if ( test_and_clear_bit(_PGC_allocated, &cpage->count_info) )
                     put_page(cpage);
+                put_page(cpage);
             }
         }
     }
@@ -1153,9 +1170,18 @@ int __mem_sharing_unshare_page(struct do
             mem_sharing_gfn_destroy(page, d, gfn_info);
         put_page_and_type(page);
         mem_sharing_page_unlock(page);
-        if ( last_gfn && 
-            test_and_clear_bit(_PGC_allocated, &page->count_info) ) 
+        if ( last_gfn )
+        {
+            if ( !get_page(page, d) )
+            {
+                put_gfn(d, gfn);
+                domain_crash(d);
+                return -EOVERFLOW;
+            }
+            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
+                put_page(page);
             put_page(page);
+        }
         put_gfn(d, gfn);
 
         return 0;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -313,20 +313,36 @@ int guest_remove_page(struct domain *d,
 
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
+        /*
+         * If the page hasn't yet been paged out, there is an
+         * actual page that needs to be released.
+         */
+        if ( p2mt == p2m_ram_paging_out )
+        {
+            ASSERT(mfn_valid(mfn));
+            page = mfn_to_page(mfn);
+            rc = -ENXIO;
+            if ( !get_page(page, d) )
+                goto out_put_gfn;
+        }
+        else
+            page = NULL;
+
         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         if ( rc )
+        {
+            if ( page )
+                put_page(page);
             goto out_put_gfn;
+        }
 
         put_gfn(d, gmfn);
 
-        /* If the page hasn't yet been paged out, there is an
-         * actual page that needs to be released. */
-        if ( p2mt == p2m_ram_paging_out )
+        if ( page )
         {
-            ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn);
             if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
                 put_page(page);
+            put_page(page);
         }
         p2m_mem_paging_drop_page(d, gmfn, p2mt);
 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86: correct instances of PGC_allocated clearing

[Andrew Cooper]

On 20/11/2018 16:18, Jan Beulich wrote:
> For domain heap pages assigned to a domain dropping the page reference
> tied to PGC_allocated may not drop the last reference, as otherwise the
> test_and_clear_bit() might already act on an unowned page.
>
> Work around this where possible, but the need to acquire extra page
> references is a fair hint that references should have been acquired in
> other places instead.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Compile tested only, as I have neither a mem-sharing nor a mem-paging
> environment set up ready to be used for such testing.

Perhaps we should compile them out by default?  It's clear there are no
production users, given the quality of the code and how many security
issues we spot accidentally.

Either way, the code looks plausible.  Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86: correct instances of PGC_allocated clearing

[Jan Beulich]

>>> On 20.11.18 at 17:59, <andrew.cooper3@citrix.com> wrote:
> On 20/11/2018 16:18, Jan Beulich wrote:
>> For domain heap pages assigned to a domain dropping the page reference
>> tied to PGC_allocated may not drop the last reference, as otherwise the
>> test_and_clear_bit() might already act on an unowned page.
>>
>> Work around this where possible, but the need to acquire extra page
>> references is a fair hint that references should have been acquired in
>> other places instead.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Compile tested only, as I have neither a mem-sharing nor a mem-paging
>> environment set up ready to be used for such testing.
> 
> Perhaps we should compile them out by default?  It's clear there are no
> production users, given the quality of the code and how many security
> issues we spot accidentally.

Yeah, well - if we're going to have a perhaps much wider set of
config options, then these two surely should become "default n"
until they've been brought out of their sorry state.

> Either way, the code looks plausible.  Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86: correct instances of PGC_allocated clearing

[Tamas K Lengyel]

On Tue, Nov 20, 2018 at 10:12 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 20.11.18 at 17:59, <andrew.cooper3@citrix.com> wrote:
> > On 20/11/2018 16:18, Jan Beulich wrote:
> >> For domain heap pages assigned to a domain dropping the page reference
> >> tied to PGC_allocated may not drop the last reference, as otherwise the
> >> test_and_clear_bit() might already act on an unowned page.
> >>
> >> Work around this where possible, but the need to acquire extra page
> >> references is a fair hint that references should have been acquired in
> >> other places instead.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Compile tested only, as I have neither a mem-sharing nor a mem-paging
> >> environment set up ready to be used for such testing.

This is how I test memsharing: you save a vm such that it's kept
paused (xl save -p), copy its config file but change the name, restore
the saved image with this new config file and the vm kept paused (xl
restore -p). Then you can use tests/mem-sharing on the two VMs:
./memshrtool enable <first_vm> &&./memshrtool enable <second_vm> &&
./memshrtool range <first_vm> <second_vm> 0 1000

Unpausing the second vm afterwards will exercise memsharing.

> >
> > Perhaps we should compile them out by default?  It's clear there are no
> > production users, given the quality of the code and how many security
> > issues we spot accidentally.
>
> Yeah, well - if we're going to have a perhaps much wider set of
> config options, then these two surely should become "default n"
> until they've been brought out of their sorry state.

+1

Code also looks OK to me:

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/4] x86: reduce code duplication in guest_remove_page()

[Jan Beulich]

Quite a bit of duplicate code has accumulated on the "paging" types
special case path. Re-use what can be re-used from the common path.

Since it needs touching anyway, slightly re-format and extend the
gdprintk() on the common path as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -320,30 +320,15 @@ int guest_remove_page(struct domain *d,
         if ( p2mt == p2m_ram_paging_out )
         {
             ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn);
-            rc = -ENXIO;
-            if ( !get_page(page, d) )
-                goto out_put_gfn;
+            goto obtain_page;
         }
-        else
-            page = NULL;
 
         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         if ( rc )
-        {
-            if ( page )
-                put_page(page);
             goto out_put_gfn;
-        }
 
         put_gfn(d, gmfn);
 
-        if ( page )
-        {
-            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
-                put_page(page);
-            put_page(page);
-        }
         p2m_mem_paging_drop_page(d, gmfn, p2mt);
 
         return 0;
@@ -385,11 +370,16 @@ int guest_remove_page(struct domain *d,
     }
 #endif /* CONFIG_X86 */
 
+ obtain_page: __maybe_unused;
     page = mfn_to_page(mfn);
     if ( unlikely(!get_page(page, d)) )
     {
         put_gfn(d, gmfn);
-        gdprintk(XENLOG_INFO, "Bad page free for domain %u\n", d->domain_id);
+#ifdef CONFIG_X86
+        if ( !p2m_is_paging(p2mt) )
+#endif
+            gdprintk(XENLOG_INFO, "Bad page free for Dom%u GFN %lx\n",
+                     d->domain_id, gmfn);
 
         return -ENXIO;
     }





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86: reduce code duplication in guest_remove_page()

[Andrew Cooper]

On 20/11/2018 16:19, Jan Beulich wrote:
> Quite a bit of duplicate code has accumulated on the "paging" types
> special case path. Re-use what can be re-used from the common path.
>
> Since it needs touching anyway, slightly re-format and extend the
> gdprintk() on the common path as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Doesn't this negate the previous patch?  I'm afraid that I'm rather
confused.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86: reduce code duplication in guest_remove_page()

[Jan Beulich]

>>> On 20.11.18 at 18:06, <andrew.cooper3@citrix.com> wrote:
> On 20/11/2018 16:19, Jan Beulich wrote:
>> Quite a bit of duplicate code has accumulated on the "paging" types
>> special case path. Re-use what can be re-used from the common path.
>>
>> Since it needs touching anyway, slightly re-format and extend the
>> gdprintk() on the common path as well.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Doesn't this negate the previous patch?  I'm afraid that I'm rather
> confused.

No - please note the new "obtain_page" label that the code branches
to, which replaces what the earlier patch has introduced. Beyond this
code replacement the patch here simply leverages that "page" would
then be NULL throughout the remaining part of this special path, and
hence all respective conditionals and code can go away. It merely
seemed to me that doing the overall transformation in two steps would
be better; in principle the change here could be merged into the
earlier patch.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/4] make domain_adjust_tot_pages() __must_check

[Jan Beulich]

Even if unlikely, donate_page() should not ignore the possible need to
obtain a domain reference. To make people look more closely when they
add new uses of domain_adjust_tot_pages(), force its return value to be
checked. This in turn requires a benign change to assign_pages().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4008,7 +4008,8 @@ int donate_page(
     {
         if ( d->tot_pages >= d->max_pages )
             goto fail;
-        domain_adjust_tot_pages(d, 1);
+        if ( unlikely(domain_adjust_tot_pages(d, 1) == 1) )
+            get_knownalive_domain(d);
     }
 
     page->count_info = PGC_allocated | 1;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2265,10 +2265,8 @@ int assign_pages(
             goto out;
         }
 
-        if ( unlikely(d->tot_pages == 0) )
+        if ( unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
             get_knownalive_domain(d);
-
-        domain_adjust_tot_pages(d, 1 << order);
     }
 
     for ( i = 0; i < (1 << order); i++ )
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -190,7 +190,8 @@ int destroy_xen_mappings(unsigned long v
  */
 int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
 /* Claim handling */
-unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
+unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
+    long pages);
 int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
 void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 0/2] remaining XSA-276 follow-up

[Jan Beulich]

A few things I had run into while working on that issue:

1: x86: reduce code duplication in guest_remove_page()
2: make domain_adjust_tot_pages() __must_check

They don't depend on one another, they're grouped together
just because of their origin.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/2] x86: reduce code duplication in guest_remove_page()

[Jan Beulich]

Quite a bit of duplicate code has accumulated on the "paging" types
special case path. Re-use what can be re-used from the common path.

Since it needs touching anyway, slightly re-format and extend the
gdprintk() on the common path as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base.

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -320,30 +320,15 @@ int guest_remove_page(struct domain *d,
         if ( p2mt == p2m_ram_paging_out )
         {
             ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn);
-            rc = -ENXIO;
-            if ( !get_page(page, d) )
-                goto out_put_gfn;
+            goto obtain_page;
         }
-        else
-            page = NULL;
 
         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         if ( rc )
-        {
-            if ( page )
-                put_page(page);
             goto out_put_gfn;
-        }
 
         put_gfn(d, gmfn);
 
-        if ( page )
-        {
-            if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
-                put_page(page);
-            put_page(page);
-        }
         p2m_mem_paging_drop_page(d, gmfn, p2mt);
 
         return 0;
@@ -387,13 +372,16 @@ int guest_remove_page(struct domain *d,
     }
 #endif /* CONFIG_X86 */
 
+ obtain_page: __maybe_unused;
     page = mfn_to_page(mfn);
     if ( unlikely(!get_page(page, d)) )
     {
 #ifdef CONFIG_X86
         put_gfn(d, gmfn);
+        if ( !p2m_is_paging(p2mt) )
 #endif
-        gdprintk(XENLOG_INFO, "Bad page free for domain %u\n", d->domain_id);
+            gdprintk(XENLOG_INFO, "Bad page free for Dom%u GFN %lx\n",
+                     d->domain_id, gmfn);
 
         return -ENXIO;
     }





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/2] x86: reduce code duplication in guest_remove_page()

[Andrew Cooper]

On 05/12/2018 16:17, Jan Beulich wrote:
> Quite a bit of duplicate code has accumulated on the "paging" types
> special case path. Re-use what can be re-used from the common path.
>
> Since it needs touching anyway, slightly re-format and extend the
> gdprintk() on the common path as well.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/2] make domain_adjust_tot_pages() __must_check

[Jan Beulich]

Even if unlikely, donate_page() should not ignore the possible need to
obtain a domain reference. To make people look more closely when they
add new uses of domain_adjust_tot_pages(), force its return value to be
checked. This in turn requires a benign change to assign_pages().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4009,7 +4009,8 @@ int donate_page(
     {
         if ( d->tot_pages >= d->max_pages )
             goto fail;
-        domain_adjust_tot_pages(d, 1);
+        if ( unlikely(domain_adjust_tot_pages(d, 1) == 1) )
+            get_knownalive_domain(d);
     }
 
     page->count_info = PGC_allocated | 1;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2273,10 +2273,8 @@ int assign_pages(
             goto out;
         }
 
-        if ( unlikely(d->tot_pages == 0) )
+        if ( unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
             get_knownalive_domain(d);
-
-        domain_adjust_tot_pages(d, 1 << order);
     }
 
     for ( i = 0; i < (1 << order); i++ )
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -190,7 +190,8 @@ int destroy_xen_mappings(unsigned long v
  */
 int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
 /* Claim handling */
-unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
+unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
+    long pages);
 int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
 void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] make domain_adjust_tot_pages() __must_check

[Andrew Cooper]

On 05/12/2018 16:17, Jan Beulich wrote:
> Even if unlikely, donate_page() should not ignore the possible need to
> obtain a domain reference. To make people look more closely when they
> add new uses of domain_adjust_tot_pages(), force its return value to be
> checked. This in turn requires a benign change to assign_pages().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This is very weird code to read.  It is a side effect of the ABI, and it
appears we do have this style used elsewhere.

Therefore, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

However, I think this would be improved if we had helpers along the
lines of "bool domain_{add,remove}_pages()" where the boolean return
indicated whether you needed to play with the domain reference count.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel