| 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
| ||
|
Message-ID: <1664989063.2279.1596804857895.JavaMail.zimbra@efficios.com>
Date: Fri, 7 Aug 2020 08:54:17 -0400 (EDT)
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: 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>,
Peter Oskolkov <posk@...k.io>
Subject: Re: [PATCH 2/2 v2] rseq/selftests: test
MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU
----- On Aug 6, 2020, at 8:27 PM, Boqun Feng boqun.feng@...il.com wrote:
> On Thu, Aug 06, 2020 at 10:05:44AM -0700, Peter Oskolkov wrote:
>> Based on Google-internal RSEQ work done by
>> Paul Turner and Andrew Hunter.
>>
>> This patch adds a selftest for MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU.
>> The test quite often fails without the previous patch in this patchset,
>> but consistently passes with it.
>>
>> Signed-off-by: Peter Oskolkov <posk@...gle.com>
>> ---
>> .../selftests/rseq/basic_percpu_ops_test.c | 181 ++++++++++++++++++
>> 1 file changed, 181 insertions(+)
>>
>> diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> index eb3f6db36d36..147c80deac19 100644
>> --- a/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> +++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
>> @@ -3,16 +3,21 @@
>> #include <assert.h>
>> #include <pthread.h>
>> #include <sched.h>
>> +#include <stdatomic.h>
>> #include <stdint.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <stddef.h>
>> +#include <syscall.h>
>> +#include <unistd.h>
>>
>> #include "rseq.h"
>>
>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>>
>> +#define MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU (1<<7)
>> +
>> struct percpu_lock_entry {
>> intptr_t v;
>> } __attribute__((aligned(128)));
>> @@ -289,6 +294,180 @@ void test_percpu_list(void)
>> assert(sum == expected_sum);
>> }
>>
>> +struct test_membarrier_thread_args {
>> + int stop;
>> + intptr_t percpu_list_ptr;
>> +};
>> +
>> +/* Worker threads modify data in their "active" percpu lists. */
>> +void *test_membarrier_worker_thread(void *arg)
>> +{
>> + struct test_membarrier_thread_args *args =
>> + (struct test_membarrier_thread_args *)arg;
>> + const int iters = 10 * 1000 * 1000;
>> + int i;
>> +
>> + if (rseq_register_current_thread()) {
>> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> +
>> + for (i = 0; i < iters; ++i) {
>> + while (true) {
>> + int cpu, ret;
>> + struct percpu_list *list_ptr = (struct percpu_list *)
>> + atomic_load(&args->percpu_list_ptr);
>> +
>
> 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?
I'm with you on this, something looks fishy. It would be good to use
delay-inducing testing methods like rseq parametrized selftests to
increase the odds of hitting this race more reliably.
Thanks,
Mathieu
>
> Regards,
> Boqun
>
>> + if (!list_ptr)
>> + continue; /* Not yet initialized. */
>> +
>> + cpu = rseq_cpu_start();
>> + struct percpu_list_node *node = list_ptr->c[cpu].head;
>> + const intptr_t prev = node->data;
>> +
>> + ret = rseq_cmpeqv_cmpeqv_storev(&node->data, prev,
>> + &args->percpu_list_ptr,
>> + (intptr_t)list_ptr, prev + 1, cpu);
>> + if (!ret)
>> + break; /* Success. */
>> + }
>> + }
>> +
>> + if (rseq_unregister_current_thread()) {
>> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d):
>> %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> + return NULL;
>> +}
>> +
>> +void test_membarrier_init_percpu_list(struct percpu_list *list)
>> +{
>> + int i;
>> +
>> + memset(list, 0, sizeof(*list));
>> + for (i = 0; i < CPU_SETSIZE; i++) {
>> + struct percpu_list_node *node;
>> +
>> + node = malloc(sizeof(*node));
>> + assert(node);
>> + node->data = 0;
>> + node->next = NULL;
>> + list->c[i].head = node;
>> + }
>> +}
>> +
>> +void test_membarrier_free_percpu_list(struct percpu_list *list)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + free(list->c[i].head);
>> +}
>> +
>> +static int sys_membarrier(int cmd, int flags)
>> +{
>> + return syscall(__NR_membarrier, cmd, flags);
>> +}
>> +
>> +/*
>> + * The manager thread swaps per-cpu lists that worker threads see,
>> + * and validates that there are no unexpected modifications.
>> + */
>> +void *test_membarrier_manager_thread(void *arg)
>> +{
>> + struct test_membarrier_thread_args *args =
>> + (struct test_membarrier_thread_args *)arg;
>> + struct percpu_list list_a, list_b;
>> + intptr_t expect_a = 0, expect_b = 0;
>> + int cpu_a = 0, cpu_b = 0;
>> +
>> + if (rseq_register_current_thread()) {
>> + fprintf(stderr, "Error: rseq_register_current_thread(...) failed(%d): %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> +
>> + /* Init lists. */
>> + test_membarrier_init_percpu_list(&list_a);
>> + test_membarrier_init_percpu_list(&list_b);
>> +
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> +
>> + while (!atomic_load(&args->stop)) {
>> + /* list_a is "active". */
>> + cpu_a = rand() % CPU_SETSIZE;
>> + /*
>> + * As list_b is "inactive", we should never see changes
>> + * to list_b.
>> + */
>> + if (expect_b != atomic_load(&list_b.c[cpu_b].head->data)) {
>> + fprintf(stderr, "Membarrier test failed\n");
>> + abort();
>> + }
>> +
>> + /* Make list_b "active". */
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_b);
>> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_a);
>> + /*
>> + * Cpu A should now only modify list_b, so we values
>> + * in list_a should be stable.
>> + */
>> + expect_a = atomic_load(&list_a.c[cpu_a].head->data);
>> +
>> + cpu_b = rand() % CPU_SETSIZE;
>> + /*
>> + * As list_a is "inactive", we should never see changes
>> + * to list_a.
>> + */
>> + if (expect_a != atomic_load(&list_a.c[cpu_a].head->data)) {
>> + fprintf(stderr, "Membarrier test failed\n");
>> + abort();
>> + }
>> +
>> + /* Make list_a "active". */
>> + atomic_store(&args->percpu_list_ptr, (intptr_t)&list_a);
>> + sys_membarrier(MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU, cpu_b);
>> + /* Remember a value from list_b. */
>> + expect_b = atomic_load(&list_b.c[cpu_b].head->data);
>> + }
>> +
>> + test_membarrier_free_percpu_list(&list_a);
>> + test_membarrier_free_percpu_list(&list_b);
>> +
>> + if (rseq_unregister_current_thread()) {
>> + fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d):
>> %s\n",
>> + errno, strerror(errno));
>> + abort();
>> + }
>> + return NULL;
>> +}
>> +
>> +/* Test MEMBARRIER_CMD_PRIVATE_RESTART_RSEQ_ON_CPU membarrier command. */
>> +void test_membarrier(void)
>> +{
>> + struct test_membarrier_thread_args thread_args;
>> + pthread_t worker_threads[CPU_SETSIZE];
>> + pthread_t manager_thread;
>> + int i;
>> +
>> + thread_args.stop = 0;
>> + thread_args.percpu_list_ptr = 0;
>> + pthread_create(&manager_thread, NULL,
>> + test_membarrier_manager_thread, &thread_args);
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + pthread_create(&worker_threads[i], NULL,
>> + test_membarrier_worker_thread, &thread_args);
>> +
>> + for (i = 0; i < CPU_SETSIZE; i++)
>> + pthread_join(worker_threads[i], NULL);
>> +
>> + atomic_store(&thread_args.stop, 1);
>> + pthread_join(manager_thread, NULL);
>> +}
>> +
>> int main(int argc, char **argv)
>> {
>> if (rseq_register_current_thread()) {
>> @@ -300,6 +479,8 @@ int main(int argc, char **argv)
>> test_percpu_spinlock();
>> printf("percpu_list\n");
>> test_percpu_list();
>> + printf("membarrier\n");
>> + test_membarrier();
>> if (rseq_unregister_current_thread()) {
>> fprintf(stderr, "Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
>> errno, strerror(errno));
>> --
>> 2.28.0.163.g6104cc2f0b6-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Powered by blists - more mailing lists