[PATCH 0/5 v2] seccomp: add the synchronous mode for seccomp_unotify

[PATCH 0/5 v2] seccomp: add the synchronous mode for seccomp_unotify

[Andrei Vagin]

seccomp_unotify allows more privileged processes does actions on behalf
of less privileged processes.

In many cases, the workflow is fully synchronous. It means a target
process triggers a system call and passes controls to a supervisor
process that handles the system call and returns controls back to the
target process. In this context, "synchronous" means that only one
process is running and another one is waiting.

The new WF_CURRENT_CPU flag advises the scheduler to move the wakee to
the current CPU. For such synchronous workflows, it makes context
switches a few times faster.

Right now, each interaction takes 12µs. With this patch, it takes about
3µs.

v2: clean up the first patch and add the test.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Peter Oskolkov <posk@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho@tycho.pizza>
Cc: Will Drewry <wad@chromium.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>

Andrei Vagin (4):
  seccomp: don't use semaphore and wait_queue together
  sched: add a few helpers to wake up tasks on the current cpu
  seccomp: add the synchronous mode for seccomp_unotify
  selftest/seccomp: add a new test for the sync mode of
    seccomp_user_notify

Peter Oskolkov (1):
  sched: add WF_CURRENT_CPU and externise ttwu

 include/linux/completion.h                    |  1 +
 include/linux/swait.h                         |  1 +
 include/linux/wait.h                          |  3 +
 include/uapi/linux/seccomp.h                  |  4 +
 kernel/sched/completion.c                     | 12 +++
 kernel/sched/core.c                           |  5 +-
 kernel/sched/fair.c                           |  4 +
 kernel/sched/sched.h                          | 13 +--
 kernel/sched/swait.c                          | 11 +++
 kernel/sched/wait.c                           |  5 ++
 kernel/seccomp.c                              | 72 +++++++++++++++--
 tools/testing/selftests/seccomp/seccomp_bpf.c | 80 +++++++++++++++++++
 12 files changed, 196 insertions(+), 15 deletions(-)

-- 
2.37.2

[PATCH 1/5] seccomp: don't use semaphore and wait_queue together

[Andrei Vagin]

Here is no reason to use two different primitives that do similar things.

Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 kernel/seccomp.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index e9852d1b4a5e..876022e9c88c 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -145,7 +145,7 @@ struct seccomp_kaddfd {
  * @notifications: A list of struct seccomp_knotif elements.
  */
 struct notification {
-	struct semaphore request;
+	atomic_t requests;
 	u64 next_id;
 	struct list_head notifications;
 };
@@ -1116,7 +1116,7 @@ static int seccomp_do_user_notification(int this_syscall,
 	list_add_tail(&n.list, &match->notif->notifications);
 	INIT_LIST_HEAD(&n.addfd);
 
-	up(&match->notif->request);
+	atomic_add(1, &match->notif->requests);
 	wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
 
 	/*
@@ -1450,6 +1450,37 @@ find_notification(struct seccomp_filter *filter, u64 id)
 	return NULL;
 }
 
+static int recv_wake_function(wait_queue_entry_t *wait, unsigned int mode, int sync,
+				  void *key)
+{
+	/* Avoid a wakeup if event not interesting for us. */
+	if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR)))
+		return 0;
+	return autoremove_wake_function(wait, mode, sync, key);
+}
+
+static int recv_wait_event(struct seccomp_filter *filter)
+{
+	DEFINE_WAIT_FUNC(wait, recv_wake_function);
+	int ret;
+
+	if (atomic_add_unless(&filter->notif->requests, -1, 0) != 0)
+		return 0;
+
+	for (;;) {
+		ret = prepare_to_wait_event(&filter->wqh, &wait, TASK_INTERRUPTIBLE);
+
+		if (atomic_add_unless(&filter->notif->requests, -1, 0) != 0)
+			break;
+
+		if (ret)
+			return ret;
+
+		schedule();
+	}
+	finish_wait(&filter->wqh, &wait);
+	return 0;
+}
 
 static long seccomp_notify_recv(struct seccomp_filter *filter,
 				void __user *buf)
@@ -1467,7 +1498,7 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 
 	memset(&unotif, 0, sizeof(unotif));
 
