Re: [PATCH v2] xen: rework error handling in vcpu_create

[Jürgen Groß <jgross@xxxxxxxx>]

On 05.08.25 11:49, Jan Beulich wrote:
> On 05.08.2025 11:33, Stewart Hildebrand wrote:
> > On 8/5/25 05:17, Jan Beulich wrote:
> > > On 05.08.2025 11:06, Stewart Hildebrand wrote:
> > > > 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
> > > If sched_init_vcpu() took the indicated path,
> > What path? In the one I'm looking at, sched_free_unit() gets called,
> Oh, I see - that wasn't quite obvious, though. Yet of course ...
>
> > setting v->sched_unit = NULL, and in that case ...
> >
> > > the if() you add here won't
> > > help, and ...
> > ... the condition is true, and ...
> >
> > > > > > > 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.)
> ... this latter part of my remark still applies. IOW I continue to think
> that discussing the correctness of this change needs to be extended.
> Unless of course a scheduler maintainer wants to ack it as is.
It doesn't apply.

vcpus of normal domains (i.e. not the idle domain) are created in sequential
order. If the 2nd error path of sched_init_vcpu() is taken, this means we
are handling the first vcpu of the unit. In this case sched_free_unit() will
free the unit itself completely, so the path you are referring to can't be
reached.

I think only Andrew's comments need to be worked on.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature