[PATCH 0/5] xen/domain: Cleanup to the domain_create() error paths

[PATCH 0/5] xen/domain: Cleanup to the domain_create() error paths

[Andrew Cooper]

This is the start of a large amount of cleanup work to eventually allow for
the removal of XEN_DOMCTL_max_cpus hypercall.

The work to do is:

  1) Make the domain destruction path fully idempotent, and use instead of the
     ad-hoc cleanup in each of the various create functions.
  2) Do the same for the vcpu create/destroy path (which is in a far worse
     mess).

The arch-specific ARM code is all idempotent, but the common and x86 code has
a long way to go.

With this done, we should be able to cleanly unwind from any failure at any
point during domain creation, including when moving the vcpu allocation loop
into domain_create().

Andrew Cooper (5):
  xen/domain: Prepare data for is_{pv,hvm}_domain() as early as possible
  xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()
  xen/domain: Call lock_profile_deregister_struct() from common code
  xen/domain: Fold xsm_free_security_domain() paths together
  xen/domain: Make rangeset_domain_destroy() idempotent

 xen/arch/x86/domain.c |  1 -
 xen/common/domain.c   | 62 +++++++++++++++++++++++++++++++--------------------
 xen/common/rangeset.c |  3 +++
 3 files changed, 41 insertions(+), 25 deletions(-)

-- 
2.1.4


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

[PATCH 1/5] xen/domain: Prepare data for is_{pv, hvm}_domain() as early as possible

[Andrew Cooper]

Given two subtle failures from getting this wrong before, and more cleanup on
the way, move the setting of d->guest_type as early as possible.

Note that despite moving the assignment of d->guest_type outside of the
is_idle_domain(d) check, it still behaves the same.  Previously, system
domains had no direct assignment of d->guest_type and behaved as PV guests
because guest_type_pv has the value 0.

