[PATCH] add page_get_owner_and_reference() related ASSERT()s

[PATCH] add page_get_owner_and_reference() related ASSERT()s

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4103 bytes --]

The function shouldn't return NULL after having obtained a reference,
or else the caller won't know to drop it.

Also its result shouldn't be ignored - if calling code is certain that
a page already has a non-zero refcount, it better ASSERT()s so.

Finally this as well as get_page() and put_page() are required to be
available on all architectures - move the declarations to xen/mm.h.

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

--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,10 +1,8 @@
-#include <xen/config.h>
 #include <xen/lib.h>
 #include <xen/domain_page.h>
+#include <xen/mm.h>
 #include <xen/sched.h>
 #include <asm/current.h>
-
-#include <asm/mm.h>
 #include <asm/guest_access.h>
 
 static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1170,6 +1170,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
 struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
+    struct domain *owner;
 
     do {
         x = y;
@@ -1182,7 +1183,10 @@ struct domain *page_get_owner_and_refere
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    return page_get_owner(page);
+    owner = page_get_owner(page);
+    ASSERT(owner);
+
+    return owner;
 }
 
 void put_page(struct page_info *page)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2039,6 +2039,7 @@ void put_page(struct page_info *page)
 struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
+    struct domain *owner;
 
     do {
         x = y;
@@ -2052,7 +2053,10 @@ struct domain *page_get_owner_and_refere
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    return page_get_owner(page);
+    owner = page_get_owner(page);
+    ASSERT(owner);
+
+    return owner;
 }
 
 
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2245,7 +2245,8 @@ __acquire_grant_for_copy(
     {
         ASSERT(mfn_valid(act->frame));
         *page = mfn_to_page(act->frame);
-        (void)page_get_owner_and_reference(*page);
+        td = page_get_owner_and_reference(*page);
+        ASSERT(td);
     }
 
     act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -275,10 +275,6 @@ static inline void *page_to_virt(const s
     return mfn_to_virt(page_to_mfn(pg));
 }
 
-struct domain *page_get_owner_and_reference(struct page_info *page);
-void put_page(struct page_info *page);
-int  get_page(struct page_info *page, struct domain *domain);
-
 struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
                                     unsigned long flags);
 
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -352,9 +352,6 @@ const unsigned long *get_platform_badpag
 int page_lock(struct page_info *page);
 void page_unlock(struct page_info *page);
 
-struct domain *page_get_owner_and_reference(struct page_info *page);
-void put_page(struct page_info *page);
-int  get_page(struct page_info *page, struct domain *domain);
 void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  put_page_type_preemptible(struct page_info *page);
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -45,6 +45,7 @@
 #ifndef __XEN_MM_H__
 #define __XEN_MM_H__
 
+#include <xen/compiler.h>
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
@@ -77,9 +78,12 @@ TYPE_SAFE(unsigned long, pfn);
 #undef pfn_t
 #endif
 
-struct domain;
 struct page_info;
 
