linux-kernel - Re: [PATCH v2 11/11] mm,sched: conditionally skip lazy TLB mm refcounting

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: <91F4157A-8004-46EC-AE04-F2F338B0D5BB@amacapital.net>
Date:   Mon, 30 Jul 2018 15:00:40 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Rik van Riel <riel@...riel.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-team <kernel-team@...com>, X86 ML <x86@...nel.org>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Ingo Molnar <mingo@...nel.org>, Mike Galbraith <efault@....de>,
        Dave Hansen <dave.hansen@...el.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [PATCH v2 11/11] mm,sched: conditionally skip lazy TLB mm refcounting



> On Jul 30, 2018, at 2:46 PM, Rik van Riel <riel@...riel.com> wrote:
> 
>> On Mon, 2018-07-30 at 12:49 -0700, Andy Lutomirski wrote:
>> 
>> 
>> I think it's a big step in the right direction, but it still makes be
>> nervous.  I'd be more comfortable with it if you at least had a
>> functional set of patches that result in active_mm being gone,
>> because
>> that will mean that you actually audited the whole mess and fixed
>> anything that might rely on active_mm pointing somewhere or that
>> might
>> be putting a value you didn't take into account into active_mm.  IOW
>> I'm not totally thrilled by applying the patches as is if we're still
>> a bit unsure as to what might have gotten missed.
>> 
>> I don't think it's at all necessary to redo the patches.
>> 
>> Does that seem reasonable?
> 
> Absolutely. I tried to keep ->active_mm very similar
> to before for exactly that reason.
> 
> Lets go through all the places where it is used, in
> x86 and architecture independent code. I have not
> checked other architectures.
> 
> It looks like we should be able to get rid of
> ->active_mm at some point, but a lot of it depends
> on other architecture maintainers.
> 
> 
> arch/x86/events/core.c:
> - get_segment_base: get current->active_mm->context.ldt,
>  this appears to be for TIF_IA32 user programs only, so
>  we should be able to use current->mm here

->mm sounds more correct anyway

> 
> arch/x86/kernel/cpu/common.c:
> - current task's ->active_mm assigned in two places,
>  never read
> 
> arch/x86/lib/insn-eval.c:
> - get_desc() gets current->active_mm->context.ldt, this
>  appears to be only for user space programs

Same as above

> 
> arch/x86/mm/tlb.c:
> - this series adds two places where current->active_mm is
>  written, it is never read
> 
> arch/x86/platform/efi/efi_64.c:
> - current->active_mm is set to efi_mm for a little bit,
>  with irqs disabled, and then changed back, with irqs still
>  disabled; we should be able to get rid of ->active_mm here
> - in the init code, ->active_mm is set to efi_mm as well,
>  presumably the kernel automatically switches that back on
>  the next context switch; this may be buggy, since preemption
>  is enabled and a GFP_KERNEL allocation is just a few lines
>  below

Ick. This should mostly go away soon — most EFI code will move to a real thread.

> 
> arch/x86/power/cpu.c:
> - fix_processor_context() calls load_mm_ldt(current->active_mm);,
>  we should be able to use cpu_tlbstate.loaded_mm instead

Agreed

> 
> drivers/cpufreq/pmac32-cpufreq.c:
> - pmu_set_cpu_speed() restores current->active_mm - don't know if
>  anyone still cares about 32 bit PPC :)
> 

I think we should only remove active_mm when the new #define is set. So this doesn’t need to change.

> drivers/firmware/efi/arm-runtime.c:
> - efi_virtmap_unload switches back the pgd to current->active_mm
>  from &efi_mm; that mm could be stored elsewhere if we excised
>  ->active_mm everywhere

Ditto

IOW the active_mm refcounting may be genuinely useful for architectures that are not able to efficiently shoot down remote lazy mm references in exit_mmap(). I suspect that ARM64 may be in that category.