RE: [PATCH v3] seccomp: Improve performace by optimizing rmb()

From: Wanghongzhe (Hongzhe, EulerOS)
Date: Thu Feb 25 2021 - 07:04:43 EST


> > On Feb 24, 2021, at 12:03 AM, wanghongzhe <wanghongzhe@xxxxxxxxxx>
> wrote:
> >
> > As Kees haved accepted the v2 patch at a381b70a1 which just replaced
> > rmb() with smp_rmb(), this patch will base on that and just adjust the
> > smp_rmb() to the correct position.
> >
> > As the original comment shown (and indeed it should be):
> > /*
> > * Make sure that any changes to mode from another thread have
> > * been seen after SYSCALL_WORK_SECCOMP was seen.
> > */
> > the smp_rmb() should be put between reading SYSCALL_WORK_SECCOMP
> and
> > reading seccomp.mode to make sure that any changes to mode from
> > another thread have been seen after SYSCALL_WORK_SECCOMP was seen,
> for
> > TSYNC situation. However, it is misplaced between reading seccomp.mode
> > and seccomp->filter. This issue seems to be misintroduced at
> > 13aa72f0fd0a9f98a41cefb662487269e2f1ad65 which aims to refactor the
> > filter callback and the API. So let's just adjust the
> > smp_rmb() to the correct position.
> >
> > A next optimization patch will be provided if this ajustment is appropriate.
>
> Would it be better to make the syscall work read be smp_load_acquire()?
>
> >
> > v2 -> v3:
> > - move the smp_rmb() to the correct position
> >
> > v1 -> v2:
> > - only replace rmb() with smp_rmb()
> > - provide the performance test number
> >
> > RFC -> v1:
> > - replace rmb() with smp_rmb()
> > - move the smp_rmb() logic to the middle between TIF_SECCOMP and mode
> >
> > Signed-off-by: wanghongzhe <wanghongzhe@xxxxxxxxxx>
> > ---
> > kernel/seccomp.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c index
> > 1d60fc2c9987..64b236cb8a7f 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1160,12 +1160,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.
> > - */
> > - smp_rmb();
> > -
> > if (!sd) {
> > populate_seccomp_data(&sd_local);
> > sd = &sd_local;
> > @@ -1291,7 +1285,6 @@ static int __seccomp_filter(int this_syscall,
> > const struct seccomp_data *sd,
> >
> > int __secure_computing(const struct seccomp_data *sd) {
> > - int mode = current->seccomp.mode;
> > int this_syscall;
> >
> > if (IS_ENABLED(CONFIG_CHECKPOINT_RESTORE) && @@ -1301,7
> +1294,13 @@
> > int __secure_computing(const struct seccomp_data *sd)
> > this_syscall = sd ? sd->nr :
> > syscall_get_nr(current, current_pt_regs());
> >
> > - switch (mode) {
> > + /*
> > + * Make sure that any changes to mode from another thread have
> > + * been seen after SYSCALL_WORK_SECCOMP was seen.
> > + */
> > + smp_rmb();
> > +
> > + switch (current->seccomp.mode) {
> > case SECCOMP_MODE_STRICT:
> > __secure_computing_strict(this_syscall); /* may call do_exit */
> > return 0;
> > --
> > 2.19.1
> >
> Would it be better to make the syscall work read be smp_load_acquire()?
Maybe we can do something like this (untested):
__syscall_enter_from_user_work(struct pt_regs *regs, long syscall)
{
- unsigned long work = READ_ONCE(current_thread_info()->syscall_work);
+ unsigned long work = smp_load_acquire (&(current_thread_info()->syscall_work));

if (work & SYSCALL_WORK_ENTER)
syscall = syscall_trace_enter(regs, syscall, work);
However, this may insert a memory barrier and slow down all works
behind it in SYSCALL_WORK_ENTER, not just seccomp, which is not
we want. And in order to match with the smp_mb__before_atomic() in
seccomp_assign_mode() which called in seccomp_sync_threads(), it is
better to use smp_rmb() between the work and mode read:
task->seccomp.mode = seccomp_mode;
/*
* Make sure SYSCALL_WORK_SECCOMP cannot be set before the mode (and
* filter) is set.
*/
* smp_mb__before_atomic();
/* Assume default seccomp processes want spec flaw mitigation. */
if ((flags & SECCOMP_FILTER_FLAG_SPEC_ALLOW) == 0)
arch_seccomp_spec_mitigate(task);
set_task_syscall_work(task, SECCOMP);