Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Qianli Zhao <zhaoqianligood@gmail.com>, john.stultz@linaro.org,
        sboyd@kernel.org
Cc:     linux-kernel@vger.kernel.org, zhaoqianli@xiaomi.com
Subject: Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer
In-Reply-To: <1596767378-27241-1-git-send-email-zhaoqianligood@gmail.com>
References: <1596767378-27241-1-git-send-email-zhaoqianligood@gmail.com>
Date:   Thu, 13 Aug 2020 12:46:05 +0200
Message-ID: <87h7t6lt76.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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