[PATCH] mm: option to _always_ scrub freed domheap pages

[PATCH] mm: option to _always_ scrub freed domheap pages

[Eslam Elnikety]

Give the administrator further control on when to scrub domheap pages by adding
an option to always scrub. This is a safety feature that, when enabled,
prevents a (buggy) domain from leaking secrets if it accidentally frees a page
without proper scrubbing.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
---
 docs/misc/xen-command-line.pandoc |  8 ++++++++
 xen/common/page_alloc.c           | 11 +++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 7dcb22932a..5a92949c5a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
 enough. Setting this to a high value may cause boot failure, particularly if
 the NMI watchdog is also enabled.
 
+### scrub_domheap
+> `= <boolean>`
+
+> Default: `false`
+
+Scrub domains' freed pages. This is a safety net against a (buggy) domain
+accidentally leaking secrets by releasing pages without proper sanitization.
+
 ### clocksource (x86)
 > `= pit | hpet | acpi | tsc`
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..678a00ac9b 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
 static unsigned long __initdata opt_bootscrub_chunk = MB(128);
 size_param("bootscrub_chunk", opt_bootscrub_chunk);
 
+/*
+ * scrub_domheap -> Domheap pages are scrubbed when freed
+ */
+static bool_t opt_scrub_domheap = 0;
+boolean_param("scrub_domheap", opt_scrub_domheap);
+
 #ifdef CONFIG_SCRUB_DEBUG
 static bool __read_mostly scrub_debug;
 #else
@@ -2378,9 +2384,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
             /*
              * Normally we expect a domain to clear pages before freeing them,
              * if it cares about the secrecy of their contents. However, after
-             * a domain has died we assume responsibility for erasure.
+             * a domain has died we assume responsibility for erasure. We do
+             * scrub regardless if option scrub_domheap is set.
              */
-            scrub = d->is_dying || scrub_debug;
+            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
         }
         else
         {
-- 
2.15.3.AMZN


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

[Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Eslam Elnikety]

Give the administrator further control on when to scrub domheap pages by adding
an option to always scrub. This is a safety feature that, when enabled,
prevents a (buggy) domain from leaking secrets if it accidentally frees a page
without proper scrubbing.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
---
 docs/misc/xen-command-line.pandoc |  8 ++++++++
 xen/common/page_alloc.c           | 11 +++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 7dcb22932a..5a92949c5a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
 enough. Setting this to a high value may cause boot failure, particularly if
 the NMI watchdog is also enabled.
 
+### scrub_domheap
+> `= <boolean>`
+
+> Default: `false`
+
+Scrub domains' freed pages. This is a safety net against a (buggy) domain
+accidentally leaking secrets by releasing pages without proper sanitization.
+
 ### clocksource (x86)
 > `= pit | hpet | acpi | tsc`
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..678a00ac9b 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
 static unsigned long __initdata opt_bootscrub_chunk = MB(128);
 size_param("bootscrub_chunk", opt_bootscrub_chunk);
 
+/*
+ * scrub_domheap -> Domheap pages are scrubbed when freed
+ */
+static bool_t opt_scrub_domheap = 0;
+boolean_param("scrub_domheap", opt_scrub_domheap);
+
 #ifdef CONFIG_SCRUB_DEBUG
 static bool __read_mostly scrub_debug;
 #else
@@ -2378,9 +2384,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
             /*
              * Normally we expect a domain to clear pages before freeing them,
              * if it cares about the secrecy of their contents. However, after
-             * a domain has died we assume responsibility for erasure.
+             * a domain has died we assume responsibility for erasure. We do
+             * scrub regardless if option scrub_domheap is set.
              */
-            scrub = d->is_dying || scrub_debug;
+            scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
         }
         else
         {
-- 
2.15.3.AMZN


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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[George Dunlap]

On 5/6/19 1:46 PM, Eslam Elnikety wrote:
> Give the administrator further control on when to scrub domheap pages by adding
> an option to always scrub. This is a safety feature that, when enabled,
> prevents a (buggy) domain from leaking secrets if it accidentally frees a page
> without proper scrubbing.
> 
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>

Now that I think about it -- Andy, isn't there a patch in the XenServer
patchqueue to enable scrubbing by default?

I'm wondering if this should default to 'true', and people who really
want the extra performance should turn it off.

Only one other minor comment:

> ---
>  docs/misc/xen-command-line.pandoc |  8 ++++++++
>  xen/common/page_alloc.c           | 11 +++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 7dcb22932a..5a92949c5a 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
>  enough. Setting this to a high value may cause boot failure, particularly if
>  the NMI watchdog is also enabled.
>  
> +### scrub_domheap
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Scrub domains' freed pages. This is a safety net against a (buggy) domain
> +accidentally leaking secrets by releasing pages without proper sanitization.
> +
>  ### clocksource (x86)
>  > `= pit | hpet | acpi | tsc`
>  
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index be44158033..678a00ac9b 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>  
> +/*
> + * scrub_domheap -> Domheap pages are scrubbed when freed
> + */
> +static bool_t opt_scrub_domheap = 0;
> +boolean_param("scrub_domheap", opt_scrub_domheap);

I'm sure Jan will request this to be 'scrub-domheap' instead (not using
'_' when you can use '-').

