linux-kernel - Re: [PATCH 3/9] x86/mm/tlb: Refactor common code into flush_tlb_on

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: <30E271DF-8D6B-4D5F-8033-9B17F6990994@vmware.com>
Date:   Wed, 26 Jun 2019 01:57:45 +0000
From:   Nadav Amit <namit@...are.com>
To:     Dave Hansen <dave.hansen@...el.com>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "x86@...nel.org" <x86@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 3/9] x86/mm/tlb: Refactor common code into
 flush_tlb_on_cpus()

> On Jun 25, 2019, at 2:07 PM, Dave Hansen <dave.hansen@...el.com> wrote:
> 
> On 6/12/19 11:48 PM, Nadav Amit wrote:
>> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
>> index 91f6db92554c..c34bcf03f06f 100644
>> --- a/arch/x86/mm/tlb.c
>> +++ b/arch/x86/mm/tlb.c
>> @@ -734,7 +734,11 @@ static inline struct flush_tlb_info *get_flush_tlb_info(struct mm_struct *mm,
>> 			unsigned int stride_shift, bool freed_tables,
>> 			u64 new_tlb_gen)
>> {
>> -	struct flush_tlb_info *info = this_cpu_ptr(&flush_tlb_info);
>> +	struct flush_tlb_info *info;
>> +
>> +	preempt_disable();
>> +
>> +	info = this_cpu_ptr(&flush_tlb_info);
>> 
>> #ifdef CONFIG_DEBUG_VM
>> 	/*
>> @@ -762,6 +766,23 @@ static inline void put_flush_tlb_info(void)
>> 	barrier();
>> 	this_cpu_dec(flush_tlb_info_idx);
>> #endif
>> +	preempt_enable();
>> +}
> 
> The addition of this disable/enable pair is unchangelogged and
> uncommented.  I think it makes sense since we do need to make sure we
> stay on this CPU, but it would be nice to mention.

I’ll add some comments and update the changeling . I see I marked
get_flush_tlb_info() as “inline” for no reason. I’m going to remove it in
this patch, unless you say it should be in a separate patch.

>> +static void flush_tlb_on_cpus(const cpumask_t *cpumask,
>> +			      const struct flush_tlb_info *info)
>> +{
> 
> Might be nice to mention that preempt must be disabled.  It's kinda
> implied from the smp_processor_id(), but being explicit is always nice too.

I will add a comment, although smp_processor_id() should anyhow shout at you
if you use it with CONFIG_DEBUG_PREEMPT=y.

>> +	int this_cpu = smp_processor_id();
>> +
>> +	if (cpumask_test_cpu(this_cpu, cpumask)) {
>> +		lockdep_assert_irqs_enabled();
>> +		local_irq_disable();
>> +		flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN);
>> +		local_irq_enable();
>> +	}
>> +
>> +	if (cpumask_any_but(cpumask, this_cpu) < nr_cpu_ids)
>> +		flush_tlb_others(cpumask, info);
>> }
>> 
>> void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>> @@ -770,9 +791,6 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>> {
>> 	struct flush_tlb_info *info;
>> 	u64 new_tlb_gen;
>> -	int cpu;
>> -
>> -	cpu = get_cpu();
>> 
>> 	/* Should we flush just the requested range? */
>> 	if ((end == TLB_FLUSH_ALL) ||
>> @@ -787,18 +805,18 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>> 	info = get_flush_tlb_info(mm, start, end, stride_shift, freed_tables,
>> 				  new_tlb_gen);
>> 
>> -	if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
>> -		lockdep_assert_irqs_enabled();
>> -		local_irq_disable();
>> -		flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN);
>> -		local_irq_enable();
>> -	}
>> +	/*
>> +	 * Assert that mm_cpumask() corresponds with the loaded mm. We got one
>> +	 * exception: for init_mm we do not need to flush anything, and the
>> +	 * cpumask does not correspond with loaded_mm.
>> +	 */
>> +	VM_WARN_ON_ONCE(cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm)) !=
>> +			(mm == this_cpu_read(cpu_tlbstate.loaded_mm)) &&
>> +			mm != &init_mm);
> 
> Very very cool.  You thought "these should be equivalent", and you added
> a corresponding warning to ensure they are.

The credit for this assertion goes to Peter who suggested I add it...

> 
>> -	if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
>> -		flush_tlb_others(mm_cpumask(mm), info);
>> +	flush_tlb_on_cpus(mm_cpumask(mm), info);
>> 
>> 	put_flush_tlb_info();
>> -	put_cpu();
>> }
> 
> 
> 
> Reviewed-by: Dave Hansen <dave.hansen@...ux.intel.com>

Thanks for the reviews of this patch and the others (don’t worry, I won’t
add the “Reviewed-by” tag to the others).