-	ret = down_interruptible(&filter->notif->request);
+	ret = recv_wait_event(filter);
 	if (ret < 0)
 		return ret;
 
@@ -1515,7 +1546,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 			if (should_sleep_killable(filter, knotif))
 				complete(&knotif->ready);
 			knotif->state = SECCOMP_NOTIFY_INIT;
-			up(&filter->notif->request);
+			atomic_add(1, &filter->notif->requests);
+			wake_up_poll(&filter->wqh, EPOLLIN | EPOLLRDNORM);
 		}
 		mutex_unlock(&filter->notify_lock);
 	}
@@ -1777,7 +1809,6 @@ static struct file *init_listener(struct seccomp_filter *filter)
 	if (!filter->notif)
 		goto out;
 
-	sema_init(&filter->notif->request, 0);
 	filter->notif->next_id = get_random_u64();
 	INIT_LIST_HEAD(&filter->notif->notifications);
 
-- 
2.37.2

[PATCH 2/5] sched: add WF_CURRENT_CPU and externise ttwu

[Andrei Vagin]

From: Peter Oskolkov <posk@google.com>

Add WF_CURRENT_CPU wake flag that advices the scheduler to
move the wakee to the current CPU. This is useful for fast on-CPU
context switching use cases such as UMCG.

In addition, make ttwu external rather than static so that
the flag could be passed to it from outside of sched/core.c.

Signed-off-by: Peter Oskolkov <posk@google.com>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 kernel/sched/core.c  |  3 +--
 kernel/sched/fair.c  |  4 ++++
 kernel/sched/sched.h | 13 ++++++++-----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5800b0623ff3..cffa8f314c9a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4039,8 +4039,7 @@ bool ttwu_state_match(struct task_struct *p, unsigned int state, int *success)
  * Return: %true if @p->state changes (an actual wakeup was done),
  *	   %false otherwise.
  */
-static int
-try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
+int try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 {
 	unsigned long flags;
 	int cpu, success = 0;
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e4a0b8bd941c..4ebe7222664c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7204,6 +7204,10 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
 	if (wake_flags & WF_TTWU) {
 		record_wakee(p);
 
+		if ((wake_flags & WF_CURRENT_CPU) &&
+		    cpumask_test_cpu(cpu, p->cpus_ptr))
+			return cpu;
+
 		if (sched_energy_enabled()) {
 			new_cpu = find_energy_efficient_cpu(p, prev_cpu);
 			if (new_cpu >= 0)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1644242ecd11..ee24141c4942 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2071,12 +2071,13 @@ static inline int task_on_rq_migrating(struct task_struct *p)
 }
 
 /* Wake flags. The first three directly map to some SD flag value */
-#define WF_EXEC     0x02 /* Wakeup after exec; maps to SD_BALANCE_EXEC */
-#define WF_FORK     0x04 /* Wakeup after fork; maps to SD_BALANCE_FORK */
-#define WF_TTWU     0x08 /* Wakeup;            maps to SD_BALANCE_WAKE */
+#define WF_EXEC         0x02 /* Wakeup after exec; maps to SD_BALANCE_EXEC */
+#define WF_FORK         0x04 /* Wakeup after fork; maps to SD_BALANCE_FORK */
+#define WF_TTWU         0x08 /* Wakeup;            maps to SD_BALANCE_WAKE */
 
-#define WF_SYNC     0x10 /* Waker goes to sleep after wakeup */
-#define WF_MIGRATED 0x20 /* Internal use, task got migrated */
+#define WF_SYNC         0x10 /* Waker goes to sleep after wakeup */
+#define WF_MIGRATED     0x20 /* Internal use, task got migrated */
+#define WF_CURRENT_CPU  0x40 /* Prefer to move the wakee to the current CPU. */
 
 #ifdef CONFIG_SMP
 static_assert(WF_EXEC == SD_BALANCE_EXEC);
@@ -3161,6 +3162,8 @@ static inline bool is_per_cpu_kthread(struct task_struct *p)
 extern void swake_up_all_locked(struct swait_queue_head *q);
 extern void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait);
 
+extern int try_to_wake_up(struct task_struct *tsk, unsigned int state, int wake_flags);
+
 #ifdef CONFIG_PREEMPT_DYNAMIC
 extern int preempt_dynamic_mode;
 extern int sched_dynamic_mode(const char *str);
-- 
2.37.2

[PATCH 3/5] sched: add a few helpers to wake up tasks on the current cpu

[Andrei Vagin]

Add complete_on_current_cpu, wake_up_poll_on_current_cpu helpers to wake
up processes on the current CPU.

Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 include/linux/completion.h |  1 +
 include/linux/swait.h      |  1 +
 include/linux/wait.h       |  3 +++
 kernel/sched/completion.c  | 12 ++++++++++++
 kernel/sched/core.c        |  2 +-
 kernel/sched/swait.c       | 11 +++++++++++
 kernel/sched/wait.c        |  5 +++++
 7 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 62b32b19e0a8..fb2915676574 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -116,6 +116,7 @@ extern bool try_wait_for_completion(struct completion *x);
 extern bool completion_done(struct completion *x);
 
 extern void complete(struct completion *);
+extern void complete_on_current_cpu(struct completion *x);
 extern void complete_all(struct completion *);
 
 #endif
diff --git a/include/linux/swait.h b/include/linux/swait.h
index 6a8c22b8c2a5..1f27b254adf5 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -147,6 +147,7 @@ static inline bool swq_has_sleeper(struct swait_queue_head *wq)
 extern void swake_up_one(struct swait_queue_head *q);
 extern void swake_up_all(struct swait_queue_head *q);
 extern void swake_up_locked(struct swait_queue_head *q);
+extern void swake_up_locked_on_current_cpu(struct swait_queue_head *q);
 
 extern void prepare_to_swait_exclusive(struct swait_queue_head *q, struct swait_queue *wait, int state);
 extern long prepare_to_swait_event(struct swait_queue_head *q, struct swait_queue *wait, int state);
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 7f5a51aae0a7..c7d3e78a500d 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -210,6 +210,7 @@ __remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq
 }
 
 void __wake_up(struct wait_queue_head *wq_head, unsigned int mode, int nr, void *key);
