[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 1 Oct 2020 08:59:48 +1000
From: "Singh, Balbir" <sblbir@...zon.com>
To: Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>
CC: Lukas Bulwahn <lukas.bulwahn@...il.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Andy Lutomirski <luto@...nel.org>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"x86@...nel.org" <x86@...nel.org>,
"H. Peter Anvin" <hpa@...or.com>,
Nathan Chancellor <natechancellor@...il.com>,
Nick Desaulniers <ndesaulniers@...gle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"clang-built-linux@...glegroups.com"
<clang-built-linux@...glegroups.com>,
"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
"linux-safety@...ts.elisa.tech" <linux-safety@...ts.elisa.tech>
Subject: Re: [PATCH -next for tip:x86/pti] x86/tlb: drop unneeded local vars in
enable_l1d_flush_for_task()
On 1/10/20 7:38 am, Thomas Gleixner wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Wed, Sep 30 2020 at 20:35, Peter Zijlstra wrote:
>> On Wed, Sep 30, 2020 at 08:00:59PM +0200, Thomas Gleixner wrote:
>>> On Wed, Sep 30 2020 at 19:03, Peter Zijlstra wrote:
>>>> On Wed, Sep 30, 2020 at 05:40:08PM +0200, Thomas Gleixner wrote:
>>>> Also, that preempt_disable() in there doesn't actually do anything.
>>>> Worse, preempt_disable(); for_each_cpu(); is an anti-pattern. It mixes
>>>> static_cpu_has() and boot_cpu_has() in the same bloody condition and has
>>>> a pointless ret variable.
>>
>> Also, I forgot to add, it accesses ->cpus_mask without the proper
>> locking, so it could be reading intermediate state from whatever cpumask
>> operation that's in progress.
>
> Yes. I saw that after hitting send. :(
>
>>> I absolutely agree and I really missed it when looking at it before
>>> merging. cpus_read_lock()/unlock() is the right thing to do if at all.
>>>
>>>> It's shoddy code, that only works if you align the planets right. We
>>>> really shouldn't provide interfaces that are this bad.
>>>>
>>>> It's correct operation is only by accident.
>>>
>>> True :(
>>>
>>> I understand Balbirs problem and it makes some sense to provide a
>>> solution. We can:
>>>
>>> 1) reject set_affinity() if the task has that flush muck enabled
>>> and user space tries to move it to a SMT enabled core
>>>
>>> 2) disable the muck if if detects that it is runs on a SMT enabled
>>> core suddenly (hotplug says hello)
>>>
>>> This one is nasty because there is no feedback to user space
>>> about the wreckage.
>>
>> That's and, right, not or. because 1) deals with sched_setffinity()
>> and 2) deals wit hotplug.
>
> It was meant as AND of course.
>
>> Now 1) requires an arch hook in sched_setaffinity(), something I'm not
>> keen on providing, once we provide it, who knows what strange and
>> wonderful things archs will dream up.
>
> I don't think so. We can have that magic in core:
>
> #ifdef CONFIG_HAS_PARANOID_L1D_FLUSH
> static bool paranoid_l1d_valid(struct task_struct *tsk,
> const struct cpumask *msk)
> {
> if (!test_tsk_thread_flag(tsk, TIF_SPEC_L1D_FLUSH))
> return true;
> /* Do magic stuff */
> return res;
> }
> #else
> static bool paranoid_l1d_valid(struct task_struct *tsk,
> const struct cpumask *msk)
> {
> return true;
> }
> #endif
>
> It's a pretty well defined problem and having the magic in core code
> prevents an arch hook which allows abuse of all sorts.
>
> And the same applies to enable_l1d_flush_for_task(). The only
> architecture specific nonsense are the checks whether the CPU bug is
> there and whether the hardware supports L1D flushing.
>
> So we can have:
>
> #ifdef CONFIG_HAS_PARANOID_L1D_FLUSH
> int paranoid_l1d_enable(struct task_struct *tsk)
> {
> /* Do the SMT validation under the proper locks */
> if (!res)
> set_task_thread_flag(tsk, TIF_SPEC_L1D_FLUSH);
> return res;
> }
> #endif
>
>> And 2) also happens on hot-un-plug, when the task's affinity gets
>> forced because it became empty. No user feedback there either, and
>> information is lost.
>
> Of course. It's both that suddenly SMT gets enabled on a core which was
> isolated and when the last isolated core in the tasks CPU mask goes
> offline.
>
>> I suppose we can do 2) but send a signal. That would cover all cases and
>> keep it in arch code. But yes, that's pretty terrible too.
>
> Bah. I just looked at the condition to flush:
>
> if (sched_smt_active() && !this_cpu_read(cpu_info.smt_active) &&
> (prev_mm & LAST_USER_MM_L1D_FLUSH))
> l1d_flush_hw();
>
> That fails to flush when SMT is disabled globally. Balbir?
It should have been
!sched_smt_active() || (cond)
and not
sched_smt_active && (cond)
I'll fix that up, but your simplification below works as well.
>
> Of course this should be:
>
> if (!this_cpu_read(cpu_info.smt_active) && (prev_mm & LAST_USER_MM_L1D_FLUSH))
> l1d_flush_hw();
>
> Now we can make this:
>
> if (unlikely(prev_mm & LAST_USER_MM_L1D_FLUSH)) {
> if (!this_cpu_read(cpu_info.smt_active))
> l1d_flush_hw();
> else
> task_work_add(...);
>
> And that task work clears the flag and sends a signal. We're not going
> to send a signal from switch_mm() ....
>
Yes, I see MCE handling uses a similar pattern, so SIGBUS for a task that migrates/moves
to a SMT disabled core?
Thanks,
Balbir
Powered by blists - more mailing lists