While tidying up the predicate, leave a comment refering to
is_system_domain(), and move the associated ASSERT() to be beside the
asignment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/domain.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 256c59a..43ab926 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -272,8 +272,12 @@ struct domain *domain_create(domid_t domid,
     if ( (d = alloc_domain_struct()) == NULL )
         return ERR_PTR(-ENOMEM);
 
+    /* Sort out our idea of is_system_domain(). */
     d->domain_id = domid;
 
+    /* Debug sanity. */
+    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
+
     /* Sort out our idea of is_control_domain(). */
     d->is_privileged = is_priv;
 
@@ -289,8 +293,11 @@ struct domain *domain_create(domid_t domid,
         hardware_domain = d;
     }
 
-    /* Debug sanity. */
-    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
+    /* Sort out our idea of is_{pv,hvm}_domain(). */
+    if ( config && (config->flags & XEN_DOMCTL_CDF_hvm_guest) )
+        d->guest_type = guest_type_hvm;
+    else
+        d->guest_type = guest_type_pv;
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
@@ -331,11 +338,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( !is_idle_domain(d) )
     {
-        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest )
-            d->guest_type = guest_type_hvm;
-        else
-            d->guest_type = guest_type_pv;
-
         if ( !is_hardware_domain(d) )
             d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
         else
-- 
2.1.4


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

Re: [PATCH 1/5] xen/domain: Prepare data for is_{pv, hvm}_domain() as early as possible

[Jan Beulich]

>>> On 03.09.18 at 16:46, <andrew.cooper3@citrix.com> wrote:
> Given two subtle failures from getting this wrong before, and more cleanup on
> the way, move the setting of d->guest_type as early as possible.
> 
> Note that despite moving the assignment of d->guest_type outside of the
> is_idle_domain(d) check, it still behaves the same.  Previously, system
> domains had no direct assignment of d->guest_type and behaved as PV guests
> because guest_type_pv has the value 0.
> 
> While tidying up the predicate, leave a comment refering to
> is_system_domain(), and move the associated ASSERT() to be beside the
> asignment.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -272,8 +272,12 @@ struct domain *domain_create(domid_t domid,
>      if ( (d = alloc_domain_struct()) == NULL )
>          return ERR_PTR(-ENOMEM);
>  
> +    /* Sort out our idea of is_system_domain(). */
>      d->domain_id = domid;
>  
> +    /* Debug sanity. */
> +    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);

Would you mind shortening this to at least

    ASSERT(is_system_domain(d) ? !config : config);

while you move it?

Jan



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

Re: [PATCH 1/5] xen/domain: Prepare data for is_{pv, hvm}_domain() as early as possible

[Andrew Cooper]

On 03/09/18 17:03, Jan Beulich wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -272,8 +272,12 @@ struct domain *domain_create(domid_t domid,
>>      if ( (d = alloc_domain_struct()) == NULL )
>>          return ERR_PTR(-ENOMEM);
>>  
>> +    /* Sort out our idea of is_system_domain(). */
>>      d->domain_id = domid;
>>  
>> +    /* Debug sanity. */
>> +    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
> Would you mind shortening this to at least
>
>     ASSERT(is_system_domain(d) ? !config : config);
>
> while you move it?

Unfortunately not.

domain.c: In function ‘domain_create’:
domain.c:296:67: error: pointer/integer type mismatch in conditional
expression [-Werror]
     ASSERT(is_system_domain(d) ? !config : config);
                                                                   ^
cc1: all warnings being treated as errors
/local/xen.git/xen/Rules.mk:194: recipe for target 'domain.o' failed

which reminds me why I wrote it the way I did originally.

~Andrew

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

Re: [PATCH 1/5] xen/domain: Prepare data for is_{pv, hvm}_domain() as early as possible

[Jan Beulich]

>>> On 04.09.18 at 14:17, <andrew.cooper3@citrix.com> wrote:
> On 03/09/18 17:03, Jan Beulich wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -272,8 +272,12 @@ struct domain *domain_create(domid_t domid,
>>>      if ( (d = alloc_domain_struct()) == NULL )
>>>          return ERR_PTR(-ENOMEM);
>>>  
>>> +    /* Sort out our idea of is_system_domain(). */
>>>      d->domain_id = domid;
>>>  
>>> +    /* Debug sanity. */
>>> +    ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
>> Would you mind shortening this to at least
>>
>>     ASSERT(is_system_domain(d) ? !config : config);
>>
>> while you move it?
> 
> Unfortunately not.
> 
> domain.c: In function ‘domain_create’:
> domain.c:296:67: error: pointer/integer type mismatch in conditional
> expression [-Werror]
>      ASSERT(is_system_domain(d) ? !config : config);
>                                                                    ^
> cc1: all warnings being treated as errors
> /local/xen.git/xen/Rules.mk:194: recipe for target 'domain.o' failed
> 
> which reminds me why I wrote it the way I did originally.

Well, okay then. I suppose

    ASSERT(is_system_domain(d) ? !config : !!config);

or

    ASSERT(is_system_domain(d) == !config);

aren't really acceptable to you.

Jan


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

Re: [PATCH 1/5] xen/domain: Prepare data for is_{pv, hvm}_domain() as early as possible

[Wei Liu]

On Mon, Sep 03, 2018 at 03:46:56PM +0100, Andrew Cooper wrote:
> Given two subtle failures from getting this wrong before, and more cleanup on
> the way, move the setting of d->guest_type as early as possible.
> 
> Note that despite moving the assignment of d->guest_type outside of the
> is_idle_domain(d) check, it still behaves the same.  Previously, system
> domains had no direct assignment of d->guest_type and behaved as PV guests
> because guest_type_pv has the value 0.
> 
> While tidying up the predicate, leave a comment refering to
> is_system_domain(), and move the associated ASSERT() to be beside the
> asignment.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Andrew Cooper]

This is the first step in making the destroy path idepotent, and using it in
place of the ad-hoc cleanup paths in the create path.

To begin with, the trivial free operations are broken out.  The rest of the
cleanup code will be moved as it is demonstrated (or made) to be idempotent.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/domain.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 43ab926..2253c2d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
 }
 custom_param("extra_guest_irqs", parse_extra_guest_irqs);
 
+/*
+ * Destroy a domain once all references to it have been dropped.  Used either
+ * from the RCU path, or from the domain_create() error path before the domain
+ * is inserted into the domlist.
+ */
+static void __domain_destroy(struct domain *d)
+{
+    BUG_ON(!d->is_dying);
+    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
+
+    xfree(d->pbuf);
+
+    free_cpumask_var(d->dirty_cpumask);
+
+    free_domain_struct(d);
+}
+
 struct domain *domain_create(domid_t domid,
                              struct xen_domctl_createdomain *config,
                              bool is_priv)
@@ -437,7 +454,6 @@ struct domain *domain_create(domid_t domid,
     if ( hardware_domain == d )
         hardware_domain = old_hwdom;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
-    xfree(d->pbuf);
 
     sched_destroy_domain(d);
 
@@ -462,8 +478,9 @@ struct domain *domain_create(domid_t domid,
         watchdog_domain_destroy(d);
     if ( init_status & INIT_xsm )
         xsm_free_security_domain(d);
-    free_cpumask_var(d->dirty_cpumask);
-    free_domain_struct(d);
+
+    __domain_destroy(d);
+
     return ERR_PTR(err);
 }
 
@@ -881,8 +898,6 @@ static void complete_domain_destroy(struct rcu_head *head)
     xfree(d->vm_event_share);
 #endif
 
-    xfree(d->pbuf);
-
     for ( i = d->max_vcpus - 1; i >= 0; i-- )
         if ( (v = d->vcpu[i]) != NULL )
         {
@@ -901,9 +916,9 @@ static void complete_domain_destroy(struct rcu_head *head)
     radix_tree_destroy(&d->pirq_tree, free_pirq_struct);
 
     xsm_free_security_domain(d);
-    free_cpumask_var(d->dirty_cpumask);
     xfree(d->vcpu);
-    free_domain_struct(d);
+
+    __domain_destroy(d);
 
     send_global_virq(VIRQ_DOM_EXC);
 }
-- 
2.1.4


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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Jan Beulich]

>>> On 03.09.18 at 16:46, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> +/*
> + * Destroy a domain once all references to it have been dropped.  Used either
> + * from the RCU path, or from the domain_create() error path before the domain
> + * is inserted into the domlist.
> + */
> +static void __domain_destroy(struct domain *d)

With just a single leading underscore
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Wei Liu]

