linux-kernel - Re: [PATCH RFC UGLY] x86,mm,sched: make lazy TLB mode even lazier

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:   Mon, 29 Aug 2016 12:08:52 -0400
From:   Rik van Riel <riel@...hat.com>
To:     "H. Peter Anvin" <hpa@...or.com>, serebrin@...gle.com
Cc:     mingo@...nel.org, peterz@...radead.org, torvalds@...nel.org,
        linux-kernel@...r.kernel.org, luto@...nel.org, bp@...e.de,
        mgorman@...e.de, tglx@...utronix.de
Subject: Re: [PATCH RFC UGLY] x86,mm,sched: make lazy TLB mode even lazier

On Thu, 2016-08-25 at 12:42 -0700, H. Peter Anvin wrote:

> > +static void set_lazy_tlbstate_flush(int cpu) {
> > +	if (per_cpu(cpu_tlbstate.state, cpu) == TLBSTATE_LAZY) {
> > +		raw_spin_lock(&cpu_rq(cpu)->lock);
> > +		if (per_cpu(cpu_tlbstate.state, cpu) ==
> > TLBSTATE_LAZY)
> > +			per_cpu(cpu_tlbstate.state, cpu) =
> > TLBSTATE_FLUSH;
> > +		raw_spin_unlock(&cpu_rq(cpu)->lock);
> > +	}
> > +}
> > +
> > 
> Why grabbing a lock instead of cmpxchg?

The second and third version of the patch had cmpxchg,
instead of grabbing the remote CPU's runqueue lock,
but I am no longer convinced it is safe.

At TLB invalidation time, we have this:

        int *tlbstate = &per_cpu(cpu_tlbstate.state, cpu);
        int old;

        switch (*tlbstate) {
        case TLBSTATE_LAZY:
                /*
                 * The CPU is in TLBSTATE_LAZY, which could context switch back
                 * to TLBSTATE_OK, re-using the old TLB state without a flush.
                 * If that happened, send a TLB flush IPI.
                 *
                 * Otherwise, the state is now TLBSTATE_FLUSH, and TLB will
                 * be flushed at the next context switch. Skip the IPI.
                 */ 
                old = cmpxchg(tlbstate, TLBSTATE_LAZY, TLBSTATE_FLUSH);
                return old != TLBSTATE_OK;

At context switch time, we have this:

                int oldstate = this_cpu_read(cpu_tlbstate.state);

                this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
                BUG_ON(this_cpu_read(cpu_tlbstate.active_mm) != next);

                if (oldstate == TLBSTATE_FLUSH ||
                                !cpumask_test_cpu(cpu, mm_cpumask(next))) {

In each case, the read will happen before the write, because
they are to the same address.

If the invalidate and context switch happen concurrently,
the writes can be ordered in two directions:

1) The cmpxchg in the TLB flush code happens after the
this_cpu_write in the context switch code. This is safe.

2) The cmpxchg in the TLB flush code happens before the
this_cpu_write in the context switch code. This is broken.

I can see two ways to fix that:
1) Change the write in the context switch code to a
   cmpxchg. I do not know how expensive this is on
   modern CPUs, or whether the overhead of doing this
   is unacceptable (or even noticeable, considering the
   cache line needs to be acquired for write anyway).
2) Acquire the runqueue lock of the remote CPU from the
   (much rarer?) TLB flush code, in order to ensure it
   does not run concurrently with the context switch
   code.

Any preferences?

-- 

All Rights Reversed.
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)