+void __wake_up_on_current_cpu(struct wait_queue_head *wq_head, unsigned int mode, void *key);
 void __wake_up_locked_key(struct wait_queue_head *wq_head, unsigned int mode, void *key);
 void __wake_up_locked_key_bookmark(struct wait_queue_head *wq_head,
 		unsigned int mode, void *key, wait_queue_entry_t *bookmark);
@@ -237,6 +238,8 @@ void __wake_up_pollfree(struct wait_queue_head *wq_head);
 #define key_to_poll(m) ((__force __poll_t)(uintptr_t)(void *)(m))
 #define wake_up_poll(x, m)							\
 	__wake_up(x, TASK_NORMAL, 1, poll_to_key(m))
+#define wake_up_poll_on_current_cpu(x, m)					\
+	__wake_up_on_current_cpu(x, TASK_NORMAL, poll_to_key(m))
 #define wake_up_locked_poll(x, m)						\
 	__wake_up_locked_key((x), TASK_NORMAL, poll_to_key(m))
 #define wake_up_interruptible_poll(x, m)					\
diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index d57a5c1c1cd9..a1931a79c05a 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -38,6 +38,18 @@ void complete(struct completion *x)
 }
 EXPORT_SYMBOL(complete);
 
+void complete_on_current_cpu(struct completion *x)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&x->wait.lock, flags);
+
+	if (x->done != UINT_MAX)
+		x->done++;
+	swake_up_locked_on_current_cpu(&x->wait);
+	raw_spin_unlock_irqrestore(&x->wait.lock, flags);
+}
+
 /**
  * complete_all: - signals all threads waiting on this completion
  * @x:  holds the state of this particular completion
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cffa8f314c9a..1412470216c3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6822,7 +6822,7 @@ asmlinkage __visible void __sched preempt_schedule_irq(void)
 int default_wake_function(wait_queue_entry_t *curr, unsigned mode, int wake_flags,
 			  void *key)
 {
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_SCHED_DEBUG) && wake_flags & ~WF_SYNC);
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_SCHED_DEBUG) && wake_flags & ~(WF_SYNC|WF_CURRENT_CPU));
 	return try_to_wake_up(curr->private, mode, wake_flags);
 }
 EXPORT_SYMBOL(default_wake_function);
diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
index 76b9b796e695..9ebe23868942 100644
--- a/kernel/sched/swait.c
+++ b/kernel/sched/swait.c
@@ -31,6 +31,17 @@ void swake_up_locked(struct swait_queue_head *q)
 }
 EXPORT_SYMBOL(swake_up_locked);
 
+void swake_up_locked_on_current_cpu(struct swait_queue_head *q)
+{
+	struct swait_queue *curr;
+
+	if (list_empty(&q->task_list))
+		return;
+
+	curr = list_first_entry(&q->task_list, typeof(*curr), task_list);
+	try_to_wake_up(curr->task, TASK_NORMAL, WF_CURRENT_CPU);
+	list_del_init(&curr->task_list);
+}
 /*
  * Wake up all waiters. This is an interface which is solely exposed for
  * completions and not for general usage.
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 9860bb9a847c..9a78bca79419 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -157,6 +157,11 @@ void __wake_up(struct wait_queue_head *wq_head, unsigned int mode,
 }
 EXPORT_SYMBOL(__wake_up);
 
+void __wake_up_on_current_cpu(struct wait_queue_head *wq_head, unsigned int mode, void *key)
+{
+	__wake_up_common_lock(wq_head, mode, 1, WF_CURRENT_CPU, key);
+}
+
 /*
  * Same as __wake_up but called with the spinlock in wait_queue_head_t held.
  */
