1. Patchew
  2. Xen
  3. View series

[Xen-devel] [PATCH] mm: Safe to clear PGC_allocated on xenheap pages without an extra reference

George Dunlap posted 1 patch 4 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190802161611.4145-1-george.dunlap@citrix.com
xen/include/xen/mm.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
[Xen-devel] [PATCH] mm: Safe to clear PGC_allocated on xenheap pages without an extra reference
Posted by George Dunlap 4 years, 8 months ago 
Commits ec83f825627 "mm.h: add helper function to test-and-clear
_PGC_allocated" (and subsequent fix-up 44a887d021d "mm.h: fix BUG_ON()
condition in put_page_alloc_ref()") introduced a BUG_ON() to detect
unsafe behavior of callers.

Unfortunately this condition still turns out to be too strict.
xenheap pages are somewhat "magic": calling free_domheap_pages() on
them will not cause free_heap_pages() to be called: whichever part of
Xen allocated them specially must call free_xenheap_pages()
specifically.  (They'll also be handled appropriately at domain
destruction time.)

Only crash Xen when put_page_alloc_ref() finds only a single refcount
if the page is not a xenheap page.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
NB this has been compile-tested only.

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
 xen/include/xen/mm.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 977e45aae7..297fc6ad7b 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -666,15 +666,20 @@ static inline void share_xen_page_with_privileged_guests(
 static inline void put_page_alloc_ref(struct page_info *page)
 {
     /*
-     * Whenever a page is assigned to a domain then the _PGC_allocated bit
-     * is set and the reference count is set to at least 1. This function
-     * clears that 'allocation reference' but it is unsafe to do so without
-     * the caller holding an additional reference. I.e. the allocation
-     * reference must never be the last reference held.
+     * Whenever a page is assigned to a domain then the _PGC_allocated
+     * bit is set and the reference count is set to at least 1. This
+     * function clears that 'allocation reference' but it is unsafe to
+     * do so to domheap pages without the caller holding an additional
+     * reference. I.e. the allocation reference must never be the last
+     * reference held.
+     * 
+     * (It's safe for xenheap pages, because put_page() will not cause
+     * them to be freed.)
      */
     if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
     {
-        BUG_ON((page->count_info & PGC_count_mask) <= 1);
+        BUG_ON((page->count_info & PGC_count_mask) <= 1 &&
+               !(page->count_info & PGC_xen_heap));
         put_page(page);
     }
 }
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] mm: Safe to clear PGC_allocated on xenheap pages without an extra reference
Posted by Andrew Cooper 4 years, 8 months ago 
On 02/08/2019 17:16, George Dunlap wrote:
> Commits ec83f825627 "mm.h: add helper function to test-and-clear
> _PGC_allocated" (and subsequent fix-up 44a887d021d "mm.h: fix BUG_ON()
> condition in put_page_alloc_ref()") introduced a BUG_ON() to detect
> unsafe behavior of callers.
>
> Unfortunately this condition still turns out to be too strict.
> xenheap pages are somewhat "magic": calling free_domheap_pages() on
> them will not cause free_heap_pages() to be called: whichever part of
> Xen allocated them specially must call free_xenheap_pages()
> specifically.  (They'll also be handled appropriately at domain
> destruction time.)
>
> Only crash Xen when put_page_alloc_ref() finds only a single refcount
> if the page is not a xenheap page.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

This works for me, so Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

Some suggestions however...

