From: "Wanghongzhe (Hongzhe, EulerOS)" <wanghongzhe@huawei.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: "keescook@chromium.org" <keescook@chromium.org>,
"wad@chromium.org" <wad@chromium.org>,
"ast@kernel.org" <ast@kernel.org>,
"daniel@iogearbox.net" <daniel@iogearbox.net>,
"andrii@kernel.org" <andrii@kernel.org>,
"kafai@fb.com" <kafai@fb.com>,
"songliubraving@fb.com" <songliubraving@fb.com>,
"yhs@fb.com" <yhs@fb.com>,
"john.fastabend@gmail.com" <john.fastabend@gmail.com>,
"kpsingh@kernel.org" <kpsingh@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: RE: [PATCH] seccomp: Improve performance by optimizing memory barrier
Date: Tue, 2 Feb 2021 01:50:18 +0000 [thread overview]
Message-ID: <003c156cf88c4ccd82d50e450c4696ed@huawei.com> (raw)
In-Reply-To: <B1DC6A42-15AF-4804-B20E-FC6E2BDD1C8E@amacapital.net>
>> On Feb 1, 2021, at 4:06 AM, wanghongzhe <wanghongzhe@huawei.com> wrote:
>>
>> If a thread(A)'s TSYNC flag is set from seccomp(), then it will
>> synchronize its seccomp filter to other threads(B) in same thread
>> group. To avoid race condition, seccomp puts rmb() between reading the
>> mode and filter in seccomp check patch(in B thread).
>> As a result, every syscall's seccomp check is slowed down by the
>> memory barrier.
>>
>> However, we can optimize it by calling rmb() only when filter is NULL
>> and reading it again after the barrier, which means the rmb() is
>> called only once in thread lifetime.
>>
>> The 'filter is NULL' conditon means that it is the first time
>> attaching filter and is by other thread(A) using TSYNC flag.
>> In this case, thread B may read the filter first and mode later in CPU
>> out-of-order exection. After this time, the thread B's mode is always
>> be set, and there will no race condition with the filter/bitmap.
>>
>> In addtion, we should puts a write memory barrier between writing the
>> filter and mode in smp_mb__before_atomic(), to avoid the race
>> condition in TSYNC case.
>
> I haven’t fully worked this out, but rmb() is bogus. This should be smp_rmb().
Yes, I think you are right.I will fix it and send another patch.
>>
>> Signed-off-by: wanghongzhe <wanghongzhe@huawei.com>
>> ---
>> kernel/seccomp.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c index
>> 952dc1c90229..b944cb2b6b94 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -397,8 +397,20 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
>> READ_ONCE(current->seccomp.filter);
>>
>> /* Ensure unexpected behavior doesn't result in failing open. */
>> - if (WARN_ON(f == NULL))
>> - return SECCOMP_RET_KILL_PROCESS;
>> + if (WARN_ON(f == NULL)) {
>> + /*
>> + * Make sure the first filter addtion (from another
>> + * thread using TSYNC flag) are seen.
>> + */
>> + rmb();
>> +
>> + /* Read again */
>> + f = READ_ONCE(current->seccomp.filter);
>> +
>> + /* Ensure unexpected behavior doesn't result in failing open. */
>> + if (WARN_ON(f == NULL))
>> + return SECCOMP_RET_KILL_PROCESS;
>> + }
>>
>> if (seccomp_cache_check_allow(f, sd))
>> return SECCOMP_RET_ALLOW;
>> @@ -614,9 +626,16 @@ static inline void seccomp_sync_threads(unsigned long flags)
>> * equivalent (see ptrace_may_access), it is safe to
>> * allow one thread to transition the other.
>> */
>> - if (thread->seccomp.mode == SECCOMP_MODE_DISABLED)
>> + if (thread->seccomp.mode == SECCOMP_MODE_DISABLED) {
>> + /*
>> + * Make sure mode cannot be set before the filter
>> + * are set.
>> + */
>> + smp_mb__before_atomic();
>> +
>> seccomp_assign_mode(thread, SECCOMP_MODE_FILTER,
>> flags);
>> + }
>> }
>> }
>>
>> @@ -1160,12 +1179,6 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>> int data;
>> struct seccomp_data sd_local;
>>
>> - /*
>> - * Make sure that any changes to mode from another thread have
>> - * been seen after SYSCALL_WORK_SECCOMP was seen.
>> - */
>> - rmb();
>> -
>> if (!sd) {
>> populate_seccomp_data(&sd_local);
>> sd = &sd_local;
>> --
>> 2.19.1
>>
next prev parent reply other threads:[~2021-02-02 1:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-01 12:50 [PATCH] seccomp: Improve performance by optimizing memory barrier wanghongzhe
2021-02-01 15:39 ` Andy Lutomirski
2021-02-02 1:50 ` Wanghongzhe (Hongzhe, EulerOS) [this message]
2021-02-02 10:13 ` [PATCH v1 1/1] Firstly, as Andy mentioned, this should be smp_rmb() instead of rmb(). considering that TSYNC is a cross-thread situation, and rmb() is a mandatory barrier which should not be used to control SMP effects, since mandatory barriers impose unnecessary overhead on both SMP and UP systems, as kernel Documentation said wanghongzhe
2021-02-02 11:53 ` Greg KH
2021-02-02 19:02 ` Kees Cook
2021-02-04 8:45 ` [PATCH v1 1/1] Firstly, as Andy mentioned, this should be smp_rmb() instead of rmb(). considering that TSYNC is a cross-thread situation, and rmb() is a mandatory barrier which should not be used to control SMP effects, since mandatory barriers imp Wanghongzhe (Hongzhe, EulerOS)
-- strict thread matches above, loose matches on Subject: below --
2021-02-01 12:49 [PATCH] seccomp: Improve performance by optimizing memory barrier wanghongzhe
2021-02-08 6:43 ` Leon Romanovsky
2021-02-08 7:10 ` Wanghongzhe (Hongzhe, EulerOS)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=003c156cf88c4ccd82d50e450c4696ed@huawei.com \
--to=wanghongzhe@huawei.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=keescook@chromium.org \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=wad@chromium.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).