linux-kernel - Re: [PATCH] x86/mm/tlb: ignore f->new_tlb

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]

Date:   Fri, 8 Jul 2022 17:04:12 +0000
From:   Nadav Amit <namit@...are.com>
To:     Dave Hansen <dave.hansen@...el.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "x86@...nel.org" <x86@...nel.org>, Linux MM <linux-mm@...ck.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH] x86/mm/tlb: ignore f->new_tlb_gen when zero

On Jul 8, 2022, at 7:49 AM, Dave Hansen <dave.hansen@...el.com> wrote:

> ⚠ External Email
> 
> On 7/7/22 17:30, Nadav Amit wrote:
> 
> You might want to fix the clock on the system from which you sent this.
> I was really scratching my head trying to figure out how you got this
> patch out before Hugh's bug report.
> 
>> From: Nadav Amit <namit@...are.com>
>> 
>> Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when
>> possible") introduced an optimization of skipping the flush if the TLB
>> generation that is flushed (as provided in flush_tlb_info) was already
>> flushed.
>> 
>> However, arch_tlbbatch_flush() does not provide any generation in
>> flush_tlb_info. As a result, try_to_unmap_one() would not perform any
>> TLB flushes.
>> 
>> Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is
>> anyhow is an invalid generation value.
> 
> It is, but the check below uses 'f->end == TLB_FLUSH_ALL' as the marker
> for f->new_tlb_gen being invalid.  Being consistent seems like a good
> idea on this stuff.

If we get a request to do a flush, regardless whether full or partial,
that logically we have already done, there is not reason to do it.

I therefore do not see a reason to look on f->end. I think that looking
at the generation is very intuitive. If you want, I can add a constant
such as TLB_GENERATION_INVALID.

> 
>> In addition, add the missing unlikely() and jump to get tracing right.
> 
> There are currently five routes out of flush_tlb_func():
> * Three early returns
> * One 'goto done'
> * One implicit return
> 
> The tracing code doesn't get run for any of the early returns, but
> that's intentional because they don't *actually* flush the TLB.  We
> don't want to record that flush_tlb_func() flushed the TLB when it
> didn't.  There's another tracepoint on the TLB_REMOTE_SEND_IPI side to
> tell where the flushes were requested.
> 
> That said, I think the
> 
>        if (unlikely(local_tlb_gen == mm_tlb_gen))
>                goto done;
> 
> is arguably buggy, as is the 'goto done' in this hunk:

I was just trying to follow it for consistency. Will remove.

> 
> We might want to (eventually) think about doing something like the
> attached patch to make the skipped flushes explicit in the tracing and
> make the return paths out of this function more consistent.

That’s fine with me. I just recommend that you have a single tracing call in
the function, since having too many ruins the generated code.