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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
---
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?

--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -2403,7 +2403,7 @@ csched2_res_pick(const struct scheduler
         } d;
         d.dom = unit->domain->domain_id;
         d.unit = unit->unit_id;
-        d.rq_id = min_rqd->id;
+        d.rq_id = min_rqd ? min_rqd->id : -1;
         d.b_avgload = min_avgload;
         d.new_cpu = new_cpu;
         __trace_var(TRC_CSCHED2_PICKED_CPU, 1,

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