Re: [PATCH 2/3] seccomp: release task filters when the task exits

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Oleg Nesterov]

(add lkml)

On 05/15, Andrei Vagin wrote:
>
> On Wed, May 15, 2024 at 5:52 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Let me repeat I forgot everything about seccomp, but let me ask
> > a couple of questions...
>
> It seems you still remember something:). Thank you for the feedback.

Just I am still remember how to use grep ;)

> > > @@ -2126,6 +2137,11 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
> > >        */
> > >       spin_lock_irq(&task->sighand->siglock);
> > >
> > > +     if (task->flags & PF_EXITING) {
> > > +             spin_unlock_irq(&task->sighand->siglock);
> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> >
> > Why do we need the PF_EXITING check here?
> >
> > This looks unnecessary even if get_nth_filter() could race with the
> > exiting task, but this doesn't matter.
> >
> > This race is not possible, get_nth_filter() is only called from ptrace()
> > paths, but the tracee can't stop in TASK_TRACED after exit_signals() which
> > sets PF_EXITING.
>
> If we rely on using seccomp_get_filter only from ptrace, you are right.

Plus it too does __get_seccomp_filter/__get_seccomp_filter, so I guess it
should be safe without this check even if it could be used outside of ptrace.
Just like proc_pid_seccomp_cache(), see below.

> > > @@ -2494,6 +2510,11 @@ int proc_pid_seccomp_cache(struct seq_file *m, struct pid_namespace *ns,
> > >       if (!lock_task_sighand(task, &flags))
> > >               return -ESRCH;
> > >
> > > +     if (thread->flags & PF_EXITING) {
> > > +             unlock_task_sighand(task, &flags);
> > > +             return 0;
> >
> > Again, do we really need this check?
> >
> > It can race with the exiting task and (without this check) do
> > __get_seccomp_filter(f) right before seccomp_filter_release()
> > takes sighand->siglock. But why is it bad?
>
> I think you are right, this check isn't required.
>
> >
> > OTOH. I guess proc_pid_seccomp_cache() is the only reason why
> > seccomp_filter_release() takes ->siglock with your patch?
>
> seccomp_sync_threads and seccomp_can_sync_threads should be considered too.

Yes. But we only need to consider them in the multi-thread case, right?
In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
miss this flag, seccomp_filter_release() doesn't need to take siglock.

> If we check PF_EXITING in all of them, we don't need to take ->siglock in
> seccomp_filter_release. Does it sound right?

The problem is a single-threaded exiting task. In this case exit_signals()
sets PF_EXITING lockless. This means that in this case

	- proc_pid_seccomp_cache() can't rely on the PF_EXITING check
	  but it can be safely removed.

	- seccomp_filter_release() needs to take ->siglock to avoid the
	  race with proc_pid_seccomp_cache().

And this chunk from your patch

	 static void __seccomp_filter_orphan(struct seccomp_filter *orig)
	 {
	+       lockdep_assert_held(&current->sighand->siglock);
	+

looks unnecessary too, seccomp_filter_release() can just do

	spin_lock_irq(siglock);
	orig = tsk->seccomp.filter;
	tsk->seccomp.filter = NULL;
	spin_unlock_irq(siglock);

	__seccomp_filter_release(orig);

Right?

Oleg.

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Oleg Nesterov]

On 05/16, Oleg Nesterov wrote:
>
> On 05/15, Andrei Vagin wrote:
> >
> > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
>
> Yes. But we only need to consider them in the multi-thread case, right?
> In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> miss this flag, seccomp_filter_release() doesn't need to take siglock.
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Ah, no. seccomp_filter_release() does need to take ->siglock even if we
forget about proc_pid_seccomp_cache().

Without siglock

	orig = tsk->seccomp.filter;

can leak into the critical section in exit_signals() (spin_unlock is the
one-way barrier) and this LOAD can be reordered with "flags |= PF_EXITING".

Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
don't. So we can't add a fast-path

	if (!tsk->seccomp.filter)
		return;

check at the start of seccomp_filter_release().


Cough... Now that I look at seccomp_can_sync_threads() I think it too
doesn't need the PF_EXITING check.

If it is called before seccomp_filter_release(), this doesn't really
differ from the case when it is called before do_exit/exit_signals.

If it is called after seccomp_filter_release(), then is_ancestor()
must be true.

But perhaps I missed something, I won't insist, up to you.

> > If we check PF_EXITING in all of them, we don't need to take ->siglock in
> > seccomp_filter_release. Does it sound right?
>
> The problem is a single-threaded exiting task. In this case exit_signals()
> sets PF_EXITING lockless. This means that in this case
>
> 	- proc_pid_seccomp_cache() can't rely on the PF_EXITING check
> 	  but it can be safely removed.
>
> 	- seccomp_filter_release() needs to take ->siglock to avoid the
> 	  race with proc_pid_seccomp_cache().
>
> And this chunk from your patch
>
> 	 static void __seccomp_filter_orphan(struct seccomp_filter *orig)
> 	 {
> 	+       lockdep_assert_held(&current->sighand->siglock);
> 	+
>
> looks unnecessary too, seccomp_filter_release() can just do
>
> 	spin_lock_irq(siglock);
> 	orig = tsk->seccomp.filter;
> 	tsk->seccomp.filter = NULL;
> 	spin_unlock_irq(siglock);
>
> 	__seccomp_filter_release(orig);
>
> Right?
>
> Oleg.

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Andrei Vagin]

On Thu, May 16, 2024 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 05/16, Oleg Nesterov wrote:
> >
> > On 05/15, Andrei Vagin wrote:
> > >
> > > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
> >
> > Yes. But we only need to consider them in the multi-thread case, right?



> > In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> > miss this flag, seccomp_filter_release() doesn't need to take siglock.
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


>
> Ah, no. seccomp_filter_release() does need to take ->siglock even if we
> forget about proc_pid_seccomp_cache().
>
> Without siglock
>
>         orig = tsk->seccomp.filter;
>
> can leak into the critical section in exit_signals() (spin_unlock is the
> one-way barrier) and this LOAD can be reordered with "flags |= PF_EXITING".
>
> Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
> don't. So we can't add a fast-path

We have smp_mb__after_unlock_lock in include/linux/rcupdate.h.

>
>         if (!tsk->seccomp.filter)
>                 return;
>
> check at the start of seccomp_filter_release().
>
>
> Cough... Now that I look at seccomp_can_sync_threads() I think it too
> doesn't need the PF_EXITING check.
>
> If it is called before seccomp_filter_release(), this doesn't really
> differ from the case when it is called before do_exit/exit_signals.
>
> If it is called after seccomp_filter_release(), then is_ancestor()
> must be true.
>
> But perhaps I missed something, I won't insist, up to you.
>
> > > If we check PF_EXITING in all of them, we don't need to take ->siglock in
> > > seccomp_filter_release. Does it sound right?
> >
> > The problem is a single-threaded exiting task. In this case exit_signals()
> > sets PF_EXITING lockless. This means that in this case
> >
> >       - proc_pid_seccomp_cache() can't rely on the PF_EXITING check
> >         but it can be safely removed.
> >
> >       - seccomp_filter_release() needs to take ->siglock to avoid the
> >         race with proc_pid_seccomp_cache().
> >
> > And this chunk from your patch
> >
> >        static void __seccomp_filter_orphan(struct seccomp_filter *orig)
> >        {
> >       +       lockdep_assert_held(&current->sighand->siglock);
> >       +
> >
> > looks unnecessary too, seccomp_filter_release() can just do
> >
> >       spin_lock_irq(siglock);
> >       orig = tsk->seccomp.filter;
> >       tsk->seccomp.filter = NULL;
> >       spin_unlock_irq(siglock);
> >
> >       __seccomp_filter_release(orig);
> >
> > Right?
> >
> > Oleg.
>

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Andrei Vagin]

> On Thu, May 16, 2024 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 05/16, Oleg Nesterov wrote:
> > >
> > > On 05/15, Andrei Vagin wrote:
> > > >
> > > > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
> > >
> > > Yes. But we only need to consider them in the multi-thread case, right?
> > > In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> > > miss this flag, seccomp_filter_release() doesn't need to take siglock.
> >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

PF_EXITING is set without holding ->siglock if tsk->signal has the
SIGNAL_GROUP_EXIT flag. I think it can be a case when one thread is in
seccomp_sync_threads and others are exiting. The first thread can check
that PF_EXITING isn't set for another thread. Then, the second thread calls
exit_signals and seccomp_filter_release(), and finally, the first thread
sets its seccomp.filter to the second thread. If seccomp_filter_release takes
siglock, it will be handled properly.

> >
> > Ah, no. seccomp_filter_release() does need to take ->siglock even if we
> > forget about proc_pid_seccomp_cache().
> >
> > Without siglock
> >
> >         orig = tsk->seccomp.filter;
> >
> > can leak into the critical section in exit_signals() (spin_unlock is the
> > one-way barrier) and this LOAD can be reordered with "flags |= PF_EXITING".
> >
> > Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
> > don't. So we can't add a fast-path

We have smp_mb__after_unlock_lock in include/linux/rcupdate.h.

> >
> >         if (!tsk->seccomp.filter)
> >                 return;
> >
> > check at the start of seccomp_filter_release().
> >
> >
> > Cough... Now that I look at seccomp_can_sync_threads() I think it too
> > doesn't need the PF_EXITING check.
> >
> > If it is called before seccomp_filter_release(), this doesn't really
> > differ from the case when it is called before do_exit/exit_signals.
> >
> > If it is called after seccomp_filter_release(), then is_ancestor()
> > must be true.
> >
> > But perhaps I missed something, I won't insist, up to you.
> >

You are right, this check isn't required in seccomp_can_sync_threads, but
I decided that it is better to be consistent with seccomp_sync_threads.

Thanks,
Andrei

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Oleg Nesterov]

On 05/22, Andrei Vagin wrote:
>
> > On Thu, May 16, 2024 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > On 05/16, Oleg Nesterov wrote:
> > > >
> > > > On 05/15, Andrei Vagin wrote:
> > > > >
> > > > > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
> > > >
> > > > Yes. But we only need to consider them in the multi-thread case, right?
> > > > In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> > > > miss this flag, seccomp_filter_release() doesn't need to take siglock.
> > >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> PF_EXITING is set without holding ->siglock if tsk->signal has the
> SIGNAL_GROUP_EXIT flag. I think it can be a case when one thread is in
> seccomp_sync_threads and others are exiting.

Yes, I forgot this.

> > > Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
> > > don't. So we can't add a fast-path
>
> We have smp_mb__after_unlock_lock in include/linux/rcupdate.h.

This is another thing.

But sorry for confusion, this doesn't really matter, we could you a plain mb().
I mean, I was thinking about something like

	seccomp_filter_release:

		smp_mb();
		if (!READ_ONCE(tsk->seccomp.filter))
			return;

		spin_lock_irq(siglock);
		orig = tsk->seccomp.filter;
		...

but then seccomp_sync_threads() should do something like


		orig = READ_ONCE(thread->seccomp.filter);

		smp_store_release(&thread->seccomp.filter,
				  caller->seccomp.filter);

		smp_mb(); // pairs with mb() in seccomp_filter_release()

		if (READ_ONCE(thread->flags) & PF_EXITING) {
			WRITE_ONCE(thread->seccomp.filter, orig);
			continue;
		}
		__seccomp_filter_release(orig);

		...

too subtle even _if_ correct, and I am not sure at all this would be correct.

> > > Cough... Now that I look at seccomp_can_sync_threads() I think it too
> > > doesn't need the PF_EXITING check.
> > >
> > > If it is called before seccomp_filter_release(), this doesn't really
> > > differ from the case when it is called before do_exit/exit_signals.
> > >
> > > If it is called after seccomp_filter_release(), then is_ancestor()
> > > must be true.
> > >
> > > But perhaps I missed something, I won't insist, up to you.
> > >
>
> You are right, this check isn't required in seccomp_can_sync_threads, but
> I decided that it is better to be consistent with seccomp_sync_threads.

OK, agreed.

Oleg.

[PATCH 0/3 v2] seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

[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.

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 <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Tycho Andersen <tandersen@netflix.com>


-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH 1/3] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

[Andrei Vagin]

SECCOMP_IOCTL_NOTIF_RECV promptly returns when a seccomp filter becomes
unused, as a filter without users can't trigger any events.

Previously, event listeners had to rely on epoll to detect when all
processes had exited.

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

Reviewed-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Andrei Vagin <avagin@google.com>
---
 kernel/seccomp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f70e031e06a8..35435e8f1035 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -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)))
 		return 0;
 	return autoremove_wake_function(wait, mode, sync, key);
 }
