linux-kernel - Re: [PATCH 2/2 v2] rseq/selftests: test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_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]

Date:   Fri, 7 Aug 2020 14:25:15 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Peter Oskolkov <posk@...k.io>
Cc:     Boqun Feng <boqun.feng@...il.com>,
        Peter Oskolkov <posk@...gle.com>, paulmck <paulmck@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Paul Turner <pjt@...gle.com>,
        Chris Kennelly <ckennelly@...gle.com>
Subject: Re: [PATCH 2/2 v2] rseq/selftests: test
 MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU

----- On Aug 7, 2020, at 1:55 PM, Peter Oskolkov posk@...k.io wrote:

> On Thu, Aug 6, 2020 at 5:27 PM Boqun Feng <boqun.feng@...il.com> wrote:
[...]
>> What if the manager thread update ->percpu_list_ptr and call
>> membarrier() here? I.e.
>>
>>         CPU0                    CPU1
>>                                 list_ptr = atomic_load(&args->percpu_list_ptr); // read list_b
>>
>>         atomic_store(&args->percpu_list_ptr, list_a);
>>         sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, 1); // send ipi to
>>         restart rseq.cs on CPU1
>>
>>                                 <got IPI, but not in a rseq.cs, so nothing to do>
>>                                 cpu = rseq_cpu_start(); // start a rseq.cs and accessing list_b!
>>
>> The thing is, atomic_load() is an reference to ->percpu_list_ptr, which
>> is outside the rseq.cs, simply restarting rseq doesn't kill this
>> reference.
>>
>> Am I missing something subtle?
> 
> rseq_cmpeqv_cmpeqv_store is used below to make sure the reference is
> the one that should be used; if it is no longer "active", the
> iteration is restarted.

I suspect it "works" because the manager thread does not free and
repurpose the memory where list_a is allocated, nor does it store to
its list head (which would corrupt the pointer dereferenced by CPU 1
in the scenario above). This shares similarities with type-safe memory
allocation (see SLAB_TYPESAFE_BY_RCU).

Even though it is not documented as such (or otherwise) in the example code,
I feel this example looks like it guarantees that the manager thread "owns"
list_a after the rseq-fence, when in fact it can still be read by the rseq
critical sections.

AFAIU moving the atomic_load(&args->percpu_list_ptr) into the critical section
should entirely solve this and guarantee exclusive access to the old list
after the manager's rseq-fence. I wonder why this simpler approach is not
favored ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com