RE: [PATCH] seccomp: Improve performance by optimizing memory barrier

From: Leon Romanovsky [mailto:leon@kernel.org]
Sent: Monday, February 8, 2021 2:44 PM
To: Wanghongzhe (Hongzhe, EulerOS) <redacted>
Cc: keescook@chromium.org; luto@amacapital.net; wad@chromium.org;
ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; kafai@fb.com;
songliubraving@fb.com; yhs@fb.com; john.fastabend@gmail.com;
kpsingh@kernel.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.org;
bpf@vger.kernel.org
Subject: Re: [PATCH] seccomp: Improve performance by optimizing memory
barrier

On Mon, Feb 01, 2021 at 08:49:41PM +0800, wanghongzhe wrote:

quoted

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.

Signed-off-by: wanghongzhe <redacted>
---
 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,

quoted

 			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;
+	}

IMHO, double WARN_ON() for the fallback flow is too much.
Also according to the description, this "f == NULL" check is due to races and
not programming error which WARN_ON() are intended to catch.

Thanks

Maybe you are right. I think 'if (f == NULL)' is enough for this optimizing.