Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer
From: qianli zhao
Date: Thu Aug 13 2020 - 11:17:32 EST
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
> > 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.