+struct domain *__must_check page_get_owner_and_reference(struct page_info *);
+void put_page(struct page_info *);
+int get_page(struct page_info *, struct domain *);
+
 /* Boot-time allocator. Turns into generic allocator after bootstrap. */
 void init_boot_pages(paddr_t ps, paddr_t pe);
 unsigned long alloc_boot_pages(



[-- Attachment #2: pgoar-assert.patch --]
[-- Type: text/plain, Size: 4155 bytes --]

add page_get_owner_and_reference() related ASSERT()s

The function shouldn't return NULL after having obtained a reference,
or else the caller won't know to drop it.

Also its result shouldn't be ignored - if calling code is certain that
a page already has a non-zero refcount, it better ASSERT()s so.

Finally this as well as get_page() and put_page() are required to be
available on all architectures - move the declarations to xen/mm.h.

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

--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,10 +1,8 @@
-#include <xen/config.h>
 #include <xen/lib.h>
 #include <xen/domain_page.h>
+#include <xen/mm.h>
 #include <xen/sched.h>
 #include <asm/current.h>
-
-#include <asm/mm.h>
 #include <asm/guest_access.h>
 
 static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1170,6 +1170,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
 struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
+    struct domain *owner;
 
     do {
         x = y;
@@ -1182,7 +1183,10 @@ struct domain *page_get_owner_and_refere
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    return page_get_owner(page);
+    owner = page_get_owner(page);
+    ASSERT(owner);
+
+    return owner;
 }
 
 void put_page(struct page_info *page)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2039,6 +2039,7 @@ void put_page(struct page_info *page)
 struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
+    struct domain *owner;
 
     do {
         x = y;
@@ -2052,7 +2053,10 @@ struct domain *page_get_owner_and_refere
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    return page_get_owner(page);
+    owner = page_get_owner(page);
+    ASSERT(owner);
+
+    return owner;
 }
 
 
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2245,7 +2245,8 @@ __acquire_grant_for_copy(
     {
         ASSERT(mfn_valid(act->frame));
         *page = mfn_to_page(act->frame);
-        (void)page_get_owner_and_reference(*page);
+        td = page_get_owner_and_reference(*page);
+        ASSERT(td);
     }
 
     act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -275,10 +275,6 @@ static inline void *page_to_virt(const s
     return mfn_to_virt(page_to_mfn(pg));
 }
 
-struct domain *page_get_owner_and_reference(struct page_info *page);
-void put_page(struct page_info *page);
-int  get_page(struct page_info *page, struct domain *domain);
-
 struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
                                     unsigned long flags);
 
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -352,9 +352,6 @@ const unsigned long *get_platform_badpag
 int page_lock(struct page_info *page);
 void page_unlock(struct page_info *page);
 
-struct domain *page_get_owner_and_reference(struct page_info *page);
-void put_page(struct page_info *page);
-int  get_page(struct page_info *page, struct domain *domain);
 void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  put_page_type_preemptible(struct page_info *page);
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -45,6 +45,7 @@
 #ifndef __XEN_MM_H__
 #define __XEN_MM_H__
 
+#include <xen/compiler.h>
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
@@ -77,9 +78,12 @@ TYPE_SAFE(unsigned long, pfn);
 #undef pfn_t
 #endif
 
-struct domain;
 struct page_info;
 
+struct domain *__must_check page_get_owner_and_reference(struct page_info *);
+void put_page(struct page_info *);
+int get_page(struct page_info *, struct domain *);
+
 /* Boot-time allocator. Turns into generic allocator after bootstrap. */
 void init_boot_pages(paddr_t ps, paddr_t pe);
 unsigned long alloc_boot_pages(

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Ping: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Jan Beulich]

Folks on the To list: Ping?

Wei: Now that all patches to go in need your ack, I'm Cc-ing you.

Thanks, Jan

>>> On 24.07.15 at 14:37, <JBeulich@suse.com> wrote:
> The function shouldn't return NULL after having obtained a reference,
> or else the caller won't know to drop it.
> 
> Also its result shouldn't be ignored - if calling code is certain that
> a page already has a non-zero refcount, it better ASSERT()s so.
> 
> Finally this as well as get_page() and put_page() are required to be
> available on all architectures - move the declarations to xen/mm.h.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,8 @@
> -#include <xen/config.h>
>  #include <xen/lib.h>
>  #include <xen/domain_page.h>
> +#include <xen/mm.h>
>  #include <xen/sched.h>
>  #include <asm/current.h>
> -
> -#include <asm/mm.h>
>  #include <asm/guest_access.h>
>  
>  static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1170,6 +1170,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
>  struct domain *page_get_owner_and_reference(struct page_info *page)
>  {
>      unsigned long x, y = page->count_info;
> +    struct domain *owner;
>  
>      do {
>          x = y;
> @@ -1182,7 +1183,10 @@ struct domain *page_get_owner_and_refere
>      }
>      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
>  
> -    return page_get_owner(page);
> +    owner = page_get_owner(page);
> +    ASSERT(owner);
> +
> +    return owner;
>  }
>  
>  void put_page(struct page_info *page)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2039,6 +2039,7 @@ void put_page(struct page_info *page)
>  struct domain *page_get_owner_and_reference(struct page_info *page)
>  {
>      unsigned long x, y = page->count_info;
> +    struct domain *owner;
>  
>      do {
>          x = y;
> @@ -2052,7 +2053,10 @@ struct domain *page_get_owner_and_refere
>      }
>      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
>  
> -    return page_get_owner(page);
> +    owner = page_get_owner(page);
> +    ASSERT(owner);
> +
> +    return owner;
>  }
>  
>  
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2245,7 +2245,8 @@ __acquire_grant_for_copy(
>      {
>          ASSERT(mfn_valid(act->frame));
>          *page = mfn_to_page(act->frame);
> -        (void)page_get_owner_and_reference(*page);
> +        td = page_get_owner_and_reference(*page);
> +        ASSERT(td);
>      }
>  
>      act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -275,10 +275,6 @@ static inline void *page_to_virt(const s
>      return mfn_to_virt(page_to_mfn(pg));
>  }
>  
> -struct domain *page_get_owner_and_reference(struct page_info *page);
> -void put_page(struct page_info *page);
> -int  get_page(struct page_info *page, struct domain *domain);
> -
>  struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
>                                      unsigned long flags);
>  
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -352,9 +352,6 @@ const unsigned long *get_platform_badpag
>  int page_lock(struct page_info *page);
>  void page_unlock(struct page_info *page);
>  
> -struct domain *page_get_owner_and_reference(struct page_info *page);
> -void put_page(struct page_info *page);
> -int  get_page(struct page_info *page, struct domain *domain);
>  void put_page_type(struct page_info *page);
>  int  get_page_type(struct page_info *page, unsigned long type);
>  int  put_page_type_preemptible(struct page_info *page);
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -45,6 +45,7 @@
>  #ifndef __XEN_MM_H__
>  #define __XEN_MM_H__
>  
> +#include <xen/compiler.h>
>  #include <xen/types.h>
>  #include <xen/list.h>
>  #include <xen/spinlock.h>
> @@ -77,9 +78,12 @@ TYPE_SAFE(unsigned long, pfn);
>  #undef pfn_t
>  #endif
>  
> -struct domain;
>  struct page_info;
>  
> +struct domain *__must_check page_get_owner_and_reference(struct page_info 
> *);
> +void put_page(struct page_info *);
> +int get_page(struct page_info *, struct domain *);
> +
>  /* Boot-time allocator. Turns into generic allocator after bootstrap. */
>  void init_boot_pages(paddr_t ps, paddr_t pe);
>  unsigned long alloc_boot_pages(

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Ian Campbell]

On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
> The function shouldn't return NULL after having obtained a reference,
> or else the caller won't know to drop it.

> Also its result shouldn't be ignored - if calling code is certain that
> a page already has a non-zero refcount, it better ASSERT()s so.

How is page_get_owner_and_reference sure the reference count was not zero
on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
itself) Or have I misunderstood the assertion being made here?

Likewise where does the preexisting reference in the grant table case come
from? Perhaps a comment alongside the ASSERT would make it clearer what the
assert was doing to future readers ("/* Reference count must have already
been >=1 due to XXX, so this cannot have failed */") or some such.

> 
> Finally this as well as get_page() and put_page() are required to be
> available on all architectures - move the declarations to xen/mm.h.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,8 @@
> -#include <xen/config.h>
>  #include <xen/lib.h>
>  #include <xen/domain_page.h>
> +#include <xen/mm.h>
>  #include <xen/sched.h>
>  #include <asm/current.h>
> -
> -#include <asm/mm.h>
>  #include <asm/guest_access.h>
>  
>  static unsigned long raw_copy_to_guest_helper(void *to, const void 
> *from,
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1170,6 +1170,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
>  struct domain *page_get_owner_and_reference(struct page_info *page)
>  {
>      unsigned long x, y = page->count_info;
> +    struct domain *owner;
>  
>      do {
>          x = y;
> @@ -1182,7 +1183,10 @@ struct domain *page_get_owner_and_refere
>      }
>      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
>  
> -    return page_get_owner(page);
> +    owner = page_get_owner(page);
> +    ASSERT(owner);
> +
> +    return owner;
>  }
>  
>  void put_page(struct page_info *page)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2039,6 +2039,7 @@ void put_page(struct page_info *page)
>  struct domain *page_get_owner_and_reference(struct page_info *page)
>  {
>      unsigned long x, y = page->count_info;
> +    struct domain *owner;
>  
>      do {
>          x = y;
> @@ -2052,7 +2053,10 @@ struct domain *page_get_owner_and_refere
>      }
>      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
>  
> -    return page_get_owner(page);
> +    owner = page_get_owner(page);
> +    ASSERT(owner);
> +
> +    return owner;
>  }
>  
>  
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2245,7 +2245,8 @@ __acquire_grant_for_copy(
>      {
>          ASSERT(mfn_valid(act->frame));
>          *page = mfn_to_page(act->frame);
> -        (void)page_get_owner_and_reference(*page);
> +        td = page_get_owner_and_reference(*page);
> +        ASSERT(td);
>      }
>  
>      act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -275,10 +275,6 @@ static inline void *page_to_virt(const s
>      return mfn_to_virt(page_to_mfn(pg));
>  }
>  
> -struct domain *page_get_owner_and_reference(struct page_info *page);
> -void put_page(struct page_info *page);
> -int  get_page(struct page_info *page, struct domain *domain);
> -
>  struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
>                                      unsigned long flags);
>  
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -352,9 +352,6 @@ const unsigned long *get_platform_badpag
>  int page_lock(struct page_info *page);
>  void page_unlock(struct page_info *page);
>  
> -struct domain *page_get_owner_and_reference(struct page_info *page);
> -void put_page(struct page_info *page);
> -int  get_page(struct page_info *page, struct domain *domain);
>  void put_page_type(struct page_info *page);
>  int  get_page_type(struct page_info *page, unsigned long type);
>  int  put_page_type_preemptible(struct page_info *page);
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -45,6 +45,7 @@
>  #ifndef __XEN_MM_H__
>  #define __XEN_MM_H__
>  
> +#include <xen/compiler.h>
>  #include <xen/types.h>
>  #include <xen/list.h>
>  #include <xen/spinlock.h>
> @@ -77,9 +78,12 @@ TYPE_SAFE(unsigned long, pfn);
>  #undef pfn_t
>  #endif
>  
> -struct domain;
>  struct page_info;
>  
> +struct domain *__must_check page_get_owner_and_reference(struct 
> page_info *);
> +void put_page(struct page_info *);
> +int get_page(struct page_info *, struct domain *);
> +
>  /* Boot-time allocator. Turns into generic allocator after bootstrap. */
>  void init_boot_pages(paddr_t ps, paddr_t pe);
>  unsigned long alloc_boot_pages(
> 
> 

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Jan Beulich]

>>> On 13.08.15 at 10:50, <ian.campbell@citrix.com> wrote:
> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
>> The function shouldn't return NULL after having obtained a reference,
>> or else the caller won't know to drop it.
> 
>> Also its result shouldn't be ignored - if calling code is certain that
>> a page already has a non-zero refcount, it better ASSERT()s so.
> 
> How is page_get_owner_and_reference sure the reference count was not zero
> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
> itself) Or have I misunderstood the assertion being made here?

There is still a NULL return path left (when the reference couldn't
be obtained, which includes the case where the reference count
was zero). 

> Likewise where does the preexisting reference in the grant table case come
> from? Perhaps a comment alongside the ASSERT would make it clearer what the
> assert was doing to future readers ("/* Reference count must have already
> been >=1 due to XXX, so this cannot have failed */") or some such.

It is my understanding that - as seen in the patch context - the
page's MFN being read from act->frame implies that (together with
the condition of the respective if() the else it's sitting in, namely the
fact that act->pin is non-zero when we get here). It would seem
odd to state in a comment what surrounding code does (I would
agree to the need of a comment if the requirement was satisfied in
a place far away).

Jan

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Tim Deegan]

At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
> > The function shouldn't return NULL after having obtained a reference,
> > or else the caller won't know to drop it.
> 
> > Also its result shouldn't be ignored - if calling code is certain that
> > a page already has a non-zero refcount, it better ASSERT()s so.
> 
> How is page_get_owner_and_reference sure the reference count was not zero
> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
> itself) Or have I misunderstood the assertion being made here?