-- 
2.37.2

[PATCH 4/5] seccomp: add the synchronous mode for seccomp_unotify

[Andrei Vagin]

seccomp_unotify allows more privileged processes does actions on behalf
of less privileged processes.

In many cases, the workflow is fully synchronous. It means a target
process triggers a system call and passes controls to a supervisor
process that handles the system call and returns controls to the target
process. In this context, "synchronous" means that only one process is
running and another one is waiting.

There is the WF_CURRENT_CPU flag that is used to advise the scheduler to
move the wakee to the current CPU. For such synchronous workflows, it
makes context switches a few times faster.

Right now, each interaction takes 12µs. With this patch, it takes about
3µs.

This change introduce the SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP flag that
it used to enable the sync mode.

Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 include/uapi/linux/seccomp.h |  4 ++++
 kernel/seccomp.c             | 31 +++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 0fdc6ef02b94..dbfc9b37fcae 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -115,6 +115,8 @@ struct seccomp_notif_resp {
 	__u32 flags;
 };
 
+#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
+
 /* valid flags for seccomp_notif_addfd */
 #define SECCOMP_ADDFD_FLAG_SETFD	(1UL << 0) /* Specify remote fd */
 #define SECCOMP_ADDFD_FLAG_SEND		(1UL << 1) /* Addfd and return it, atomically */
@@ -150,4 +152,6 @@ struct seccomp_notif_addfd {
 #define SECCOMP_IOCTL_NOTIF_ADDFD	SECCOMP_IOW(3, \
 						struct seccomp_notif_addfd)
 
+#define SECCOMP_IOCTL_NOTIF_SET_FLAGS	SECCOMP_IOW(4, __u64)
+
 #endif /* _UAPI_LINUX_SECCOMP_H */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 876022e9c88c..0a62d44f4898 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -143,9 +143,12 @@ struct seccomp_kaddfd {
  *           filter->notify_lock.
  * @next_id: The id of the next request.
  * @notifications: A list of struct seccomp_knotif elements.
+ * @flags: A set of SECCOMP_USER_NOTIF_FD_* flags.
  */
+
 struct notification {
 	atomic_t requests;
+	u32 flags;
 	u64 next_id;
 	struct list_head notifications;
 };
