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: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.

Jan



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