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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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 can send a patch for that.

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part

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