@@ -1117,7 +1120,10 @@ static int seccomp_do_user_notification(int this_syscall,
 	INIT_LIST_HEAD(&n.addfd);
 
 	atomic_add(1, &match->notif->requests);
-	wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
+	if (match->notif->flags & SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
+		wake_up_poll_on_current_cpu(&match->wqh, EPOLLIN | EPOLLRDNORM);
+	else
+		wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
 
 	/*
 	 * This is where we wait for a reply from userspace.
@@ -1593,7 +1599,10 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
 	knotif->error = resp.error;
 	knotif->val = resp.val;
 	knotif->flags = resp.flags;
-	complete(&knotif->ready);
+	if (filter->notif->flags & SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
+		complete_on_current_cpu(&knotif->ready);
+	else
+		complete(&knotif->ready);
 out:
 	mutex_unlock(&filter->notify_lock);
 	return ret;
@@ -1623,6 +1632,22 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
 	return ret;
 }
 
+static long seccomp_notify_set_flags(struct seccomp_filter *filter,
+				    unsigned long flags)
+{
+	long ret;
+
+	if (flags & ~SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&filter->notify_lock);
+	if (ret < 0)
+		return ret;
+	filter->notif->flags = flags;
+	mutex_unlock(&filter->notify_lock);
+	return 0;
+}
+
 static long seccomp_notify_addfd(struct seccomp_filter *filter,
 				 struct seccomp_notif_addfd __user *uaddfd,
 				 unsigned int size)
@@ -1752,6 +1777,8 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
 	case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
 	case SECCOMP_IOCTL_NOTIF_ID_VALID:
 		return seccomp_notify_id_valid(filter, buf);
+	case SECCOMP_IOCTL_NOTIF_SET_FLAGS:
+		return seccomp_notify_set_flags(filter, arg);
 	}
 
 	/* Extensible Argument ioctls */
-- 
2.37.2

[PATCH 5/5] selftest/seccomp: add a new test for the sync mode of seccomp_user_notify

[Andrei Vagin]

Test output:
RUN           global.user_notification_sync ...
seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
OK  global.user_notification_sync

Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 4ae6c8991307..01f872415c17 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4241,6 +4241,86 @@ TEST(user_notification_addfd_rlimit)
 	close(memfd);
 }
 
+/* USER_NOTIF_BENCH_TIMEOUT is 100 miliseconds. */
+#define USER_NOTIF_BENCH_TIMEOUT  100000000ULL
+#define NSECS_PER_SEC            1000000000ULL
+
+#ifndef SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP
+#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
+#define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
+#endif
+
+static void user_notification_sync_loop(struct __test_metadata *_metadata,
+					char *test_name, int listener)
+{
+	struct timespec ts;
+	uint64_t start, end, nr;
+	struct seccomp_notif req = {};
+	struct seccomp_notif_resp resp = {};
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	start = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
+	for (end = start, nr = 0; end - start < USER_NOTIF_BENCH_TIMEOUT; nr++) {
+		memset(&req, 0, sizeof(req));
+		req.pid = 0;
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+
+		EXPECT_EQ(req.data.nr,  __NR_getppid);
+
+		resp.id = req.id;
+		resp.error = 0;
+		resp.val = USER_NOTIF_MAGIC;
+		resp.flags = 0;
+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+		clock_gettime(CLOCK_MONOTONIC, &ts);
+		end = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
+	}
+	TH_LOG("%s:\t%lld nsec/syscall", test_name, USER_NOTIF_BENCH_TIMEOUT / nr);
+}
+
+TEST(user_notification_sync)
+{
+	pid_t pid;
+	long ret;
+	int status, listener;
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
+	listener = user_notif_syscall(__NR_getppid,
+				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
+	ASSERT_GE(listener, 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		while (1) {
+			ret = syscall(__NR_getppid);
+			if (ret == USER_NOTIF_MAGIC)
+				continue;
+			break;
+		}
+		_exit(1);
+	}
+
+	user_notification_sync_loop(_metadata, "basic", listener);
+
+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SET_FLAGS,
+			SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP, 0), 0);
+
+	user_notification_sync_loop(_metadata, "sync", listener);
+
+	kill(pid, SIGKILL);
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFSIGNALED(status));
+	EXPECT_EQ(SIGKILL, WTERMSIG(status));
+}
+
+
 /* Make sure PTRACE_O_SUSPEND_SECCOMP requires CAP_SYS_ADMIN. */
 FIXTURE(O_SUSPEND_SECCOMP) {
 	pid_t pid;
-- 
2.37.2

Re: [PATCH 2/5] sched: add WF_CURRENT_CPU and externise ttwu

[Kees Cook]

On October 19, 2022 6:10:45 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
>From: Peter Oskolkov <posk@google.com>
>
>Add WF_CURRENT_CPU wake flag that advices the scheduler to
>move the wakee to the current CPU. This is useful for fast on-CPU
>context switching use cases such as UMCG.

UMCG is https://lwn.net/Articles/879398/ ?

>In addition, make ttwu external rather than static so that
>the flag could be passed to it from outside of sched/core.c.
>
>Signed-off-by: Peter Oskolkov <posk@google.com>
>Signed-off-by: Andrei Vagin <avagin@gmail.com>
>---
> kernel/sched/core.c  |  3 +--
> kernel/sched/fair.c  |  4 ++++
> kernel/sched/sched.h | 13 ++++++++-----
> 3 files changed, 13 insertions(+), 7 deletions(-)

This would need an Ack from the sched maintainers...


-- 
Kees Cook

Re: [PATCH 5/5] selftest/seccomp: add a new test for the sync mode of seccomp_user_notify

[Kees Cook]

On October 19, 2022 6:10:48 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
>Test output:
>RUN           global.user_notification_sync ...
>seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
>seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
>OK  global.user_notification_sync

This looks like a benchmark, not a functionality test. But maybe the test is "is sync faster than async?"

>
>Signed-off-by: Andrei Vagin <avagin@gmail.com>
>---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
>diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
>index 4ae6c8991307..01f872415c17 100644
>--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
>+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
>@@ -4241,6 +4241,86 @@ TEST(user_notification_addfd_rlimit)
> 	close(memfd);
> }
> 
>+/* USER_NOTIF_BENCH_TIMEOUT is 100 miliseconds. */
>+#define USER_NOTIF_BENCH_TIMEOUT  100000000ULL
>+#define NSECS_PER_SEC            1000000000ULL
>+
>+#ifndef SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP
>+#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
>+#define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
>+#endif
>+
>+static void user_notification_sync_loop(struct __test_metadata *_metadata,
>+					char *test_name, int listener)
>+{
>+	struct timespec ts;
>+	uint64_t start, end, nr;
>+	struct seccomp_notif req = {};
>+	struct seccomp_notif_resp resp = {};
>+
>+	clock_gettime(CLOCK_MONOTONIC, &ts);
>+	start = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
>+	for (end = start, nr = 0; end - start < USER_NOTIF_BENCH_TIMEOUT; nr++) {
>+		memset(&req, 0, sizeof(req));
>+		req.pid = 0;
>+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
>+
>+		EXPECT_EQ(req.data.nr,  __NR_getppid);
>+
>+		resp.id = req.id;
>+		resp.error = 0;
>+		resp.val = USER_NOTIF_MAGIC;
>+		resp.flags = 0;
>+		EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);

I think these EXPECTs should be ASSERTs...

>+
>+		clock_gettime(CLOCK_MONOTONIC, &ts);
>+		end = ts.tv_nsec + ts.tv_sec * NSECS_PER_SEC;
>+	}
>+	TH_LOG("%s:\t%lld nsec/syscall", test_name, USER_NOTIF_BENCH_TIMEOUT / nr);
>+}
>+
>+TEST(user_notification_sync)
>+{
>+	pid_t pid;
>+	long ret;
>+	int status, listener;
>+
>+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>+	ASSERT_EQ(0, ret) {
>+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
>+	}
>+
>+	listener = user_notif_syscall(__NR_getppid,
>+				      SECCOMP_FILTER_FLAG_NEW_LISTENER);
>+	ASSERT_GE(listener, 0);
>+
>+	pid = fork();
>+	ASSERT_GE(pid, 0);
>+
>+	if (pid == 0) {
>+		while (1) {
>+			ret = syscall(__NR_getppid);
>+			if (ret == USER_NOTIF_MAGIC)
>+				continue;
>+			break;
>+		}
>+		_exit(1);
>+	}
>+
>+	user_notification_sync_loop(_metadata, "basic", listener);
>+
>+	EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SET_FLAGS,
>+			SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP, 0), 0);