@@ -1476,6 +1476,9 @@ static int recv_wait_event(struct seccomp_filter *filter)
 	DEFINE_WAIT_FUNC(wait, recv_wake_function);
 	int ret;
 
+	if (refcount_read(&filter->users) == 0)
+		return 0;
+
 	if (atomic_dec_if_positive(&filter->notif->requests) >= 0)
 		return 0;
 
@@ -1484,6 +1487,8 @@ static int recv_wait_event(struct seccomp_filter *filter)
 
 		if (atomic_dec_if_positive(&filter->notif->requests) >= 0)
 			break;
+		if (refcount_read(&filter->users) == 0)
+			break;
 
 		if (ret)
 			return ret;
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH 2/3] seccomp: release task filters when the task exits

[Andrei Vagin]

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 <avagin@google.com>
---
 kernel/exit.c    |  3 ++-
 kernel/seccomp.c | 22 ++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 41a12630cbbc..23439c021d8d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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) {
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 35435e8f1035..67305e776dd3 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -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 &&
@@ -563,18 +566,18 @@ static void __seccomp_filter_release(struct seccomp_filter *orig)
  * @tsk: task the filter should be released from.
  *
  * This function should only be called when the task is exiting as
- * it detaches it from its filter tree. As such, READ_ONCE() and
- * barriers are not needed here, as would normally be needed.
+ * it detaches it from its filter tree. PF_EXITING has to be set
+ * for the task.
  */
 void seccomp_filter_release(struct task_struct *tsk)
 {
-	struct seccomp_filter *orig = tsk->seccomp.filter;
-
-	/* We are effectively holding the siglock by not having any sighand. */
-	WARN_ON(tsk->sighand != NULL);
+	struct seccomp_filter *orig;
 
+	spin_lock_irq(&current->sighand->siglock);
+	orig = tsk->seccomp.filter;
 	/* Detach task from its filter tree. */
 	tsk->seccomp.filter = NULL;
+	spin_unlock_irq(&current->sighand->siglock);
 	__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);
 
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH 3/3] selftests/seccomp: add test for NOTIF_RECV and unused filters

