linux-kernel - Re: [PATCH] timer: mask unnecessary set of flags in do_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPx_LQEPRk1UnOkfPaU_Lfp-nQFe9bybeLFJ3X4wybGMzkhtmQ@mail.gmail.com>
Date:   Thu, 13 Aug 2020 23:17:16 +0800
From:   qianli zhao <zhaoqianligood@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     John Stultz <john.stultz@...aro.org>, sboyd@...nel.org,
        linux-kernel@...r.kernel.org, zhaoqianli@...omi.com
Subject: Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer

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