linux-kernel - Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor

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: <CAJZ5v0iHu3ZZuHeC7q6x4ZERaAu0pP2ubqzUv3v2upxLwOFXsg@mail.gmail.com>
Date:   Wed, 12 Oct 2022 18:58:42 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Chen Yu <yu.c.chen@...el.com>
Cc:     Pavel Machek <pavel@....cz>, Sasha Levin <sashal@...nel.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        rafael@...nel.org, daniel.lezcano@...aro.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu()
 instead of smp_processor_id() to avoid crash

On Tue, Oct 11, 2022 at 3:23 PM Chen Yu <yu.c.chen@...el.com> wrote:
>
> Hi Pavel,
> On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> > Hi!
> >
> > > From: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> > >
> > > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > >
> > > When CPU 0 is offline and intel_powerclamp is used to inject
> > > idle, it generates kernel BUG:
> > >
> > > BUG: using smp_processor_id() in preemptible [00000000] code: bash/15687
> > > caller is debug_smp_processor_id+0x17/0x20
> > > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > > Call Trace:
> > > <TASK>
> > > dump_stack_lvl+0x49/0x63
> > > dump_stack+0x10/0x16
> > > check_preemption_disabled+0xdd/0xe0
> > > debug_smp_processor_id+0x17/0x20
> > > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > > ...
> > > ...
> > >
> > > Here CPU 0 is the control CPU by default and changed to the current CPU,
> > > if CPU 0 offlined. This check has to be performed under cpus_read_lock(),
> > > hence the above warning.
> > >
> > > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> >
> > This has exactly the same problem as smp_processor_id(), you just
> > worked around the warning. If it is okay that control_cpu contains
> > stale value, could we have a comment explaining why?
> >
> May I know why does control_cpu have stale value? The control_cpu
> is a random picked online CPU which will be used later to collect statistics.
> As long as the control_cpu is online, it is valid IMO.

So this is confusing, because the code makes the impression that
getting the number of the CPU running the code matters in some way,
which isn't the case.

Something like cpumask_first(cpu_online_mask) should work as well if
I'm not mistaken and it would be less confusing to use this instead
IMO.