On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
> This is the first step in making the destroy path idepotent, and using it in

"idempotent".

> place of the ad-hoc cleanup paths in the create path.
> 
> To begin with, the trivial free operations are broken out.  The rest of the
> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 43ab926..2253c2d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> +/*
> + * Destroy a domain once all references to it have been dropped.  Used either
> + * from the RCU path, or from the domain_create() error path before the domain
> + * is inserted into the domlist.
> + */
> +static void __domain_destroy(struct domain *d)
> +{
> +    BUG_ON(!d->is_dying);
> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
> +
> +    xfree(d->pbuf);

With this changed to XFREE here:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> +
> +    free_cpumask_var(d->dirty_cpumask);

On making things idempotent: this function seems to be a candidate.

Wei.

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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Andrew Cooper]

On 03/09/18 17:54, Wei Liu wrote:
> On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
>> This is the first step in making the destroy path idepotent, and using it in
> "idempotent".
>
>> place of the ad-hoc cleanup paths in the create path.
>>
>> To begin with, the trivial free operations are broken out.  The rest of the
>> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 43ab926..2253c2d 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
>>  }
>>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>>  
>> +/*
>> + * Destroy a domain once all references to it have been dropped.  Used either
>> + * from the RCU path, or from the domain_create() error path before the domain
>> + * is inserted into the domlist.
>> + */
>> +static void __domain_destroy(struct domain *d)
>> +{
>> +    BUG_ON(!d->is_dying);
>> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
>> +
>> +    xfree(d->pbuf);
> With this changed to XFREE here:

This is the one place where it doesn't matter.  d goes fully out of
scope before the end of this function.

>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>
>> +
>> +    free_cpumask_var(d->dirty_cpumask);
> On making things idempotent: this function seems to be a candidate.

I don't understand.  One implementation is xfree() under the hood, and
the other is a no-op because no allocation took place.

~Andrew

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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Wei Liu]

On Mon, Sep 03, 2018 at 05:58:02PM +0100, Andrew Cooper wrote:
> On 03/09/18 17:54, Wei Liu wrote:
> > On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
> >> This is the first step in making the destroy path idepotent, and using it in
> > "idempotent".
> >
> >> place of the ad-hoc cleanup paths in the create path.
> >>
> >> To begin with, the trivial free operations are broken out.  The rest of the
> >> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
> >>  1 file changed, 22 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >> index 43ab926..2253c2d 100644
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
> >>  }
> >>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
> >>  
> >> +/*
> >> + * Destroy a domain once all references to it have been dropped.  Used either
> >> + * from the RCU path, or from the domain_create() error path before the domain
> >> + * is inserted into the domlist.
> >> + */
> >> +static void __domain_destroy(struct domain *d)
> >> +{
> >> +    BUG_ON(!d->is_dying);
> >> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
> >> +
> >> +    xfree(d->pbuf);
> > With this changed to XFREE here:
> 
> This is the one place where it doesn't matter.  d goes fully out of
> scope before the end of this function.

That's fair enough.

> 
> >
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >
> >> +
> >> +    free_cpumask_var(d->dirty_cpumask);
> > On making things idempotent: this function seems to be a candidate.
> 
> I don't understand.  One implementation is xfree() under the hood, and
> the other is a no-op because no allocation took place.

I mean it would probably be useful to make free_cpumask_var idempotent
by using XFREE so multiple calls to it will not free dangling pointer.

Wei.

> 
> ~Andrew

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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Andrew Cooper]

