xen/sched: fix rare error case in cpu_schedule_down()

[PATCH] xen/sched: fix rare error case in cpu_schedule_down()

Posted by Juergen Gross 2 months, 2 weeks ago

In case cpu_schedule_up() is failing to allocate memory for struct
sched_resource, cpu_schedule_down() will be called with the
sched_resource pointer being NULL. This needs to be handled.

Reported-by: Jan Beulich <jbeulich@suse.com>
Fixes: 207589dbacd4 ("xen/sched: move per cpu scheduler private data into struct sched_resource")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d84b65f197..0dc86b8f6c 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2829,6 +2829,8 @@ static void cpu_schedule_down(unsigned int cpu)
     rcu_read_lock(&sched_res_rculock);
 
     sr = get_sched_res(cpu);
+    if ( !sr )
+        goto out;
 
     kill_timer(&sr->s_timer);
 
@@ -2839,6 +2841,7 @@ static void cpu_schedule_down(unsigned int cpu)
     sr->sched_unit_idle = NULL;
     call_rcu(&sr->rcu, sched_res_free);
 
+ out:
     rcu_read_unlock(&sched_res_rculock);
 }
 
-- 
2.35.3

Re: [PATCH] xen/sched: fix rare error case in cpu_schedule_down()

Posted by Jan Beulich 2 months, 2 weeks ago

On 25.06.2024 10:27, Juergen Gross wrote:
> In case cpu_schedule_up() is failing to allocate memory for struct
> sched_resource,

Or (just to mention it again) in case the function isn't called at all
because some earlier CPU_UP_PREPARE handler fails.

> cpu_schedule_down() will be called with the
> sched_resource pointer being NULL. This needs to be handled.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Fixes: 207589dbacd4 ("xen/sched: move per cpu scheduler private data into struct sched_resource")
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

You didn't tag it for 4.19 and you also didn't Cc Oleksii, so I expect you
deem this minor enough to be delayed until 4.20 opens, despite the Fixes:
tag?

Jan

Re: [PATCH] xen/sched: fix rare error case in cpu_schedule_down()

Posted by Juergen Gross 2 months, 2 weeks ago

On 25.06.24 10:33, Jan Beulich wrote:
> On 25.06.2024 10:27, Juergen Gross wrote:
>> In case cpu_schedule_up() is failing to allocate memory for struct
>> sched_resource,
> 
> Or (just to mention it again) in case the function isn't called at all
> because some earlier CPU_UP_PREPARE handler fails.

This remark made me looking into the notifier implementation.

I think this patch should be reworked significantly:

In cpu_up() the CPU_UP_CANCELED notifier call is using the same
notifier_block as the one used by CPU_UP_PREPARE before. This means,
that only the handlers which didn't fail for CPU_UP_PREPARE will be
called with CPU_UP_CANCELED.

So there is no such case as "in case the function isn't called at all
because some earlier CPU_UP_PREPARE handler fails".

And a failure of cpu_schedule_up() needs to undo all externally visible
modifications instead of hoping that the CPU_UP_CANCELED notifier will
do that.

So: V2 of the patch will be needed.

Juergen

Re: [PATCH] xen/sched: fix rare error case in cpu_schedule_down()

Posted by Jürgen Groß 2 months, 2 weeks ago

On 25.06.24 10:33, Jan Beulich wrote:
> On 25.06.2024 10:27, Juergen Gross wrote:
>> In case cpu_schedule_up() is failing to allocate memory for struct
>> sched_resource,
> 
> Or (just to mention it again) in case the function isn't called at all
> because some earlier CPU_UP_PREPARE handler fails.
> 
>> cpu_schedule_down() will be called with the
>> sched_resource pointer being NULL. This needs to be handled.
>>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> Fixes: 207589dbacd4 ("xen/sched: move per cpu scheduler private data into struct sched_resource")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> You didn't tag it for 4.19 and you also didn't Cc Oleksii, so I expect you
> deem this minor enough to be delayed until 4.20 opens, despite the Fixes:
> tag?

Correct.


Juergen