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

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

[wanghongzhe]

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.

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@huawei.com>
---
 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

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

[Wanghongzhe (Hongzhe, EulerOS)]

> > On Feb 24, 2021, at 12:03 AM, wanghongzhe <wanghongzhe@huawei.com>
> 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@huawei.com>
> > ---
> > 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);

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

[Andy Lutomirski]

> On Feb 24, 2021, at 12:03 AM, wanghongzhe <wanghongzhe@huawei.com> 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@huawei.com>
> ---
> 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
> 

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

[wanghongzhe]

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.

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@huawei.com>
---
 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