On 03/09/18 18:01, Wei Liu wrote:
> On Mon, Sep 03, 2018 at 05:58:02PM +0100, Andrew Cooper wrote:
>> On 03/09/18 17:54, Wei Liu wrote:
>>> On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
>>>> This is the first step in making the destroy path idepotent, and using it in
>>> "idempotent".
>>>
>>>> place of the ad-hoc cleanup paths in the create path.
>>>>
>>>> To begin with, the trivial free operations are broken out.  The rest of the
>>>> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
>>>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>> index 43ab926..2253c2d 100644
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
>>>>  }
>>>>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>>>>  
>>>> +/*
>>>> + * Destroy a domain once all references to it have been dropped.  Used either
>>>> + * from the RCU path, or from the domain_create() error path before the domain
>>>> + * is inserted into the domlist.
>>>> + */
>>>> +static void __domain_destroy(struct domain *d)
>>>> +{
>>>> +    BUG_ON(!d->is_dying);
>>>> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
>>>> +
>>>> +    xfree(d->pbuf);
>>> With this changed to XFREE here:
>> This is the one place where it doesn't matter.  d goes fully out of
>> scope before the end of this function.
> That's fair enough.
>
>>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>>
>>>> +
>>>> +    free_cpumask_var(d->dirty_cpumask);
>>> On making things idempotent: this function seems to be a candidate.
>> I don't understand.  One implementation is xfree() under the hood, and
>> the other is a no-op because no allocation took place.
> I mean it would probably be useful to make free_cpumask_var idempotent
> by using XFREE so multiple calls to it will not free dangling pointer.

Ah - that's complicated because of the (lack of) indirection of the
parameter.

There is FREE_CPUMASK_VAR() which DTRT, but see above for why it isn't
used.  (There is a similar FREE_XENHEAP_PAGE helper).

~Andrew

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

Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

[Wei Liu]

On Mon, Sep 03, 2018 at 06:05:34PM +0100, Andrew Cooper wrote:
> On 03/09/18 18:01, Wei Liu wrote:
> > On Mon, Sep 03, 2018 at 05:58:02PM +0100, Andrew Cooper wrote:
> >> On 03/09/18 17:54, Wei Liu wrote:
> >>> On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
> >>>> This is the first step in making the destroy path idepotent, and using it in
> >>> "idempotent".
> >>>
> >>>> place of the ad-hoc cleanup paths in the create path.
> >>>>
> >>>> To begin with, the trivial free operations are broken out.  The rest of the
> >>>> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
> >>>>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> ---
> >>>> CC: Jan Beulich <JBeulich@suse.com>
> >>>> CC: Wei Liu <wei.liu2@citrix.com>
> >>>> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>>> CC: Stefano Stabellini <sstabellini@kernel.org>
> >>>> CC: Julien Grall <julien.grall@arm.com>
> >>>> ---
> >>>>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
> >>>>  1 file changed, 22 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >>>> index 43ab926..2253c2d 100644
> >>>> --- a/xen/common/domain.c
> >>>> +++ b/xen/common/domain.c
> >>>> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
> >>>>  }
> >>>>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
> >>>>  
> >>>> +/*
> >>>> + * Destroy a domain once all references to it have been dropped.  Used either
> >>>> + * from the RCU path, or from the domain_create() error path before the domain
> >>>> + * is inserted into the domlist.
> >>>> + */
> >>>> +static void __domain_destroy(struct domain *d)
> >>>> +{
> >>>> +    BUG_ON(!d->is_dying);
> >>>> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
> >>>> +
> >>>> +    xfree(d->pbuf);
> >>> With this changed to XFREE here:
> >> This is the one place where it doesn't matter.  d goes fully out of
> >> scope before the end of this function.
> > That's fair enough.
> >
> >>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >>>
> >>>> +
> >>>> +    free_cpumask_var(d->dirty_cpumask);
> >>> On making things idempotent: this function seems to be a candidate.
> >> I don't understand.  One implementation is xfree() under the hood, and
> >> the other is a no-op because no allocation took place.
> > I mean it would probably be useful to make free_cpumask_var idempotent
> > by using XFREE so multiple calls to it will not free dangling pointer.
> 
> Ah - that's complicated because of the (lack of) indirection of the
> parameter.
> 
> There is FREE_CPUMASK_VAR() which DTRT, but see above for why it isn't
> used.  (There is a similar FREE_XENHEAP_PAGE helper).

Okay. I don't have further comments on this.

Wei.

> 
> ~Andrew

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

[PATCH 3/5] xen/domain: Call lock_profile_deregister_struct() from common code

[Andrew Cooper]

lock_profile_register_struct() is called from common code, but the matching
deregister was previously only called from x86 code.

The practical upshot of this when using CONFIG_LOCK_PROFILE, destroyed domains
on ARM (and in particular, the freed page behind struct domain) remain on the
lockprofile linked list, which will become corrupt when the page is reused.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/domain.c | 1 -
 xen/common/domain.c   | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 688f7fb..cd1419e 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -298,7 +298,6 @@ struct domain *alloc_domain_struct(void)
 
 void free_domain_struct(struct domain *d)
 {
-    lock_profile_deregister_struct(LOCKPROF_TYPE_PERDOM, d);
     free_xenheap_page(d);
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 2253c2d..9f810d1 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -274,6 +274,8 @@ static void __domain_destroy(struct domain *d)
 
     free_cpumask_var(d->dirty_cpumask);
 
+    lock_profile_deregister_struct(LOCKPROF_TYPE_PERDOM, d);
+
     free_domain_struct(d);
 }
 
-- 
2.1.4


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

Re: [PATCH 3/5] xen/domain: Call lock_profile_deregister_struct() from common code

[Jan Beulich]

>>> On 03.09.18 at 16:46, <andrew.cooper3@citrix.com> wrote:
> lock_profile_register_struct() is called from common code, but the matching
> deregister was previously only called from x86 code.
> 
> The practical upshot of this when using CONFIG_LOCK_PROFILE, destroyed domains
> on ARM (and in particular, the freed page behind struct domain) remain on the
> lockprofile linked list, which will become corrupt when the page is reused.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 3/5] xen/domain: Call lock_profile_deregister_struct() from common code

