[PATCH for-4.9 0/3] Improvements to ERR_PTR() infrastructure

[PATCH for-4.9 0/3] Improvements to ERR_PTR() infrastructure

[Andrew Cooper]

One XenServer issue I am looking in to requires improvements to the memory
allocation logic, mostly to avoid losing error information internally.

As a result, I am choosing to make these ERR_PTR() improvements a prerequisite
to avoid accidentally introducing further issues.

Andrew Cooper (3):
  xen/err: Use static inlines and boolean types
  xen/err: Support an optional per-arch slide for the error region
  xen/x86: Use non-canonical pointers for ERR_PTR() errors

 xen/arch/x86/Kconfig         | 18 ++++++++++++++++++
 xen/include/asm-x86/config.h |  8 ++++++++
 xen/include/xen/err.h        | 23 ++++++++++++++++++-----
 3 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] xen/err: Use static inlines and boolean types

[Andrew Cooper]

IS_ERR() and IS_ERR_OR_NULL() both return boolean values.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/xen/err.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
index 2f29b57..ef77992 100644
--- a/xen/include/xen/err.h
+++ b/xen/include/xen/err.h
@@ -2,6 +2,7 @@
 #define __XEN_ERR_H__
 
 #include <xen/compiler.h>
+#include <xen/stdbool.h>
 #include <xen/errno.h>
 
 /*
@@ -14,7 +15,10 @@
  */
 #define MAX_ERRNO	4095
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+static inline bool IS_ERR_VALUE(unsigned long x)
+{
+	return x >= (unsigned long)-MAX_ERRNO;
+}
 
 static inline void *__must_check ERR_PTR(long error)
 {
@@ -26,12 +30,12 @@ static inline long __must_check PTR_ERR(const void *ptr)
 	return (long)ptr;
 }
 
-static inline long __must_check IS_ERR(const void *ptr)
+static inline bool __must_check IS_ERR(const void *ptr)
 {
 	return IS_ERR_VALUE((unsigned long)ptr);
 }
 
-static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
+static inline bool __must_check IS_ERR_OR_NULL(const void *ptr)
 {
 	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
 }
-- 
2.1.4


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

[PATCH 2/3] xen/err: Support an optional per-arch slide for the error region

[Andrew Cooper]

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/xen/err.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
index ef77992..bea7fa3 100644
--- a/xen/include/xen/err.h
+++ b/xen/include/xen/err.h
@@ -15,19 +15,28 @@
  */
 #define MAX_ERRNO	4095
 
+/*
+ * Individual architectures may wish to slide the error region away from its
+ * default position at the very top of virtual address space.  (e.g. if Xen
+ * doesn't own the range).
+ */
+#ifndef ARCH_ERR_PTR_SLIDE
+#define ARCH_ERR_PTR_SLIDE 0
+#endif
+
 static inline bool IS_ERR_VALUE(unsigned long x)
 {
-	return x >= (unsigned long)-MAX_ERRNO;
+	return (x + ARCH_ERR_PTR_SLIDE) >= (unsigned long)-MAX_ERRNO;
 }
 
 static inline void *__must_check ERR_PTR(long error)
 {
-	return (void *)error;
+	return (void *)((unsigned long)error - ARCH_ERR_PTR_SLIDE);
 }
 
 static inline long __must_check PTR_ERR(const void *ptr)
 {
-	return (long)ptr;
+	return (long)((unsigned long)ptr + ARCH_ERR_PTR_SLIDE);
 }
 
 static inline bool __must_check IS_ERR(const void *ptr)
-- 
2.1.4


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

[PATCH 3/3] xen/x86: Use non-canonical pointers for ERR_PTR() errors

[Andrew Cooper]

The top of the virutal address space is owned by 64bit PV kernels.  Code which
fails to correctly check an ERR_PTR() value might follow the pointer into
guest space.

Mitigate this risk by sliding the ERR_PTR() error range into the non-canonical
region.

As this comes with a small overhead, and isn't necessary if 64bit PV guests
aren't used, provide a Kconfig opt-out for power users.

As an example, ERR_PTR(-EINVAL) has the value 0xbad0eee0ffffffea.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

Ideally, the constant would read "bad err", but r's are hard in hex.
Alternative constant suggestions welcome.
---
 xen/arch/x86/Kconfig         | 18 ++++++++++++++++++
 xen/include/asm-x86/config.h |  8 ++++++++
 2 files changed, 26 insertions(+)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 96ca2bf..9aa9d3f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -89,6 +89,24 @@ config TBOOT
 	  Technology (TXT)
 
 	  If unsure, say Y.