[Andrei Vagin]

Add a new test case to check that SECCOMP_IOCTL_NOTIF_RECV returns when all
tasks have gone.

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

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 783ebce8c4de..390781d7c951 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3954,6 +3954,60 @@ TEST(user_notification_filter_empty)
 	EXPECT_GT((pollfd.revents & POLLHUP) ?: 0, 0);
 }
 
+TEST(user_ioctl_notification_filter_empty)
+{
+	pid_t pid;
+	long ret;
+	int status, p[2];
+	struct __clone_args args = {
+		.flags = CLONE_FILES,
+		.exit_signal = SIGCHLD,
+	};
+	struct seccomp_notif req = {};
+
+	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!");
+	}
+
+	if (__NR_clone3 < 0)
+		SKIP(return, "Test not built with clone3 support");
+
+	ASSERT_EQ(0, pipe(p));
+
+	pid = sys_clone3(&args, sizeof(args));
+	ASSERT_GE(pid, 0);
+
+	if (pid == 0) {
+		int listener;
+
+		listener = user_notif_syscall(__NR_mknodat, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+		if (listener < 0)
+			_exit(EXIT_FAILURE);
+
+		if (dup2(listener, 200) != 200)
+			_exit(EXIT_FAILURE);
+		close(p[1]);
+		close(listener);
+		sleep(1);
+
+		_exit(EXIT_SUCCESS);
+	}
+	if (read(p[0], &status, 1) != 0)
+		_exit(EXIT_SUCCESS);
+	close(p[0]);
+	/*
+	 * The seccomp filter has become unused so we should be notified once
+	 * the kernel gets around to cleaning up task struct.
+	 */
+	EXPECT_EQ(ioctl(200, SECCOMP_IOCTL_NOTIF_RECV, &req), -1);
+	EXPECT_EQ(errno, ENOENT);
+
+	EXPECT_EQ(waitpid(pid, &status, 0), pid);
+	EXPECT_EQ(true, WIFEXITED(status));
+	EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
 static void *do_thread(void *data)
 {
 	return NULL;
-- 
2.45.1.288.g0e0cd299f1-goog

Re: [PATCH 1/3] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

[Oleg Nesterov]

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,

On 05/23, Andrei Vagin wrote:
>
> 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

> @@ -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 <oleg@redhat.com>

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Oleg Nesterov]

On 05/23, Andrei Vagin wrote:
>
> 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 <avagin@google.com>
> ---
>  kernel/exit.c    |  3 ++-
>  kernel/seccomp.c | 22 ++++++++++++++++------
>  2 files changed, 18 insertions(+), 7 deletions(-)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH 0/3 v2] seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