Same here.

Also can you test that invalid SET_FLAGS are correctly rejected here?

>+
>+	user_notification_sync_loop(_metadata, "sync", listener);
>+

If the timings are collected, add a test that sync is <= async here?

>+	kill(pid, SIGKILL);
>+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
>+	EXPECT_EQ(true, WIFSIGNALED(status));
>+	EXPECT_EQ(SIGKILL, WTERMSIG(status));
>+}
>+
>+
> /* Make sure PTRACE_O_SUSPEND_SECCOMP requires CAP_SYS_ADMIN. */
> FIXTURE(O_SUSPEND_SECCOMP) {
> 	pid_t pid;

Otherwise, yeah, looks good.


-- 
Kees Cook

Re: [PATCH 1/5] seccomp: don't use semaphore and wait_queue together

[Kees Cook]

On October 19, 2022 6:10:44 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
>Here is no reason to use two different primitives that do similar things.
>
>Signed-off-by: Andrei Vagin <avagin@gmail.com>
>---
> kernel/seccomp.c | 41 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 36 insertions(+), 5 deletions(-)

The commit log does not justify adding 29 lines to "do similar things". :) Can you describe the rationale and benefits here a bit more? I assume this to use the the future new wake_up helper?


-- 
Kees Cook

Re: [PATCH 3/5] sched: add a few helpers to wake up tasks on the current cpu