+
+config UNSAFE_ERRPTR
+       def_bool n
+       prompt "Unsafe ERR_PTR() constants" if EXPERT = "y"
+       ---help---
+         The ERR_PTR() infrastructure by default uses the top of virtual
+         address space for error information, but with 64bit PV guests, this
+         range belongs to the guest kernel.
+
+         Xen by default slides the error range elsewhere in virtual address
+         space, but this comes at the expense of a few extra instructions of
+         calculations.
+
+         If that overhead is too much, this option is safe to enable if no
+         64bit PV guests are in use, or all 64bit PV guests are fully trusted.
+
+         If unsure, say N.
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 6fd84e7..2997ee3 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -87,6 +87,14 @@
 #define LIST_POISON1  ((void *)0x0100100100100100UL)
 #define LIST_POISON2  ((void *)0x0200200200200200UL)
 
+#if !defined(NDEBUG) || !defined(CONFIG_UNSAFE_ERRPTR)
+/*
+ * Always use safe pointers in debug builds.  Use safe pointers in release
+ * builds unless the user explicitly opts out.
+ */
+#define ARCH_ERR_PTR_SLIDE (-(unsigned long)0xbad0eee100000000ull)
+#endif
+
 #ifndef __ASSEMBLY__
 extern unsigned long trampoline_phys;
 #define bootsym_phys(sym)                                 \
-- 
2.1.4


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

Re: [PATCH 1/3] xen/err: Use static inlines and boolean types

[Jan Beulich]

>>> On 01.11.16 at 11:46, <andrew.cooper3@citrix.com> wrote:
> IS_ERR() and IS_ERR_OR_NULL() both return boolean values.
> 
> No functional change.

I'm definitely fine with this part. However, ...

> @@ -14,7 +15,10 @@
>   */
>  #define MAX_ERRNO	4095
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +static inline bool IS_ERR_VALUE(unsigned long x)
> +{
> +	return x >= (unsigned long)-MAX_ERRNO;
> +}

... for this one I'd like us to consider following Linux commit
aa00edc128 instead, which I don't think can be achieved by an
inline function alone.

Jan


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

Re: [PATCH 2/3] xen/err: Support an optional per-arch slide for the error region

[Jan Beulich]

>>> On 01.11.16 at 11:46, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/err.h
> +++ b/xen/include/xen/err.h
> @@ -15,19 +15,28 @@
>   */
>  #define MAX_ERRNO	4095
>  
> +/*
> + * Individual architectures may wish to slide the error region away from its
> + * default position at the very top of virtual address space.  (e.g. if Xen
> + * doesn't own the range).
> + */
> +#ifndef ARCH_ERR_PTR_SLIDE
> +#define ARCH_ERR_PTR_SLIDE 0
> +#endif
> +
>  static inline bool IS_ERR_VALUE(unsigned long x)
>  {
> -	return x >= (unsigned long)-MAX_ERRNO;
> +	return (x + ARCH_ERR_PTR_SLIDE) >= (unsigned long)-MAX_ERRNO;
>  }
>  
>  static inline void *__must_check ERR_PTR(long error)
>  {
> -	return (void *)error;
> +	return (void *)((unsigned long)error - ARCH_ERR_PTR_SLIDE);

I don't see the need for the inner cast here and ...

>  }
>  
>  static inline long __must_check PTR_ERR(const void *ptr)
>  {
> -	return (long)ptr;
> +	return (long)((unsigned long)ptr + ARCH_ERR_PTR_SLIDE);

... the outer one here.

And then of course this change is pointless without patch 3, which
I have reservations against (will reply there).

Jan


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

Re: [PATCH 3/3] xen/x86: Use non-canonical pointers for ERR_PTR() errors

[Jan Beulich]

>>> On 01.11.16 at 11:46, <andrew.cooper3@citrix.com> wrote:
> The top of the virutal address space is owned by 64bit PV kernels.  Code which
> fails to correctly check an ERR_PTR() value might follow the pointer into
> guest space.
> 
> Mitigate this risk by sliding the ERR_PTR() error range into the non-canonical
> region.
> 
> As this comes with a small overhead, and isn't necessary if 64bit PV guests
> aren't used, provide a Kconfig opt-out for power users.

And it's this overhead which I dislike. Not properly handling the values
here is just like not properly checking for NULL, and I don't think you
mean to propose to give NULL a value other than numeric zero?

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -87,6 +87,14 @@
>  #define LIST_POISON1  ((void *)0x0100100100100100UL)
>  #define LIST_POISON2  ((void *)0x0200200200200200UL)
>  
> +#if !defined(NDEBUG) || !defined(CONFIG_UNSAFE_ERRPTR)

With that the config option should depend on !DEBUG I would say,
or default to DEBUG and have its prompt hidden when DEBUG
(simplifying the expression above).

> +/*
> + * Always use safe pointers in debug builds.  Use safe pointers in release
> + * builds unless the user explicitly opts out.
> + */
> +#define ARCH_ERR_PTR_SLIDE (-(unsigned long)0xbad0eee100000000ull)

What good does casting the constant to unsigned long? Did you
perhaps mean to use just an ul suffix and cast to signed long?

Jan


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