Re: [Xen-devel] [PATCH RFC V2 01/45] xen/sched: add inline wrappers for calling per-scheduler functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.19 at 14:44, <jgross@xxxxxxxx> wrote:
> On 09/05/2019 14:31, Jan Beulich wrote:
>>>>> On 09.05.19 at 14:03, <jgross@xxxxxxxx> wrote:
>>> On 09/05/2019 13:50, Jan Beulich wrote:
>>>>>>> On 09.05.19 at 12:56, <jgross@xxxxxxxx> wrote:
>>>>> On 09/05/2019 12:04, George Dunlap wrote:
>>>>>> On 5/9/19 6:32 AM, Juergen Gross wrote:
>>>>>>> On 08/05/2019 18:24, George Dunlap wrote:
>>>>>>>> On 5/6/19 7:56 AM, Juergen Gross wrote:
>>>>>>>>> Instead of using the SCHED_OP() macro to call the different scheduler
>>>>>>>>> specific functions add inline wrappers for that purpose.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>>>>>
>>>>>>>> This seems like a great idea.  One minor comment...
>>>>>>>>
>>>>>>>>> +static inline int sched_init(struct scheduler *s)
>>>>>>>>> +{
>>>>>>>>> +    ASSERT(s->init);
>>>>>>>>> +    return s->init(s);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static inline void sched_deinit(struct scheduler *s)
>>>>>>>>> +{
>>>>>>>>> +    ASSERT(s->deinit);
>>>>>>>>> +    s->deinit(s);
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> I think these would better as BUG_ON()s.  These aren't hot paths, and 
>>>>>>>> if
>>>>>>>> we do somehow hit this situation in production, 1) it's safer to
>>>>>>>> BUG_ON() than dereferencing NULL, and 2) you'll get a more helpful 
>>>>>>>> error
>>>>>>>> message.
>>>>>>>
>>>>>>> Only for those 2 instances above? Or would you like BUG_ON() instead of
>>>>>>> ASSERT() in the other added inlines, too (maybe not for pick_cpu, but
>>>>>>> e.g. the ones in free_*) ?
>>>>>>
>>>>>> Why not for pick_cpu()?  It's the same basic logic -- in production, if
>>>>>> it *is* NULL, then you'll either crash with a segfault, or start
>>>>>> executing an exploit.  Much better to BUG_ON().
>>>>>
>>>>> pick_cpu is called rather often, so maybe we should avoid additional
>>>>> tests.
>>>>>
>>>>> Hmm, thinking more about it: why don't we just drop those ASSERT/BUG_ON
>>>>> for mandatory functions and test them when doing the global_init() loop
>>>>> over all schedulers. We could just reject schedulers with missing
>>>>> functions.
>>>>
>>>> This would imply pointers can't be zapped off the structures.
>>>> IMO this would require, as minimal (language level) protection,
>>>> that all instances of struct scheduler be const, which doesn't
>>>> look doable without some further rework
>>>
>>> They are const already.
>>>
>>> The default scheduler's struct is copied to a non-const struct scheduler
>>> in scheduler_init().
>> 
>> Exactly, and then we have things like
>> 
>> static int
>> rt_init(struct scheduler *ops)
>> {
>>     ...
>>     ops->sched_data = prv;
>> 
>> I.e. it would be quite easy for a specific scheduler to zap one or more
>> of its pointers.
> 
> So you suggest to ASSERT all pointers before dereferencing them? Why
> don't we have such ASSERTs in places where we use function vectors
> hooked to dynamic data (and I don't mean the single functions, but the
> pointers to the vector, e.g. domain->arch.ctxt_switch)?

Where justified I'm certainly in favor of omitting such checks, but
without the constification suggested I'm not convinced there is
sufficient justification. But here it's the scheduler maintainer to
judge anyway - I've merely voiced an opinion.

> Seriously, that would be a major programming bug and I don't think
> we need to catch that by debug code sprinkled around everywhere.

In fact we've been discussing to gradually add such checks, in
order to trade - as explained by George - privilege escalations for
DoS-es.

> After my core scheduling series is finished I'd like to do a major
> scheduler cleanup series. One action item will be to have a single
> instance const scheduler_funcs structure for each scheduler and a
> per-cpupool scheduler_data pointer.

That's good to know, being exactly what I would have hoped things
would be.

Jan



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