* [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
@ 2020-03-02 8:58 Jan Beulich
2020-03-02 9:21 ` Jürgen Groß
2020-03-02 16:49 ` Dario Faggioli
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2020-03-02 8:58 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, George Dunlap, Dario Faggioli
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@suse.com>
---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
2020-03-02 8:58 [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing Jan Beulich
@ 2020-03-02 9:21 ` Jürgen Groß
2020-03-02 16:49 ` Dario Faggioli
1 sibling, 0 replies; 6+ messages in thread
From: Jürgen Groß @ 2020-03-02 9:21 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, Dario Faggioli
On 02.03.20 09:58, 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@suse.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
2020-03-02 8:58 [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing Jan Beulich
2020-03-02 9:21 ` Jürgen Groß
@ 2020-03-02 16:49 ` Dario Faggioli
2020-03-02 16:59 ` Jürgen Groß
1 sibling, 1 reply; 6+ messages in thread
From: Dario Faggioli @ 2020-03-02 16:49 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Juergen Gross, George Dunlap
[-- Attachment #1.1: Type: text/plain, Size: 1759 bytes --]
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@suse.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
2020-03-02 16:49 ` Dario Faggioli
@ 2020-03-02 16:59 ` Jürgen Groß
2020-03-02 18:16 ` Dario Faggioli
0 siblings, 1 reply; 6+ messages in thread
From: Jürgen Groß @ 2020-03-02 16:59 UTC (permalink / raw)
To: Dario Faggioli, Jan Beulich, xen-devel; +Cc: George Dunlap
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@suse.com>
>>
> Acked-by: Dario Faggioli <dfaggioli@suse.com>
>
>> ---
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
2020-03-02 16:59 ` Jürgen Groß
@ 2020-03-02 18:16 ` Dario Faggioli
2020-03-03 5:31 ` Jürgen Groß
0 siblings, 1 reply; 6+ messages in thread
From: Dario Faggioli @ 2020-03-02 18:16 UTC (permalink / raw)
To: Jürgen Groß, Jan Beulich, xen-devel; +Cc: George Dunlap
[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]
On Mon, 2020-03-02 at 17:59 +0100, Jürgen Groß wrote:
> On 02.03.20 17:49, Dario Faggioli wrote:
> > On Mon, 2020-03-02 at 09:58 +0100, Jan Beulich wrote:
> > >
> > > @@ -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.
>
Why wouldn't it?
I mean, what I was saying is that I think it would be nice to have,
just before this:
new_cpu = cpumask_cycle(min_rqd->pick_bias, cpumask_scratch_cpu(cpu));
min_rqd->pick_bias = new_cpu;
A BUG_ON(!min_rqd), with a comment above it explainint that, no matter
how we got here, at this point we are sure that min_rqd is valid,
either because it already was before the if{} (in which case we took
either the first or the second branch of the if itself), or because we
did setup it in the if{} itself (in the third branch).
I see why one may want for something like that to be in the first
branch of the if{}... I was just saying that I personally like it
better for something like this to be close to where the pointer is
dereferenced...
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing
2020-03-02 18:16 ` Dario Faggioli
@ 2020-03-03 5:31 ` Jürgen Groß
0 siblings, 0 replies; 6+ messages in thread
From: Jürgen Groß @ 2020-03-03 5:31 UTC (permalink / raw)
To: Dario Faggioli, Jan Beulich, xen-devel; +Cc: George Dunlap
On 02.03.20 19:16, Dario Faggioli wrote:
> On Mon, 2020-03-02 at 17:59 +0100, Jürgen Groß wrote:
>> On 02.03.20 17:49, Dario Faggioli wrote:
>>> On Mon, 2020-03-02 at 09:58 +0100, Jan Beulich wrote:
>>>>
>>>> @@ -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.
>>
> Why wouldn't it?
>
> I mean, what I was saying is that I think it would be nice to have,
> just before this:
>
> new_cpu = cpumask_cycle(min_rqd->pick_bias, cpumask_scratch_cpu(cpu));
> min_rqd->pick_bias = new_cpu;
Sorry, my brain was in a low power state.
I managed to understand you completely wrong, thinking you wanted to
add the BUG_ON() before the if ... block.
Sorry for the noise,
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-03 5:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-02 8:58 [Xen-devel] [PATCH] credit2: avoid NULL deref in csched2_res_pick() when tracing Jan Beulich
2020-03-02 9:21 ` Jürgen Groß
2020-03-02 16:49 ` Dario Faggioli
2020-03-02 16:59 ` Jürgen Groß
2020-03-02 18:16 ` Dario Faggioli
2020-03-03 5:31 ` Jürgen Groß
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).