Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing

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

On 02.03.20 17:49, Dario Faggioli wrote:
> On Mon, 2020-03-02 at 09:58 +0100, Jan Beulich wrote:
> > The issue here results from one of the downsides of using goto: The
> > early "goto out" and "goto out_up" in the function very clearly
> > bypass
> > any possible initialization of min_rqd, yet the tracing code at the
> > end
> > of the function consumes the value. There's even a comment regarding
> > the
> > trace record not being accurate in this case.
> >
> > CID: 1460432
> > Fixes: 9c84bc004653 ("sched: rework credit2 run-queue allocation")
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>
> > ---
> > It took me a little to convince myself that
> >
> >      new_cpu = cpumask_cycle(min_rqd->pick_bias,
> > cpumask_scratch_cpu(cpu));
> >      min_rqd->pick_bias = new_cpu;
> >
> > are safe, i.e. min_rqd can't be NULL here. I think though that this
> > could do with making more obvious, at the very least by e.g.
> >
> >     @@ -2360,6 +2360,8 @@
> >                          unit->cpu_soft_affinity);
> >              cpumask_and(cpumask_scratch_cpu(cpu),
> > cpumask_scratch_cpu(cpu),
> >                          &min_s_rqd->active);
> >     +
> >     +        BUG_ON(!min_rqd);
> >          }
> >          else if ( min_rqd )
> >          {
> >
> > possibly accompanied by a comment. Thoughts?
> Yes, I think this is a good idea.
>
> Personally, I'd put the BUG_ON() outside of the "if {} else if {} else
> {}" block (i.e., just above the cpumask_cycle().
I don't think so.

Otherwise the "else if ( min_rqd )" wouldn't make sense.

> I can send a patch for that.

Juergen

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