[Peter Zijlstra]

On Wed, Oct 19, 2022 at 06:10:46PM -0700, Andrei Vagin wrote:
> Add complete_on_current_cpu, wake_up_poll_on_current_cpu helpers to wake
> up processes on the current CPU.

There is an astounding lack of *why* in this changelog.

Re: [PATCH 3/5] sched: add a few helpers to wake up tasks on the current cpu

[Andrei Vagin]

On Thu, Oct 20, 2022 at 04:15:04PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 19, 2022 at 06:10:46PM -0700, Andrei Vagin wrote:
> > Add complete_on_current_cpu, wake_up_poll_on_current_cpu helpers to wake
> > up processes on the current CPU.
>
> There is an astounding lack of *why* in this changelog.

I use them in the next patch to handle seccomp user notify requests
faster.

The seccomp notify mechanism allows less privileged processes to offload
specific syscalls to more privileged processes.  In many cases, the
workflow is fully synchronous. It means a target process triggers a
system call, the kernel stops it and wakes up a supervisor process that
handles the system call and returns controls back to the target process.
In this context, "synchronous" means that only one process is running
and another one is waiting.

New helpers advices the scheduler to move the wakee to the current CPU.
For synchronous workflows like described above, these helpers makes
context switches a few times faster.

For example, using these helpers allows to reduce a seccomp user notify
rountdrip time from 12µs to 3µs.

Thanks,
Andrei

Re: [PATCH 2/5] sched: add WF_CURRENT_CPU and externise ttwu

[Andrei Vagin]

On Wed, Oct 19, 2022 at 09:54:15PM -0700, Kees Cook wrote:
> On October 19, 2022 6:10:45 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
> >From: Peter Oskolkov <posk@google.com>
> >
> >Add WF_CURRENT_CPU wake flag that advices the scheduler to
> >move the wakee to the current CPU. This is useful for fast on-CPU
> >context switching use cases such as UMCG.
> 
> UMCG is https://lwn.net/Articles/879398/ ?
> 

Yes, this is it. https://lkml.org/lkml/2021/11/4/830 is the most recent
version that I've seen.

> >In addition, make ttwu external rather than static so that
> >the flag could be passed to it from outside of sched/core.c.
> >
> >Signed-off-by: Peter Oskolkov <posk@google.com>
> >Signed-off-by: Andrei Vagin <avagin@gmail.com>
> >---
> > kernel/sched/core.c  |  3 +--
> > kernel/sched/fair.c  |  4 ++++
> > kernel/sched/sched.h | 13 ++++++++-----
> > 3 files changed, 13 insertions(+), 7 deletions(-)
> 
> This would need an Ack from the sched maintainers...

Re: [PATCH 5/5] selftest/seccomp: add a new test for the sync mode of seccomp_user_notify

[Andrei Vagin]