[Oleg Nesterov]

On 05/23, Andrei Vagin wrote:
>
> 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.

Re: [PATCH 1/3] seccomp: interrupt SECCOMP_IOCTL_NOTIF_RECV when all users have exited

[Andrei Vagin]

On Thu, May 23, 2024 at 2:00 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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,
>
> On 05/23, Andrei Vagin wrote:
> >
> > 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

It depends on how we look at it. I think the intention was to address
the epoll/poll/select syscalls to return (E)POLLHUP notifications when
filters have been orphaned. Plus, this code looked a bit different that
time and recv_wake_function used another notification mechanism.

>
> > @@ -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 <oleg@redhat.com>

Thanks,
Andrei

Re: [PATCH 0/3 v2] seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

[Andrei Vagin]

Kees,

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

Thanks,
Andrei

On Wed, May 22, 2024 at 6:45 PM Andrei Vagin <avagin@google.com> wrote:
>
> 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 <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Tycho Andersen <tandersen@netflix.com>
>
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>

Re: [PATCH 0/3 v2] seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

[Andrei Vagin]

On Thu, May 23, 2024 at 2:35 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 05/23, Andrei Vagin wrote:
> >
> > 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?

It is right. I can introduce a self test for this case too, but let's
do that in a separate patch set.

>
> Oleg.
>

