[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap



Hi Penny,

On 10/05/2022 03:27, Penny Zheng wrote:
Pages used as guest RAM for static domain, shall be reserved to this
domain only.
So in case reserved pages being used for other purpose, users
shall not free them back to heap, even when last ref gets dropped.

free_staticmem_pages will be called by free_heap_pages in runtime
for static domain freeing memory resource, so let's drop the __init
flag.

Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
---
v4 changes:
- no changes
---
v3 changes:
- fix possible racy issue in free_staticmem_pages()
- introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
- move the change to free_heap_pages() to cover other potential call sites
- fix the indentation
---
v2 changes:
- new commit
---
  xen/common/page_alloc.c | 17 ++++++++++++++---
  xen/include/xen/mm.h    |  2 +-
  2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..5e569a48a2 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1443,6 +1443,10 @@ static void free_heap_pages(
ASSERT(order <= MAX_ORDER); + if ( pg->count_info & PGC_reserved )

NIT: I would suggest to use "unlikely()" here.

+        /* Reserved page shall not go back to the heap. */
+        return free_staticmem_pages(pg, 1UL << order, need_scrub);
+
      spin_lock(&heap_lock);
for ( i = 0; i < (1 << order); i++ )
@@ -2636,8 +2640,8 @@ struct domain *get_pg_owner(domid_t domid)
#ifdef CONFIG_STATIC_MEMORY
  /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
-void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
-                                 bool need_scrub)
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)

Looking at the implementation of free_staticmem_pages(), the page will be scrubbed synchronously.

If I am not mistaken, static memory is not yet supported so I would be OK to continue with synchronous scrubbing. However, this will need to be asynchronous before we even consider to security support it.

BTW, SUPPORT.md doesn't seem to explicitely say whether static memory is supported. Would you be able to send a patch to update it? I think this should be tech preview for now.

  {
      mfn_t mfn = page_to_mfn(pg);
      unsigned long i;
@@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info *pg, 
unsigned long nr_mfns,
          }
/* In case initializing page of static memory, mark it PGC_reserved. */
-        pg[i].count_info |= PGC_reserved;
+        if ( !(pg[i].count_info & PGC_reserved) )

NIT: I understand the flag may have already been set, but I am not convinced if it is worth checking it and then set.

+            pg[i].count_info |= PGC_reserved;


      }
  }
@@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn, return 0;
  }
+#else
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)
+{
+    ASSERT_UNREACHABLE();
+}
  #endif
/*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3be754da92..9fd95deaec 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,10 +85,10 @@ bool scrub_free_pages(void);
  } while ( false )
  #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
-#ifdef CONFIG_STATIC_MEMORY
  /* These functions are for static memory */
  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                            bool need_scrub);
+#ifdef CONFIG_STATIC_MEMORY
  int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int 
nr_mfns,
                              unsigned int memflags);
  #endif

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.