On Wed, Oct 19, 2022 at 10:04:44PM -0700, Kees Cook wrote:
> On October 19, 2022 6:10:48 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
> >Test output:
> >RUN           global.user_notification_sync ...
> >seccomp_bpf.c:4279:user_notification_sync:basic: 8655 nsec/syscall
> >seccomp_bpf.c:4279:user_notification_sync:sync:	 2919 nsec/syscall
> >OK  global.user_notification_sync
> 
> This looks like a benchmark, not a functionality test. But maybe the test is "is sync faster than async?"
> 

Yes, it is. I found it quite useful for debugging and understanding that
everything works as expected. I like the idea to check that sync is
faster than async. I will add it and address all your other comments in
the next version. Thanks.

Re: [PATCH 1/5] seccomp: don't use semaphore and wait_queue together

[Andrei Vagin]

On Wed, Oct 19, 2022 at 10:10:44PM -0700, Kees Cook wrote:
> On October 19, 2022 6:10:44 PM PDT, Andrei Vagin <avagin@gmail.com> wrote:
> >Here is no reason to use two different primitives that do similar things.
> >
> >Signed-off-by: Andrei Vagin <avagin@gmail.com>
> >---
> > kernel/seccomp.c | 41 ++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 36 insertions(+), 5 deletions(-)
> 
> The commit log does not justify adding 29 lines to "do similar
> things". :) Can you describe the rationale and benefits here a bit
> more? I assume this to use the the future new wake_up helper?

The main reason is to use new wake_up helpers, but there are a few
other reasons:

* if we use two different ways, we always need to call them both. This
  patch fixes seccomp_notify_recv where we forgot to call wake_up_poll
  in the error path.

* If we use one primitive, we can control how many waiters are woken up
  for each request. Our goal is to wake up just one that will handle a
  request. Right now, wake_up_poll may wake up one waiter and
  up(&match->notif->request) may wake up one more.

I will update the commit message. Thanks!

> 
> 
> -- 
> Kees Cook

Re: [PATCH 3/5] sched: add a few helpers to wake up tasks on the current cpu

[Andrei Vagin]

On Thu, Oct 20, 2022 at 5:44 PM Andrei Vagin <avagin@gmail.com> wrote:
>
> On Thu, Oct 20, 2022 at 04:15:04PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 19, 2022 at 06:10:46PM -0700, Andrei Vagin wrote:
> > > Add complete_on_current_cpu, wake_up_poll_on_current_cpu helpers to wake
> > > up processes on the current CPU.
> >
> > There is an astounding lack of *why* in this changelog.
>
> I use them in the next patch to handle seccomp user notify requests
> faster.
>
> The seccomp notify mechanism allows less privileged processes to offload
> specific syscalls to more privileged processes.  In many cases, the
> workflow is fully synchronous. It means a target process triggers a
> system call, the kernel stops it and wakes up a supervisor process that
> handles the system call and returns controls back to the target process.
> In this context, "synchronous" means that only one process is running
> and another one is waiting.
>
> New helpers advices the scheduler to move the wakee to the current CPU.
> For synchronous workflows like described above, these helpers makes
> context switches a few times faster.

Peter,

I've found that I don't understand why WF_SYNC doesn't work in this
case. The test from the last patch shows performance improvements in the
case of WF_CURRENT_CPU, but WF_SYNC doesn't make any difference. I
looked at the code and found that select_task_rq_fair calls
select_idle_sibling, but it doesn't take into account the sync flag.

Does it make sense to do something like this:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4ebe7222664c..c29f758ccfe3 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7249,7 +7249,8 @@ select_task_rq_fair(struct task_struct *p, int
prev_cpu, int wake_flags)
                new_cpu = find_idlest_cpu(sd, p, cpu, prev_cpu, sd_flag);
        } else if (wake_flags & WF_TTWU) { /* XXX always ? */
                /* Fast path */
-               new_cpu = select_idle_sibling(p, prev_cpu, new_cpu);
+               if (!(sync && cpu == new_cpu && this_rq()->nr_running == 1))
+                       new_cpu = select_idle_sibling(p, prev_cpu, new_cpu);
        }
        rcu_read_unlock();


With this patch, the test shows the same numbers for WF_CURRENT_CPU and WF_SYNC.


Thanks,
Andrei