Otherwise this looks good to me:

Acked-by: George Dunlap <george.dunlap@citrix.com>

I think both of these could probably be fixed up on check-in.

 -George

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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[George Dunlap]

On 5/6/19 1:46 PM, Eslam Elnikety wrote:
> Give the administrator further control on when to scrub domheap pages by adding
> an option to always scrub. This is a safety feature that, when enabled,
> prevents a (buggy) domain from leaking secrets if it accidentally frees a page
> without proper scrubbing.
> 
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>

Now that I think about it -- Andy, isn't there a patch in the XenServer
patchqueue to enable scrubbing by default?

I'm wondering if this should default to 'true', and people who really
want the extra performance should turn it off.

Only one other minor comment:

> ---
>  docs/misc/xen-command-line.pandoc |  8 ++++++++
>  xen/common/page_alloc.c           | 11 +++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 7dcb22932a..5a92949c5a 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
>  enough. Setting this to a high value may cause boot failure, particularly if
>  the NMI watchdog is also enabled.
>  
> +### scrub_domheap
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Scrub domains' freed pages. This is a safety net against a (buggy) domain
> +accidentally leaking secrets by releasing pages without proper sanitization.
> +
>  ### clocksource (x86)
>  > `= pit | hpet | acpi | tsc`
>  
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index be44158033..678a00ac9b 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>  
> +/*
> + * scrub_domheap -> Domheap pages are scrubbed when freed
> + */
> +static bool_t opt_scrub_domheap = 0;
> +boolean_param("scrub_domheap", opt_scrub_domheap);

I'm sure Jan will request this to be 'scrub-domheap' instead (not using
'_' when you can use '-').

Otherwise this looks good to me:

Acked-by: George Dunlap <george.dunlap@citrix.com>

I think both of these could probably be fixed up on check-in.

 -George

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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[Wei Liu]

On Tue, May 07, 2019 at 10:55:51AM +0100, George Dunlap wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
> > Give the administrator further control on when to scrub domheap pages by adding
> > an option to always scrub. This is a safety feature that, when enabled,
> > prevents a (buggy) domain from leaking secrets if it accidentally frees a page
> > without proper scrubbing.
> > 
> > Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> 
> Now that I think about it -- Andy, isn't there a patch in the XenServer
> patchqueue to enable scrubbing by default?
> 
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.
> 
> Only one other minor comment:
> 
> > ---
> >  docs/misc/xen-command-line.pandoc |  8 ++++++++
> >  xen/common/page_alloc.c           | 11 +++++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> > index 7dcb22932a..5a92949c5a 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
> >  enough. Setting this to a high value may cause boot failure, particularly if
> >  the NMI watchdog is also enabled.
> >  
> > +### scrub_domheap
> > +> `= <boolean>`
> > +
> > +> Default: `false`
> > +
> > +Scrub domains' freed pages. This is a safety net against a (buggy) domain
> > +accidentally leaking secrets by releasing pages without proper sanitization.
> > +
> >  ### clocksource (x86)
> >  > `= pit | hpet | acpi | tsc`
> >  
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index be44158033..678a00ac9b 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
> >  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
> >  size_param("bootscrub_chunk", opt_bootscrub_chunk);
> >  
> > +/*
> > + * scrub_domheap -> Domheap pages are scrubbed when freed
> > + */
> > +static bool_t opt_scrub_domheap = 0;

Please change bool_t to bool and 0 to false while you're at it. :-)

Wei.

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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Wei Liu]

