seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

The change is based on the 'commit 99cdb8b9a573 ("seccomp: notify about unused filter")' which implemented (E)POLLHUP notifications.

Hi Andrei,

the patch looks good to me even if I don't really understand what SECCOMP_IOCTL_NOTIF_RECV does. But let me ask a stupid question,

Quoted 3 lines · Andrei Vagin

The change is based on the 'commit 99cdb8b9a573 ("seccomp: notify about unused filter")' which implemented (E)POLLHUP notifications.

To me this patch fixes the commit above, because without this change

kernel/seccomp.c static int recv_wake_function(wait_queue_entry_t *wait, unsigned int mode, int s · line 1469

Quoted 7 lines

kernel/seccomp.c +1 -1

@@ -1466,7 +1466,7 @@ static int recv_wake_function(wait_queue_entry_t *wait, unsigned int mode, int s
                                void *key)
 {
      /* Avoid a wakeup if event not interesting for us. */
-     if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR)))
+     if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR | EPOLLHUP)))

__seccomp_filter_orphan() -> wake_up_poll(&orig->wqh, EPOLLHUP) won't wakeup the task sleeping in recv_wait_event(), right ?

In any case, FWIW

Reviewed-by Oleg Nesterov <[email protected]>

Previously, seccomp filters were released in release_task(), which required the process to exit and its zombie to be collected. However, exited threads/processes can't trigger any seccomp events, making it more logical to release filters upon task exits.

This adjustment simplifies scenarios where a parent is tracing its child process. The parent process can now handle all events from a seccomp listening descriptor and then call wait to collect a child zombie.

seccomp_filter_release takes the siglock to avoid races with seccomp_sync_threads. There was an idea to bypass taking the lock by checking PF_EXITING, but it can be set without holding siglock if threads have SIGNAL_GROUP_EXIT. This means it can happen concurently with seccomp_filter_release.

Signed-off-by Andrei Vagin <[email protected]>

---
 kernel/exit.c    |  3 ++-
 kernel/seccomp.c | 22 ++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

Previously, seccomp filters were released in release_task(), which required the process to exit and its zombie to be collected. However, exited threads/processes can't trigger any seccomp events, making it more logical to release filters upon task exits.

This adjustment simplifies scenarios where a parent is tracing its child process. The parent process can now handle all events from a seccomp listening descriptor and then call wait to collect a child zombie.

seccomp_filter_release takes the siglock to avoid races with seccomp_sync_threads. There was an idea to bypass taking the lock by checking PF_EXITING, but it can be set without holding siglock if threads have SIGNAL_GROUP_EXIT. This means it can happen concurently with seccomp_filter_release.

Signed-off-by Andrei Vagin <[email protected]>

---
 kernel/exit.c    |  3 ++-
 kernel/seccomp.c | 22 ++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

kernel/exit.c +2 -1

index 41a12630cbbc..23439c021d8d 100644
@@ -278,7 +278,6 @@ void release_task(struct task_struct *p)
 	}
 
 	write_unlock_irq(&tasklist_lock);
-	seccomp_filter_release(p);
 	proc_flush_pid(thread_pid);
 	put_pid(thread_pid);
 	release_thread(p);

@@ -836,6 +835,8 @@ void __noreturn do_exit(long code)
 	io_uring_files_cancel();
 	exit_signals(tsk);  /* sets PF_EXITING */
 
+	seccomp_filter_release(tsk);
+
 	acct_update_integrals(tsk);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {

kernel/seccomp.c +3 -0

index 35435e8f1035..67305e776dd3 100644
@@ -502,6 +502,9 @@ static inline pid_t seccomp_can_sync_threads(void)
 		/* Skip current, since it is initiating the sync. */
 		if (thread == caller)
 			continue;
+		/* Skip exited threads. */
+		if (thread->flags & PF_EXITING)
+			continue;
 
 		if (thread->seccomp.mode == SECCOMP_MODE_DISABLED ||
 		    (thread->seccomp.mode == SECCOMP_MODE_FILTER &&

kernel/seccomp.c +7 -0

 	__seccomp_filter_release(orig);
 }
 

@@ -602,6 +605,13 @@ static inline void seccomp_sync_threads(unsigned long flags)
 		if (thread == caller)
 			continue;
 
+		/*
+		 * Skip exited threads. seccomp_filter_release could have
+		 * been already called for this task.
+		 */
+		if (thread->flags & PF_EXITING)
+			continue;
+
 		/* Get a task reference for the new leaf node. */
 		get_seccomp_filter(caller);
 

Signature · 1 line

2.45.1.288.g0e0cd299f1-goog

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

Quoted 7 lines · Andrei Vagin

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

It seems that 2/3 also fixes another minor problem.

Suppose that a group leader installs the new filter without SECCOMP_FILTER_FLAG_TSYNC, exits, and becomes a zombie. It can't be released until all its sub-threads exit.

After that, without 2/3, SECCOMP_FILTER_FLAG_TSYNC from any other thread can never succeed, seccomp_can_sync_threads() will check a zombie leader and is_ancestor() will fail.

Right?

Oleg.

This patch set addresses two problems with the SECCOMP_IOCTL_NOTIF_RECV ioctl:
* it doesn't return when the seccomp filter becomes unused (all tasks have exited).
* EPOLLHUP is triggered not when a task exits, but rather when its zombie is collected.

v2: - Remove unnecessary checks of PF_EXITING.
- Take siglock with disabling irqs.
Thanks to Oleg for the review and the help with the first version.

Andrei Vagin (3):
seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited
seccomp: release task filters when the task exits selftests/seccomp: add test for NOTIF_RECV and unused filters

 kernel/exit.c                                 |  3 +-
 kernel/seccomp.c                              | 38 ++++++++++---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 54 +++++++++++++++++++
 3 files changed, 88 insertions(+), 7 deletions(-)

Cc Kees Cook <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Will Drewry <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Christian Brauner <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Tycho Andersen <[email protected]>

--
2.45.0.rc1.225.g2a3ae87e7f-goog

Are you waiting for anything from me? I think this series is ready to be merged.