| 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: <618355303.19633.1511300666698.JavaMail.zimbra@efficios.com>
Date: Tue, 21 Nov 2017 21:44:26 +0000 (UTC)
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Shuah Khan <shuahkh@....samsung.com>
Cc: shuah <shuah@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Boqun Feng <boqun.feng@...il.com>,
Andy Lutomirski <luto@...capital.net>,
Dave Watson <davejwatson@...com>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-api <linux-api@...r.kernel.org>,
Paul Turner <pjt@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Russell King <linux@....linux.org.uk>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, Andrew Hunter <ahh@...gle.com>,
Andi Kleen <andi@...stfloor.org>, Chris Lameter <cl@...ux.com>,
Ben Maurer <bmaurer@...com>, rostedt <rostedt@...dmis.org>,
Josh Triplett <josh@...htriplett.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Michael Kerrisk <mtk.manpages@...il.com>,
linux-kselftest <linux-kselftest@...r.kernel.org>
Subject: Re: [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide
self-tests
----- On Nov 21, 2017, at 4:24 PM, Shuah Khan shuahkh@....samsung.com wrote:
> On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote:
>> ----- On Nov 21, 2017, at 12:40 PM, shuah shuah@...nel.org wrote:
>>
>>> On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote:
>>>> ----- On Nov 21, 2017, at 10:34 AM, shuah shuah@...nel.org wrote:
>>>>
>>>> [...]
>>>>>> ---
>>>>>> MAINTAINERS | 1 +
>>>>>> tools/testing/selftests/Makefile | 1 +
>>>>>> tools/testing/selftests/rseq/.gitignore | 4 +
>>>>>
>>>>> Thanks for the .gitignore files. It is commonly missed change, I end
>>>>> up adding one to clean things up after tests get in.
>>>>
>>>> I'm used to receive patches where contributors forget to add new files
>>>> to gitignore within my own projects, which may contribute to my awareness
>>>> of this pain point. :)
>>>>
>>>> [...]
>>>>
>>>>>> +
>>>>>> +void *test_percpu_inc_thread(void *arg)
>>>>>> +{
>>>>>> + struct inc_thread_test_data *thread_data = arg;
>>>>>> + struct inc_test_data *data = thread_data->data;
>>>>>> + long long i, reps;
>>>>>> +
>>>>>> + if (!opt_disable_rseq && thread_data->reg
>>>>>> + && rseq_register_current_thread())
>>>>>> + abort();
>>>>>> + reps = thread_data->reps;
>>>>>> + for (i = 0; i < reps; i++) {
>>>>>> + int cpu, ret;
>>>>>> +
>>>>>> +#ifndef SKIP_FASTPATH
>>>>>> + /* Try fast path. */
>>>>>> + cpu = rseq_cpu_start();
>>>>>> + ret = rseq_addv(&data->c[cpu].count, 1, cpu);
>>>>>> + if (likely(!ret))
>>>>>> + goto next;
>>>>>> +#endif
>>>>>
>>>>> So the test needs to compiled with this enabled? I think it would be better
>>>>> to make this an argument to be abel to select at test start time as opposed
>>>>> to making this compile time option. Remember that these tests get run in
>>>>> automated test rings. Making this a compile time otpion pertty much ensures
>>>>> that this path will not be tested.
>>>>>
>>>>> So I would reccommend adding a paratemer.
>>>>>
>>>>>> + slowpath:
>>>>>> + __attribute__((unused));
>>>>>> + for (;;) {
>>>>>> + /* Fallback on cpu_opv system call. */
>>>>>> + cpu = rseq_current_cpu();
>>>>>> + ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
>>>>>> + if (likely(!ret))
>>>>>> + break;
>>>>>> + assert(ret >= 0 || errno == EAGAIN);
>>>>>> + }
>>>>>> + next:
>>>>>> + __attribute__((unused));
>>>>>> +#ifndef BENCHMARK
>>>>>> + if (i != 0 && !(i % (reps / 10)))
>>>>>> + printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
>>>>>> +#endif
>>>>>
>>>>> Same comment as before. Avoid compile time options.
>>>>
>>>> The goal of those compiler define are to generate the altered code without
>>>> adding branches into the fast-paths.
>>>
>>> That makes sense. You are looking to not add any overhead.
>>>
>>>>
>>>> Here is an alternative solution that should take care of your concern: I'll
>>>> build multiple targets for param_test.c:
>>>>
>>>> param_test
>>>> param_test_skip_fastpath (built with -DSKIP_FASTPATH)
>>>> param_test_benchmark (build with -DBENCHMARK)
>>>>
>>>> I'll update run_param_test.sh to run both param_test and
>>>> param_test_skip_fastpath.
>>>>
>>>> Note that "param_test_benchmark" is only useful for benchmarking,
>>>> so I don't plan to run it from run_param_test.sh which is meant
>>>> to track regressions.
>>>>
>>>> Is that approach OK with you ?
>>>>
>>>
>>> Yes. This approach addresses my concern about coverage for both paths.
>>
>> fyi, the updated patches can be found here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=a0b8eb0eb5d4d8a280969370aa1dcf51801139c6
>> "selftests: lib.mk: Introduce OVERRIDE_TARGETS"
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=4ef0214e19bb7415fe7aed6852859b8d66e09a45
>> "cpu_opv: selftests: Implement selftests (v4)"
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=7d7530b843c7ecb50bea5a136c776cf3e9155d43
>> "rseq: selftests: Provide self-tests (v4)"
>>
>> Thanks for the feedback!
>>
>
> Are you going to send these to the mailing list? That way I can do a final
> review and give my Ack if they look good.
Sure, I can do one hopefully last round of RFC with those selftests updates.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Powered by blists - more mailing lists