On Tue, May 07, 2019 at 10:55:51AM +0100, George Dunlap wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
> > Give the administrator further control on when to scrub domheap pages by adding
> > an option to always scrub. This is a safety feature that, when enabled,
> > prevents a (buggy) domain from leaking secrets if it accidentally frees a page
> > without proper scrubbing.
> > 
> > Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> 
> Now that I think about it -- Andy, isn't there a patch in the XenServer
> patchqueue to enable scrubbing by default?
> 
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.
> 
> Only one other minor comment:
> 
> > ---
> >  docs/misc/xen-command-line.pandoc |  8 ++++++++
> >  xen/common/page_alloc.c           | 11 +++++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> > index 7dcb22932a..5a92949c5a 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
> >  enough. Setting this to a high value may cause boot failure, particularly if
> >  the NMI watchdog is also enabled.
> >  
> > +### scrub_domheap
> > +> `= <boolean>`
> > +
> > +> Default: `false`
> > +
> > +Scrub domains' freed pages. This is a safety net against a (buggy) domain
> > +accidentally leaking secrets by releasing pages without proper sanitization.
> > +
> >  ### clocksource (x86)
> >  > `= pit | hpet | acpi | tsc`
> >  
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index be44158033..678a00ac9b 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
> >  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
> >  size_param("bootscrub_chunk", opt_bootscrub_chunk);
> >  
> > +/*
> > + * scrub_domheap -> Domheap pages are scrubbed when freed
> > + */
> > +static bool_t opt_scrub_domheap = 0;

Please change bool_t to bool and 0 to false while you're at it. :-)

Wei.

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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[Jan Beulich]

>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>[...]
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.

Why would we want to cater for buggy guests by default?

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
>>  enough. Setting this to a high value may cause boot failure, particularly if
>>  the NMI watchdog is also enabled.
>>  
>> +### scrub_domheap
>> +> `= <boolean>`
>> +
>> +> Default: `false`
>> +
>> +Scrub domains' freed pages. This is a safety net against a (buggy) domain
>> +accidentally leaking secrets by releasing pages without proper sanitization.
>> +
>>  ### clocksource (x86)
>>  > `= pit | hpet | acpi | tsc`

The entries here are alphabetically sorted, so the addition wants to
move down quite a bit.

>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>  
>> +/*
>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>> + */
>> +static bool_t opt_scrub_domheap = 0;
>> +boolean_param("scrub_domheap", opt_scrub_domheap);
> 
> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
> '_' when you can use '-').

Indeed, plus use "bool", drop the pointless initializer, and correct
the style of the (single line) comment.

Jan



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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Jan Beulich]

>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>[...]
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.

Why would we want to cater for buggy guests by default?

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
>>  enough. Setting this to a high value may cause boot failure, particularly if
>>  the NMI watchdog is also enabled.
>>  
>> +### scrub_domheap
>> +> `= <boolean>`
>> +
>> +> Default: `false`
>> +
>> +Scrub domains' freed pages. This is a safety net against a (buggy) domain
>> +accidentally leaking secrets by releasing pages without proper sanitization.
>> +
>>  ### clocksource (x86)
>>  > `= pit | hpet | acpi | tsc`

The entries here are alphabetically sorted, so the addition wants to
move down quite a bit.

>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>  
>> +/*
>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>> + */
>> +static bool_t opt_scrub_domheap = 0;
>> +boolean_param("scrub_domheap", opt_scrub_domheap);
> 
> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
> '_' when you can use '-').

Indeed, plus use "bool", drop the pointless initializer, and correct
the style of the (single line) comment.

Jan



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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[Jan Beulich]

>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote:
>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>>  
>>> +/*
>>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>>> + */
>>> +static bool_t opt_scrub_domheap = 0;
>>> +boolean_param("scrub_domheap", opt_scrub_domheap);
>> 
>> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
>> '_' when you can use '-').
> 
> Indeed, plus use "bool", drop the pointless initializer, and correct
> the style of the (single line) comment.

And use __read_mostly.

Jan



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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Jan Beulich]

>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote:
>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>>  static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>>  size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>>  
>>> +/*
>>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>>> + */
>>> +static bool_t opt_scrub_domheap = 0;
>>> +boolean_param("scrub_domheap", opt_scrub_domheap);
>> 
>> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
>> '_' when you can use '-').
> 
> Indeed, plus use "bool", drop the pointless initializer, and correct
> the style of the (single line) comment.

And use __read_mostly.

Jan



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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[Elnikety, Eslam]

> On 7. May 2019, at 12:06, Jan Beulich <jbeulich@suse.com> wrote:
> 
>>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote:
>>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>>> static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>>> size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>>> 
>>>> +/*
>>>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>>>> + */
>>>> +static bool_t opt_scrub_domheap = 0;
>>>> +boolean_param("scrub_domheap", opt_scrub_domheap);
>>> 
>>> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
>>> '_' when you can use '-').
>> 
>> Indeed, plus use "bool", drop the pointless initializer, and correct
>> the style of the (single line) comment.
> 
> And use __read_mostly.
> 
> Jan
> 
> 

