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

[Wanghongzhe (Hongzhe, EulerOS)]

> > 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);