On 8/5/25 03:44, Jan Beulich wrote:
> On 04.08.2025 18:57, Stewart Hildebrand wrote:
>> On 8/4/25 03:57, Jan Beulich wrote:
>>> On 01.08.2025 22:24, Stewart Hildebrand wrote:
>>>> @@ -839,6 +839,9 @@ void sched_destroy_vcpu(struct vcpu *v)
>>>> {
>>>> struct sched_unit *unit = v->sched_unit;
>>>>
>>>> + if ( !unit )
>>>> + return;
>>>> +
>>>> kill_timer(&v->periodic_timer);
>>>> kill_timer(&v->singleshot_timer);
>>>> kill_timer(&v->poll_timer);
>>>
>>> What if it's the 2nd error path in sched_init_vcpu() that is taken?
^^ This ^^ is what I'm confused about
>>> Then we
>>> might take this path (just out of context here)
>>>
>>> if ( unit->vcpu_list == v )
>>> {
>>> rcu_read_lock(&sched_res_rculock);
>>>
>>> sched_remove_unit(vcpu_scheduler(v), unit);
>>> sched_free_udata(vcpu_scheduler(v), unit->priv);
>>>
>>> and at least Credit1's hook doesn't look to be safe against being passed
>>> NULL.
>>> (Not to speak of the risk of unit->priv being used elsewhere while cleaning
>>> up.)
>>
>>
>> Are you referring to this error path in sched_init_vcpu?
>
> No, given the context I thought it was clear that I was referring to
>
> static void cf_check
> csched_free_udata(const struct scheduler *ops, void *priv)
> {
> struct csched_unit *svc = priv;
>
> BUG_ON( !list_empty(&svc->runq_elem) );
>
> (i.e. particularly this BUG_ON()).
The comment about credit1 was clear