> ---
> NB this has been compile-tested only.
>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/include/xen/mm.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 977e45aae7..297fc6ad7b 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -666,15 +666,20 @@ static inline void share_xen_page_with_privileged_guests(
>  static inline void put_page_alloc_ref(struct page_info *page)
>  {
>      /*
> -     * Whenever a page is assigned to a domain then the _PGC_allocated bit
> -     * is set and the reference count is set to at least 1. This function
> -     * clears that 'allocation reference' but it is unsafe to do so without
> -     * the caller holding an additional reference. I.e. the allocation
> -     * reference must never be the last reference held.
> +     * Whenever a page is assigned to a domain then the _PGC_allocated
> +     * bit is set and the reference count is set to at least 1. This
> +     * function clears that 'allocation reference' but it is unsafe to
> +     * do so to domheap pages without the caller holding an additional
> +     * reference. I.e. the allocation reference must never be the last
> +     * reference held.
> +     * 

Trailing whitespace.

> +     * (It's safe for xenheap pages, because put_page() will not cause
> +     * them to be freed.)
>       */
>      if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>      {
> -        BUG_ON((page->count_info & PGC_count_mask) <= 1);
> +        BUG_ON((page->count_info & PGC_count_mask) <= 1 &&
> +               !(page->count_info & PGC_xen_heap));

I know this is a matter of preference, but I think the logic would be
easier to follow as

BUG_ON(!(page->count_info & PGC_xen_heap) &&
       (page->count_info & PGC_count_mask) <= 1);

i.e. check for domheap pages before looking at the refcount.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] mm: Safe to clear PGC_allocated on xenheap pages without an extra reference
Posted by Jan Beulich 4 years, 8 months ago 
On 02.08.2019 18:16, George Dunlap wrote:
> Commits ec83f825627 "mm.h: add helper function to test-and-clear
> _PGC_allocated" (and subsequent fix-up 44a887d021d "mm.h: fix BUG_ON()
> condition in put_page_alloc_ref()") introduced a BUG_ON() to detect
> unsafe behavior of callers.
> 
> Unfortunately this condition still turns out to be too strict.
> xenheap pages are somewhat "magic": calling free_domheap_pages() on
> them will not cause free_heap_pages() to be called: whichever part of
> Xen allocated them specially must call free_xenheap_pages()
> specifically.  (They'll also be handled appropriately at domain
> destruction time.)
> 
> Only crash Xen when put_page_alloc_ref() finds only a single refcount
> if the page is not a xenheap page.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with a suggestion:

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -666,15 +666,20 @@ static inline void share_xen_page_with_privileged_guests(
>   static inline void put_page_alloc_ref(struct page_info *page)
>   {
>       /*
> -     * Whenever a page is assigned to a domain then the _PGC_allocated bit
> -     * is set and the reference count is set to at least 1. This function
> -     * clears that 'allocation reference' but it is unsafe to do so without
> -     * the caller holding an additional reference. I.e. the allocation
> -     * reference must never be the last reference held.
> +     * Whenever a page is assigned to a domain then the _PGC_allocated
> +     * bit is set and the reference count is set to at least 1. This
> +     * function clears that 'allocation reference' but it is unsafe to
> +     * do so to domheap pages without the caller holding an additional
> +     * reference. I.e. the allocation reference must never be the last
> +     * reference held.
> +     *
> +     * (It's safe for xenheap pages, because put_page() will not cause
> +     * them to be freed.)
>        */
>       if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>       {
> -        BUG_ON((page->count_info & PGC_count_mask) <= 1);
> +        BUG_ON((page->count_info & PGC_count_mask) <= 1 &&
> +               !(page->count_info & PGC_xen_heap));

This can be had with a single conditional:

         BUG_ON((page->count_info & (PGC_xen_heap | PGC_count_mask)) <= 1);

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] mm: Safe to clear PGC_allocated on xenheap pages without an extra reference
Posted by George Dunlap 4 years, 8 months ago 
On 8/5/19 10:42 AM, Jan Beulich wrote:
> On 02.08.2019 18:16, George Dunlap wrote:
>> Commits ec83f825627 "mm.h: add helper function to test-and-clear
>> _PGC_allocated" (and subsequent fix-up 44a887d021d "mm.h: fix BUG_ON()
>> condition in put_page_alloc_ref()") introduced a BUG_ON() to detect
>> unsafe behavior of callers.
>>
>> Unfortunately this condition still turns out to be too strict.
>> xenheap pages are somewhat "magic": calling free_domheap_pages() on
>> them will not cause free_heap_pages() to be called: whichever part of
>> Xen allocated them specially must call free_xenheap_pages()
>> specifically.  (They'll also be handled appropriately at domain
>> destruction time.)
>>
>> Only crash Xen when put_page_alloc_ref() finds only a single refcount
>> if the page is not a xenheap page.
>>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit with a suggestion:
> 
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -666,15 +666,20 @@ static inline void share_xen_page_with_privileged_guests(
>>   static inline void put_page_alloc_ref(struct page_info *page)
>>   {
>>       /*
>> -     * Whenever a page is assigned to a domain then the _PGC_allocated bit
>> -     * is set and the reference count is set to at least 1. This function
>> -     * clears that 'allocation reference' but it is unsafe to do so without
>> -     * the caller holding an additional reference. I.e. the allocation
>> -     * reference must never be the last reference held.
>> +     * Whenever a page is assigned to a domain then the _PGC_allocated
>> +     * bit is set and the reference count is set to at least 1. This
>> +     * function clears that 'allocation reference' but it is unsafe to
>> +     * do so to domheap pages without the caller holding an additional
>> +     * reference. I.e. the allocation reference must never be the last
>> +     * reference held.
>> +     *
>> +     * (It's safe for xenheap pages, because put_page() will not cause
>> +     * them to be freed.)
>>        */
>>       if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>>       {
>> -        BUG_ON((page->count_info & PGC_count_mask) <= 1);
>> +        BUG_ON((page->count_info & PGC_count_mask) <= 1 &&
>> +               !(page->count_info & PGC_xen_heap));
> 
> This can be had with a single conditional:
> 
>          BUG_ON((page->count_info & (PGC_xen_heap | PGC_count_mask)) <= 1);

I'll commit this version.  Thanks.

 -George

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.