That path is fine -- if the count was zero, it will return NULL before
trying to get the owner.  But I'm not convinced that the assertion is
correct -- are there really no pages anywhere that have a recount but
no owner?  What about, e.g. shadow pool pages or other uses of
MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
those, then we'd hit the new assertion.

How about unlikely(!owner) path that drops the taken ref instead?

It'd be great to add a comment explaining the semantics of the call,
since the uninformed reader might expect it to take a ref in all
cases.

Cheers,

Tim.

> Likewise where does the preexisting reference in the grant table case come
> from? Perhaps a comment alongside the ASSERT would make it clearer what the
> assert was doing to future readers ("/* Reference count must have already
> been >=1 due to XXX, so this cannot have failed */") or some such.
> 
> > Finally this as well as get_page() and put_page() are required to be
> > available on all architectures - move the declarations to xen/mm.h.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > --- a/xen/arch/arm/guestcopy.c
> > +++ b/xen/arch/arm/guestcopy.c
> > @@ -1,10 +1,8 @@
> > -#include <xen/config.h>
> >  #include <xen/lib.h>
> >  #include <xen/domain_page.h>
> > +#include <xen/mm.h>
> >  #include <xen/sched.h>
> >  #include <asm/current.h>
> > -
> > -#include <asm/mm.h>
> >  #include <asm/guest_access.h>
> >  
> >  static unsigned long raw_copy_to_guest_helper(void *to, const void 
> > *from,
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1170,6 +1170,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
> >  struct domain *page_get_owner_and_reference(struct page_info *page)
> >  {
> >      unsigned long x, y = page->count_info;
> > +    struct domain *owner;
> >  
> >      do {
> >          x = y;
> > @@ -1182,7 +1183,10 @@ struct domain *page_get_owner_and_refere
> >      }
> >      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
> >  
> > -    return page_get_owner(page);
> > +    owner = page_get_owner(page);
> > +    ASSERT(owner);
> > +
> > +    return owner;
> >  }
> >  
> >  void put_page(struct page_info *page)
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2039,6 +2039,7 @@ void put_page(struct page_info *page)
> >  struct domain *page_get_owner_and_reference(struct page_info *page)
> >  {
> >      unsigned long x, y = page->count_info;
> > +    struct domain *owner;
> >  
> >      do {
> >          x = y;
> > @@ -2052,7 +2053,10 @@ struct domain *page_get_owner_and_refere
> >      }
> >      while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
> >  
> > -    return page_get_owner(page);
> > +    owner = page_get_owner(page);
> > +    ASSERT(owner);
> > +
> > +    return owner;
> >  }
> >  
> >  
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -2245,7 +2245,8 @@ __acquire_grant_for_copy(
> >      {
> >          ASSERT(mfn_valid(act->frame));
> >          *page = mfn_to_page(act->frame);
> > -        (void)page_get_owner_and_reference(*page);
> > +        td = page_get_owner_and_reference(*page);
> > +        ASSERT(td);
> >      }
> >  
> >      act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -275,10 +275,6 @@ static inline void *page_to_virt(const s
> >      return mfn_to_virt(page_to_mfn(pg));
> >  }
> >  
> > -struct domain *page_get_owner_and_reference(struct page_info *page);
> > -void put_page(struct page_info *page);
> > -int  get_page(struct page_info *page, struct domain *domain);
> > -
> >  struct page_info *get_page_from_gva(struct domain *d, vaddr_t va,
> >                                      unsigned long flags);
> >  
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -352,9 +352,6 @@ const unsigned long *get_platform_badpag
> >  int page_lock(struct page_info *page);
> >  void page_unlock(struct page_info *page);
> >  
> > -struct domain *page_get_owner_and_reference(struct page_info *page);
> > -void put_page(struct page_info *page);
> > -int  get_page(struct page_info *page, struct domain *domain);
> >  void put_page_type(struct page_info *page);
> >  int  get_page_type(struct page_info *page, unsigned long type);
> >  int  put_page_type_preemptible(struct page_info *page);
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -45,6 +45,7 @@
> >  #ifndef __XEN_MM_H__
> >  #define __XEN_MM_H__
> >  
> > +#include <xen/compiler.h>
> >  #include <xen/types.h>
> >  #include <xen/list.h>
> >  #include <xen/spinlock.h>
> > @@ -77,9 +78,12 @@ TYPE_SAFE(unsigned long, pfn);
> >  #undef pfn_t
> >  #endif
> >  
> > -struct domain;
> >  struct page_info;
> >  
> > +struct domain *__must_check page_get_owner_and_reference(struct 
> > page_info *);
> > +void put_page(struct page_info *);
> > +int get_page(struct page_info *, struct domain *);
> > +
> >  /* Boot-time allocator. Turns into generic allocator after bootstrap. */
> >  void init_boot_pages(paddr_t ps, paddr_t pe);
> >  unsigned long alloc_boot_pages(
> > 
> > 

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Jan Beulich]

>>> On 13.08.15 at 11:31, <tim@xen.org> wrote:
> At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
>> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
>> > The function shouldn't return NULL after having obtained a reference,
>> > or else the caller won't know to drop it.
>> 
>> > Also its result shouldn't be ignored - if calling code is certain that
>> > a page already has a non-zero refcount, it better ASSERT()s so.
>> 
>> How is page_get_owner_and_reference sure the reference count was not zero
>> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
>> itself) Or have I misunderstood the assertion being made here?
> 
> That path is fine -- if the count was zero, it will return NULL before
> trying to get the owner.  But I'm not convinced that the assertion is
> correct -- are there really no pages anywhere that have a recount but
> no owner?  What about, e.g. shadow pool pages or other uses of
> MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
> those, then we'd hit the new assertion.
> 
> How about unlikely(!owner) path that drops the taken ref instead?

That would be dead code - a page the ref count of wasn't zero
prior to the function taking an extra ref shouldn't be unowned.
If the new ASSERT() ever triggers we know we have a bug
elsewhere (which would be hidden if we did what you suggest).

> It'd be great to add a comment explaining the semantics of the call,
> since the uninformed reader might expect it to take a ref in all
> cases.

It would seem to me that this ought to be implicit from the
__must_check that the patch adds, as otherwise there would
be no (reasonable, i.e. leaving aside refcount overflow) way
to get back NULL here. But yes, if you think this is _too_
implicit, I could certainly add a comment to the declaration.

Jan

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Ian Campbell]

On Thu, 2015-08-13 at 03:31 -0600, Jan Beulich wrote:
> > 
> > > > On 13.08.15 at 10:50, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
> > > The function shouldn't return NULL after having obtained a reference,
> > > or else the caller won't know to drop it.
> > 
> > > Also its result shouldn't be ignored - if calling code is certain 
> > > that
> > > a page already has a non-zero refcount, it better ASSERT()s so.
> > 
> > How is page_get_owner_and_reference sure the reference count was not 
> > zero
> > on entry? (WRT the call to page_get_owner in 
> > page_get_owner_and_reference
> > itself) Or have I misunderstood the assertion being made here?
> 
> There is still a NULL return path left (when the reference couldn't
> be obtained, which includes the case where the reference count
> was zero). 

Thanks, I missed that big comment somehow.

> > Likewise where does the preexisting reference in the grant table case 
> > come
> > from? Perhaps a comment alongside the ASSERT would make it clearer what 
> > the
> > assert was doing to future readers ("/* Reference count must have 
> > already
> > been >=1 due to XXX, so this cannot have failed */") or some such.
> 
> It is my understanding that - as seen in the patch context - the
> page's MFN being read from act->frame implies that (together with
> the condition of the respective if() the else it's sitting in, namely the
> fact that act->pin is non-zero when we get here). It would seem
> odd to state in a comment what surrounding code does (I would
> agree to the need of a comment if the requirement was satisfied in
> a place far away).

The fact that act->frame/act->pin together somehow imply a reference count
(taken elsewhere?) is not all that obvious, at least to me. A comment to
that effect was exactly what I was after.

Ian.

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Tim Deegan]

