Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer

[qianli zhao]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> 于2020年8月13日周四 下午6:46写道：
>
> Qianli Zhao <zhaoqianligood@xxxxxxxxx> writes:
>
> Please start the first word after the colon with an upper case letter.
>
> > do_init_timer can specify flags of timer_list,
>
> Please write do_init_timer() so it's entirely clear that this is about a
> function.
>
> > but this function does not expect to specify the CPU or idx.
>
> or idx does not mean anything unless someone looks at the
> code. Changelogs want to explain things so they can be understood
> without staring at the code.

will update changelog

> > If user invoking do_init_timer and specify CPU,
> > The result may not what we expected.
>
> Right. And which caller exactly hands in crappy flags?

This change is more of a sanity check to avoid these wrong use

> > E.g:
> > do_init_timer invoked in core2,and specify flags 0x1
> > final result flags is 0x3.If the specified CPU number
> > exceeds the actual number,more serious problems will happen
>
> More serious problems is not a really helpful technical explanation and
> 0x3 does not make sense for a changelog reader either because it again
> requires to look up the code.
>
> >       timer->entry.pprev = NULL;
> >       timer->function = func;
> > -     timer->flags = flags | raw_smp_processor_id();
> > +     timer->flags = (flags & ~TIMER_BASEMASK & ~TIMER_ARRAYMASK) |
> > raw_smp_processor_id();
>
> If the caller hands in invalid flags then silently fixing them up is
> fundamentally wrong. So this wants to be:
>
>    if (WARN_ON(flags & ~TIMER_INIT_FLAGS))
>         flags &= TIMER_INIT_FLAGS;
>
> and TIMER_INIT_FLAGS wants to be exactly the set of flags which are
> valid for being handed in by a caller, i.e.:
>
>       TIMER_DEFFERABLE, TIMER_PINNED, TIMER_IRQSAFE

This change is very good，thanks for teaching

> Guess what happens when the caller hands in TIMER_MIGRATING?

If TIMER_MIGRATING is set in timer_setup, lock_timer_base will fall
into a dead loop

> If we do sanity checking then we do it correct and not just silently
> papering over the particular problem which you ran into.

Thanks for teaching.

I have updated patchset,please review.

> Thanks,
>
>         tglx