[Xen-devel] [PATCH 0/2] xen/sched: fix freeing of per-vcpu data

[Xen-devel] [PATCH 0/2] xen/sched: fix freeing of per-vcpu data

[Juergen Gross]

Fix two latent bugs discovered in review of my core scheduling series.

For the series:
Release-acked-by: Juergen Gross <jgross@suse.com>

Juergen Gross (2):
  xen/sched: fix locking in a653sched_free_vdata()
  xen/sched: fix freeing per-vcpu data in sched_move_domain()

 xen/common/sched_arinc653.c | 6 ++++++
 xen/common/schedule.c       | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.16.4


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

[Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Juergen Gross]

The arinc653 scheduler's free_vdata() function is missing proper
locking: as it is modifying the scheduler's private vcpu list it needs
to take the scheduler lock during that operation.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched_arinc653.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 72b988ea5f..d47b747ef4 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -442,16 +442,22 @@ a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
 static void
 a653sched_free_vdata(const struct scheduler *ops, void *priv)
 {
+    a653sched_priv_t *sched_priv = SCHED_PRIV(ops);
     arinc653_vcpu_t *av = priv;
+    unsigned long flags;
 
     if (av == NULL)
         return;
 
+    spin_lock_irqsave(&sched_priv->lock, flags);
+
     if ( !is_idle_vcpu(av->vc) )
         list_del(&av->list);
 
     xfree(av);
     update_schedule_vcpus(ops);
+
+    spin_unlock_irqrestore(&sched_priv->lock, flags);
 }
 
 /**
-- 
2.16.4


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

[Xen-devel] [PATCH 2/2] xen/sched: fix freeing per-vcpu data in sched_move_domain()

[Juergen Gross]

In case of an allocation error of per-vcpu data in sched_move_domain()
the already allocated data is freed just using xfree(). This is wrong
as some schedulers need to do additional operations (e.g. the arinc653
scheduler needs to remove the vcpu-data from a list).

So instead xfree() make use of the sched_free_vdata() hook.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/schedule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 13b5ffc7cf..13c17fe944 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -386,7 +386,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
         if ( vcpu_priv[v->vcpu_id] == NULL )
         {
             for_each_vcpu ( d, v )
-                xfree(vcpu_priv[v->vcpu_id]);
+                sched_free_vdata(c->sched, vcpu_priv[v->vcpu_id]);
             xfree(vcpu_priv);
             sched_free_domdata(c->sched, domdata);
             return -ENOMEM;
-- 
2.16.4


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

Re: [Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Jan Beulich]

On 25.09.2019 09:05, Juergen Gross wrote:
> The arinc653 scheduler's free_vdata() function is missing proper
> locking: as it is modifying the scheduler's private vcpu list it needs
> to take the scheduler lock during that operation.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
(and maybe also Suspected-by)

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

Re: [Xen-devel] [PATCH 2/2] xen/sched: fix freeing per-vcpu data in sched_move_domain()

[Jan Beulich]

On 25.09.2019 09:05, Juergen Gross wrote:
> In case of an allocation error of per-vcpu data in sched_move_domain()
> the already allocated data is freed just using xfree(). This is wrong
> as some schedulers need to do additional operations (e.g. the arinc653
> scheduler needs to remove the vcpu-data from a list).
> 
> So instead xfree() make use of the sched_free_vdata() hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.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: [Xen-devel] [PATCH 2/2] xen/sched: fix freeing per-vcpu data in sched_move_domain()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 798 bytes --]

On Wed, 2019-09-25 at 09:05 +0200, Juergen Gross wrote:
> In case of an allocation error of per-vcpu data in
> sched_move_domain()
> the already allocated data is freed just using xfree(). This is wrong
> as some schedulers need to do additional operations (e.g. the
> arinc653
> scheduler needs to remove the vcpu-data from a list).
> 
> So instead xfree() make use of the sched_free_vdata() hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 645 bytes --]

On Wed, 2019-09-25 at 09:05 +0200, Juergen Gross wrote:
> The arinc653 scheduler's free_vdata() function is missing proper
> locking: as it is modifying the scheduler's private vcpu list it
> needs
> to take the scheduler lock during that operation.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Jürgen Groß]

On 25.09.19 12:59, Dario Faggioli wrote:
> On Wed, 2019-09-25 at 09:05 +0200, Juergen Gross wrote:
>> The arinc653 scheduler's free_vdata() function is missing proper
>> locking: as it is modifying the scheduler's private vcpu list it
>> needs
>> to take the scheduler lock during that operation.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

As this patch is a prerequisite for my core scheduling series I'd really
appreciate if it could be committed rather sooner than later.

Josh, Robert, could you please comment?

Or is Dario's R-b (and Jan's as well) enough in this rather simple and
obvious case?


Juergen

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

Re: [Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Jan Beulich]

On 27.09.2019 09:23, Jürgen Groß  wrote:
> On 25.09.19 12:59, Dario Faggioli wrote:
>> On Wed, 2019-09-25 at 09:05 +0200, Juergen Gross wrote:
>>> The arinc653 scheduler's free_vdata() function is missing proper
>>> locking: as it is modifying the scheduler's private vcpu list it
>>> needs
>>> to take the scheduler lock during that operation.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
> 
> As this patch is a prerequisite for my core scheduling series I'd really
> appreciate if it could be committed rather sooner than later.
> 
> Josh, Robert, could you please comment?
> 
> Or is Dario's R-b (and Jan's as well) enough in this rather simple and
> obvious case?

I was more or less planning to time out on waiting for their ack
later today.

Jan

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

Re: [Xen-devel] [PATCH 1/2] xen/sched: fix locking in a653sched_free_vdata()

[Jürgen Groß]

On 27.09.19 10:20, Jan Beulich wrote:
> On 27.09.2019 09:23, Jürgen Groß  wrote:
>> On 25.09.19 12:59, Dario Faggioli wrote:
>>> On Wed, 2019-09-25 at 09:05 +0200, Juergen Gross wrote:
>>>> The arinc653 scheduler's free_vdata() function is missing proper
>>>> locking: as it is modifying the scheduler's private vcpu list it
>>>> needs
>>>> to take the scheduler lock during that operation.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
>>
>> As this patch is a prerequisite for my core scheduling series I'd really
>> appreciate if it could be committed rather sooner than later.
>>
>> Josh, Robert, could you please comment?
>>
>> Or is Dario's R-b (and Jan's as well) enough in this rather simple and
>> obvious case?
> 
> I was more or less planning to time out on waiting for their ack
> later today.

Thanks.


Juergen


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