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

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

[Qianli Zhao]

From: Qianli Zhao <zhaoqianli@xiaomi.com>

do_init_timer can specify flags of timer_list,
but this function does not expect to specify the CPU or idx.
If user invoking do_init_timer and specify CPU,
The result may not what we expected.

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

Signed-off-by: Qianli Zhao <zhaoqianli@xiaomi.com>
---
 kernel/time/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 026ac01..17e3737 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -789,7 +789,7 @@ static void do_init_timer(struct timer_list *timer,
 {
 	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();
 	lockdep_init_map(&timer->lockdep_map, name, key, 0);
 }
 
-- 
2.7.4

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

[Thomas Gleixner]

Qianli Zhao <zhaoqianligood@gmail.com> 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.

> 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?

> 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

Guess what happens when the caller hands in TIMER_MIGRATING?

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

Thanks,

        tglx

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

[qianli zhao]

Thomas Gleixner <tglx@linutronix.de> 于2020年8月13日周四 下午6:46写道：
>
> Qianli Zhao <zhaoqianligood@gmail.com> 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