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 <[email protected]>
---
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
> On Feb 24, 2021, at 12:03 AM, wanghongzhe <[email protected]> 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 <[email protected]>
> ---
> 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
>
> > On Feb 24, 2021, at 12:03 AM, wanghongzhe <[email protected]>
> 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 <[email protected]>
> > ---
> > 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);