Re: [PATCH 2/3] seccomp: release task filters when the task exits

[Kees Cook]

On Thu, May 23, 2024 at 01:45:39AM +0000, Andrei Vagin wrote:
> 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 <avagin@google.com>
> ---
>  kernel/exit.c    |  3 ++-
>  kernel/seccomp.c | 22 ++++++++++++++++------
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 41a12630cbbc..23439c021d8d 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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) {
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 35435e8f1035..67305e776dd3 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -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 &&
> @@ -563,18 +566,18 @@ static void __seccomp_filter_release(struct seccomp_filter *orig)
>   * @tsk: task the filter should be released from.
>   *
>   * This function should only be called when the task is exiting as
> - * it detaches it from its filter tree. As such, READ_ONCE() and
> - * barriers are not needed here, as would normally be needed.
> + * it detaches it from its filter tree. PF_EXITING has to be set
> + * for the task.

Let's capture this requirement with a WARN_ON() (like was done for the
sighand case before). So before the spinlock, check for PF_EXITING and
fail safe (don't release):

	if (WARN_ON((tsk->flags & PF_EXITING) == 0))
		return;

>   */
>  void seccomp_filter_release(struct task_struct *tsk)
>  {
> -	struct seccomp_filter *orig = tsk->seccomp.filter;
> -
> -	/* We are effectively holding the siglock by not having any sighand. */
> -	WARN_ON(tsk->sighand != NULL);
> +	struct seccomp_filter *orig;
>  
> +	spin_lock_irq(&current->sighand->siglock);

Shouldn't this be "tsk" not "current"?

> +	orig = tsk->seccomp.filter;
>  	/* Detach task from its filter tree. */
>  	tsk->seccomp.filter = NULL;
> +	spin_unlock_irq(&current->sighand->siglock);

Same.

>  	__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);
>  
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 

Otherwise, looks good!

-- 
Kees Cook

Re: [PATCH 0/3 v2] seccomp: improve handling of SECCOMP_IOCTL_NOTIF_RECV

[Kees Cook]

On Mon, Jun 24, 2024 at 05:17:09PM -0700, Andrei Vagin wrote:
> Are you waiting for anything from me? I think this series is ready to be merged.

Oops, sorry for the silence! I had been waiting for Oleg's review, and
then it happened and I missed it, so it fell off my TODO list. :)

I just sent a reply with 2 bits of feedback, but with a v3, I think I
can land this ASAP.

Thanks for chasing me down, and I appreciate the selftest updates!

-Kees

-- 
Kees Cook