[Wei Liu]

On Mon, Sep 03, 2018 at 03:46:58PM +0100, Andrew Cooper wrote:
> lock_profile_register_struct() is called from common code, but the matching
> deregister was previously only called from x86 code.
> 
> The practical upshot of this when using CONFIG_LOCK_PROFILE, destroyed domains
> on ARM (and in particular, the freed page behind struct domain) remain on the
> lockprofile linked list, which will become corrupt when the page is reused.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH 4/5] xen/domain: Fold xsm_free_security_domain() paths together

[Andrew Cooper]

xsm_free_security_domain() is idempotent (both the dummy handler, and the
flask handler).  Move it into the shared __domain_destroy() path, and drop the
INIT_xsm flag from domain_create()

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/common/domain.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 9f810d1..d1c1993 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -274,6 +274,8 @@ static void __domain_destroy(struct domain *d)
 
     free_cpumask_var(d->dirty_cpumask);
 
+    xsm_free_security_domain(d);
+
     lock_profile_deregister_struct(LOCKPROF_TYPE_PERDOM, d);
 
     free_domain_struct(d);
@@ -284,7 +286,7 @@ struct domain *domain_create(domid_t domid,
                              bool is_priv)
 {
     struct domain *d, **pd, *old_hwdom = NULL;
-    enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
+    enum { INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
 
@@ -324,7 +326,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
         goto fail;
-    init_status |= INIT_xsm;
 
     atomic_set(&d->refcnt, 1);
     spin_lock_init_prof(d, domain_lock);
@@ -478,8 +479,6 @@ struct domain *domain_create(domid_t domid,
         rangeset_domain_destroy(d);
     if ( init_status & INIT_watchdog )
         watchdog_domain_destroy(d);
-    if ( init_status & INIT_xsm )
-        xsm_free_security_domain(d);
 
     __domain_destroy(d);
 
@@ -917,7 +916,6 @@ static void complete_domain_destroy(struct rcu_head *head)
 
     radix_tree_destroy(&d->pirq_tree, free_pirq_struct);
 
-    xsm_free_security_domain(d);
     xfree(d->vcpu);
 
     __domain_destroy(d);
-- 
2.1.4


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

Re: [PATCH 4/5] xen/domain: Fold xsm_free_security_domain() paths together

[Jan Beulich]

>>> On 03.09.18 at 16:46, <andrew.cooper3@citrix.com> wrote:
> xsm_free_security_domain() is idempotent (both the dummy handler, and the
> flask handler).  Move it into the shared __domain_destroy() path, and drop the
> INIT_xsm flag from domain_create()
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 4/5] xen/domain: Fold xsm_free_security_domain() paths together

[Wei Liu]

On Mon, Sep 03, 2018 at 03:46:59PM +0100, Andrew Cooper wrote:
> xsm_free_security_domain() is idempotent (both the dummy handler, and the
> flask handler).  Move it into the shared __domain_destroy() path, and drop the
> INIT_xsm flag from domain_create()
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [Non-DoD Source] [PATCH 4/5] xen/domain: Fold xsm_free_security_domain() paths together

[DeGraaf, Daniel G]

> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Monday, September 3, 2018 10:47 AM
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>
> Subject: [Non-DoD Source] [PATCH 4/5] xen/domain: Fold xsm_free_security_domain() paths together
> 
> xsm_free_security_domain() is idempotent (both the dummy handler, and the
> flask handler).  Move it into the shared __domain_destroy() path, and drop the
> INIT_xsm flag from domain_create()
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

[PATCH 5/5] xen/domain: Make rangeset_domain_destroy() idempotent

[Andrew Cooper]

... and move it into the common __domain_destroy() path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/domain.c   | 9 +++------
 xen/common/rangeset.c | 3 +++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index d1c1993..2f2f152 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -272,6 +272,8 @@ static void __domain_destroy(struct domain *d)
 
     xfree(d->pbuf);
 
+    rangeset_domain_destroy(d);
+
     free_cpumask_var(d->dirty_cpumask);
 
     xsm_free_security_domain(d);
@@ -286,7 +288,7 @@ struct domain *domain_create(domid_t domid,
                              bool is_priv)
 {
     struct domain *d, **pd, *old_hwdom = NULL;
-    enum { INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
+    enum { INIT_watchdog = 1u<<1,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
 
@@ -350,7 +352,6 @@ struct domain *domain_create(domid_t domid,
         goto fail;
 
     rangeset_domain_initialise(d);
-    init_status |= INIT_rangeset;
 
     /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
     if ( is_system_domain(d) && !is_idle_domain(d) )
@@ -475,8 +476,6 @@ struct domain *domain_create(domid_t domid,
         evtchn_destroy_final(d);
         radix_tree_destroy(&d->pirq_tree, free_pirq_struct);
     }
-    if ( init_status & INIT_rangeset )
-        rangeset_domain_destroy(d);
     if ( init_status & INIT_watchdog )
         watchdog_domain_destroy(d);
 
@@ -882,8 +881,6 @@ static void complete_domain_destroy(struct rcu_head *head)
 
     watchdog_domain_destroy(d);
 
-    rangeset_domain_destroy(d);
-
     sched_destroy_domain(d);
 
     /* Free page used by xen oprofile buffer. */
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 7788cdd..e3857ab 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -482,6 +482,9 @@ void rangeset_domain_destroy(
 {
     struct rangeset *r;
 
+    if ( list_head_is_null(&d->rangesets) )
+        return;
+
     while ( !list_empty(&d->rangesets) )
     {
         r = list_entry(d->rangesets.next, struct rangeset, rangeset_list);
-- 
2.1.4


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

Re: [PATCH 5/5] xen/domain: Make rangeset_domain_destroy() idempotent

[Jan Beulich]

>>> On 03.09.18 at 16:47, <andrew.cooper3@citrix.com> wrote:
> ... and move it into the common __domain_destroy() path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 5/5] xen/domain: Make rangeset_domain_destroy() idempotent

[Wei Liu]

On Mon, Sep 03, 2018 at 03:47:00PM +0100, Andrew Cooper wrote:
> ... and move it into the common __domain_destroy() path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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