linux-kernel - Re: [PATCH] cpu/hotplug: ensure the starting section runs fully regardless of target

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wmg9oyzk.ffs@tglx>
Date: Mon, 09 Dec 2024 12:51:59 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-kernel@...r.kernel.org, peterz@...radead.org
Subject: Re: [PATCH] cpu/hotplug: ensure the starting section runs fully
 regardless of target

On Mon, Dec 09 2024 at 13:19, Koichiro Den wrote:
> Now I'm wondering whether we should go further..
> Because in PREPARE section, things are not fully symmetric, so
> there is a problem like an example below:
>
>     E.g.
>
>     (1) writing <some state in between> into 'target' and then (2) bringing
>     the the cpu fully online again by writing a large value leaves
>     hrtimer_cpu_base's 'online' field at 0 because hrtimers:prepare does not
>     re-run.
>
>            - hrtimers:prepare (CPUHP_HRTIMERS_PREPARE)
>            - ...
>     ------ - <some state in between>
>      ^  :  - ...
>      :  :  - hrtimers:dying (CPUHP_AP_HRTIMERS_DYING)
>     (1)(2)
>      :  :
>      :  v

That got broken by me with commit:

  5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier")

and want's to be fixed.

> While I understand this is still a debug option, it seems to me that there are
> several approaches to consider here. I'm inclined toward (a).
>
> (a). prohibit writing all halfway states in PREPARE+STARTING sections, i.e.
>
>      --- a/kernel/cpu.c
>      +++ b/kernel/cpu.c
>      @@ -2759,7 +2759,8 @@ static ssize_t target_store(struct device *dev,
>                      return ret;
>      
>       #ifdef CONFIG_CPU_HOTPLUG_STATE_CONTROL
>      -       if (target < CPUHP_OFFLINE || target > CPUHP_ONLINE)
>      +       if (target != CPUHP_OFFLINE || target > CPUHP_ONLINE ||
>      +           target < CPUHP_AP_OFFLINE_IDLE)
>                      return -EINVAL;
>       #else
>              if (target != CPUHP_OFFLINE && target != CPUHP_ONLINE)

That's lame.

> (b). make all fully symmetric. (I'm not sure whether it could be possible)

There is no requirement to make everything symmetric as there are
prepare steps which just allocate memory and do some basic
initialization. So I rather go through the steps and keep the ability to
invoke them fine granular in all directions (except for the atomic AP
states which started this discussion.

> (c). add all safety catch at sysfs interface. (For the above example, once
>      it goes down to <some state in between>, disallow to go up without
>      once going down to a state earlier than hrtimers:prepare beforehand.
>      I guess this would mess up source code unnecessarily though.)

That's unmaintainable.

Thanks,

        tglx