At 03:46 -0600 on 13 Aug (1439437600), Jan Beulich wrote:
> >>> On 13.08.15 at 11:31, <tim@xen.org> wrote:
> > At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
> >> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
> >> > The function shouldn't return NULL after having obtained a reference,
> >> > or else the caller won't know to drop it.
> >> 
> >> > Also its result shouldn't be ignored - if calling code is certain that
> >> > a page already has a non-zero refcount, it better ASSERT()s so.
> >> 
> >> How is page_get_owner_and_reference sure the reference count was not zero
> >> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
> >> itself) Or have I misunderstood the assertion being made here?
> > 
> > That path is fine -- if the count was zero, it will return NULL before
> > trying to get the owner.  But I'm not convinced that the assertion is
> > correct -- are there really no pages anywhere that have a recount but
> > no owner?  What about, e.g. shadow pool pages or other uses of
> > MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
> > those, then we'd hit the new assertion.
> > 
> > How about unlikely(!owner) path that drops the taken ref instead?
> 
> That would be dead code - a page the ref count of wasn't zero
> prior to the function taking an extra ref shouldn't be unowned.

Right.  That being the case, Reviewed-by: Tim Deegan <tim@xen.org>.

Cheers,

Tim.

Re: [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Jan Beulich]

>>> On 13.08.15 at 12:12, <tim@xen.org> wrote:
> At 03:46 -0600 on 13 Aug (1439437600), Jan Beulich wrote:
>> >>> On 13.08.15 at 11:31, <tim@xen.org> wrote:
>> > At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
>> >> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
>> >> > The function shouldn't return NULL after having obtained a reference,
>> >> > or else the caller won't know to drop it.
>> >> 
>> >> > Also its result shouldn't be ignored - if calling code is certain that
>> >> > a page already has a non-zero refcount, it better ASSERT()s so.
>> >> 
>> >> How is page_get_owner_and_reference sure the reference count was not zero
>> >> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
>> >> itself) Or have I misunderstood the assertion being made here?
>> > 
>> > That path is fine -- if the count was zero, it will return NULL before
>> > trying to get the owner.  But I'm not convinced that the assertion is
>> > correct -- are there really no pages anywhere that have a recount but
>> > no owner?  What about, e.g. shadow pool pages or other uses of
>> > MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
>> > those, then we'd hit the new assertion.
>> > 
>> > How about unlikely(!owner) path that drops the taken ref instead?
>> 
>> That would be dead code - a page the ref count of wasn't zero
>> prior to the function taking an extra ref shouldn't be unowned.
> 
> Right.  That being the case, Reviewed-by: Tim Deegan <tim@xen.org>.

Btw, as I was about to add a comment as you asked for along with
adding the one Ian wants I realized that
page_get_owner_and_reference() is a helper of get_page(), i.e.
their behavior of not always acquiring a reference is the same.
Which - to me - makes adding such a comment pretty pointless.

Jan