Thanks for all the comments on this thread. v2 should take care of all those comments.

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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Elnikety, Eslam]

> On 7. May 2019, at 12:06, Jan Beulich <jbeulich@suse.com> wrote:
> 
>>>> On 07.05.19 at 12:03, <JBeulich@suse.com> wrote:
>>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
>>>> static unsigned long __initdata opt_bootscrub_chunk = MB(128);
>>>> size_param("bootscrub_chunk", opt_bootscrub_chunk);
>>>> 
>>>> +/*
>>>> + * scrub_domheap -> Domheap pages are scrubbed when freed
>>>> + */
>>>> +static bool_t opt_scrub_domheap = 0;
>>>> +boolean_param("scrub_domheap", opt_scrub_domheap);
>>> 
>>> I'm sure Jan will request this to be 'scrub-domheap' instead (not using
>>> '_' when you can use '-').
>> 
>> Indeed, plus use "bool", drop the pointless initializer, and correct
>> the style of the (single line) comment.
> 
> And use __read_mostly.
> 
> Jan
> 
> 

Thanks for all the comments on this thread. v2 should take care of all those comments.

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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[George Dunlap]

On 5/7/19 11:03 AM, Jan Beulich wrote:
>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>> [...]
>> I'm wondering if this should default to 'true', and people who really
>> want the extra performance should turn it off.
> 
> Why would we want to cater for buggy guests by default?

Because one of the big selling points for Xen is that it is more secure;
and one of the points of virtualization is to run guests for which you
have no option of changing or upgrading.  Having an extra safety catch
to prevent loss of data *even in the case of buggy guests* on by default
is on-brand for us.

That said, the suggestion was predicated on my rather vague impression
that XenServer has been doing this by default for years now, and thus
has been proven to not be terribly problematic performance-wise.  If
that's not the case, then off-by-default might be a better option.

 -George

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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[George Dunlap]

On 5/7/19 11:03 AM, Jan Beulich wrote:
>>>> On 07.05.19 at 11:55, <george.dunlap@citrix.com> wrote:
>> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>> [...]
>> I'm wondering if this should default to 'true', and people who really
>> want the extra performance should turn it off.
> 
> Why would we want to cater for buggy guests by default?

Because one of the big selling points for Xen is that it is more secure;
and one of the points of virtualization is to run guests for which you
have no option of changing or upgrading.  Having an extra safety catch
to prevent loss of data *even in the case of buggy guests* on by default
is on-brand for us.

That said, the suggestion was predicated on my rather vague impression
that XenServer has been doing this by default for years now, and thus
has been proven to not be terribly problematic performance-wise.  If
that's not the case, then off-by-default might be a better option.

 -George

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

Re: [PATCH] mm: option to _always_ scrub freed domheap pages

[Andrew Cooper]

On 07/05/2019 10:55, George Dunlap wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>> Give the administrator further control on when to scrub domheap pages by adding
>> an option to always scrub. This is a safety feature that, when enabled,
>> prevents a (buggy) domain from leaking secrets if it accidentally frees a page
>> without proper scrubbing.
>>
>> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> Now that I think about it -- Andy, isn't there a patch in the XenServer
> patchqueue to enable scrubbing by default?
>
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.

https://github.com/xenserver/xen.pg/blob/XS-8.0.x/master/0001-cc-memory-scrubbing.patch

We couldn't measure a performance difference with unconditional
scrubbing, so chose to do without an extra parameter to tweak.

~Andrew

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

Re: [Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages

[Andrew Cooper]

On 07/05/2019 10:55, George Dunlap wrote:
> On 5/6/19 1:46 PM, Eslam Elnikety wrote:
>> Give the administrator further control on when to scrub domheap pages by adding
>> an option to always scrub. This is a safety feature that, when enabled,
>> prevents a (buggy) domain from leaking secrets if it accidentally frees a page
>> without proper scrubbing.
>>
>> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> Now that I think about it -- Andy, isn't there a patch in the XenServer
> patchqueue to enable scrubbing by default?
>
> I'm wondering if this should default to 'true', and people who really
> want the extra performance should turn it off.

https://github.com/xenserver/xen.pg/blob/XS-8.0.x/master/0001-cc-memory-scrubbing.patch

We couldn't measure a performance difference with unconditional
scrubbing, so chose to do without an extra parameter to tweak.

~Andrew

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