On Sun, May 31, 2020 at 01:50:30PM +0200, Christian Brauner wrote:
> We've been making heavy use of the seccomp notifier to intercept and
> handle certain syscalls for containers. This patch allows a syscall
> supervisor listening on a given notifier to be notified when a seccomp
> filter has become unused.
>
> A container is often managed by a singleton supervisor process the
> so-called "monitor". This monitor process has an event loop which has
> various event handlers registered. If the user specified a seccomp
> profile that included a notifier for various syscalls then we also
> register a seccomp notify even handler. For any container using a
> separate pid namespace the lifecycle of the seccomp notifier is bound to
> the init process of the pid namespace, i.e. when the init process exits
> the filter must be unused.
> If a new process attaches to a container we force it to assume a seccomp
> profile. This can either be the same seccomp profile as the container
> was started with or a modified one. If the attaching process makes use
> of the seccomp notifier we will register a new seccomp notifier handler
> in the monitor's event loop. However, when the attaching process exits
> we can't simply delete the handler since other child processes could've
> been created (daemons spawned etc.) that have inherited the seccomp
> filter and so we need to keep the seccomp notifier fd alive in the event
> loop. But this is problematic since we don't get a notification when the
> seccomp filter has become unused and so we currently never remove the
> seccomp notifier fd from the event loop and just keep accumulating fds
> in the event loop. We've had this issue for a while but it has recently
> become more pressing as more and larger users make use of this.
>
> To fix this, we introduce a new "users" reference counter that tracks
> any tasks and dependent filters making use of a filter. When a notifier is
> registered waiting tasks will be notified that the filter is now empty by
> receiving a (E)POLLHUP event.
> The concept in this patch introduces is the same as for signal_struct,
> i.e. reference counting for life-cycle management is decoupled from
> reference counting taks using the object.
>
> There's probably some trickery possible but the second counter is just
> the correct way of doing this imho and has precedence. The patch also
> lifts the waitqeue from struct notification into sruct seccomp_filter.
> This is cleaner overall and let's us avoid having to take the notifier
> mutex since we neither need to read nor modify the notifier specific
> aspects of the seccomp filter. In the exit path I'd very much like to
> avoid having to take the notifier mutex for each filter in the task's
> filter hierarchy.
>
> Cc: Tycho Andersen <tycho@xxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Matt Denton <mpdenton@xxxxxxxxxx>
> Cc: Sargun Dhillon <sargun@xxxxxxxxx>
> Cc: Jann Horn <jannh@xxxxxxxxxx>
> Cc: Chris Palmer <palmer@xxxxxxxxxx>
> Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> Cc: Robert Sesek <rsesek@xxxxxxxxxx>
> Cc: Jeffrey Vander Stoep <jeffv@xxxxxxxxxx>
> Cc: Linux Containers <containers@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
> /* v2 */
> - Jann Horn <jannh@xxxxxxxxxx>:
> - Use more descriptive instead of seccomp_filter_notify().
> (I went with seccomp_filter_release().)
>
> /* v3 */
> - Kees Cook <keescook@xxxxxxxxxxxx>:
> - Rename counter from "live" to "users".
> ---
> kernel/seccomp.c | 68 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 53 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 55251b1fe03f..45244f1ba148 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -94,13 +94,11 @@ struct seccomp_knotif {
> * filter->notify_lock.
> * @next_id: The id of the next request.
> * @notifications: A list of struct seccomp_knotif elements.
> - * @wqh: A wait queue for poll.
> */
I split the wait queue changes into a separate patch...
> /**
> - * seccomp_filter_release - Detach the task from its filter tree
> - * and drop its reference count during
> - * exit.
> + * seccomp_filter_release - Detach the task from its filter tree,
> + * drop its reference count, and notify
> + * about unused filters
> *
> * This function should only be called when the task is exiting as
> * it detaches it from its filter tree.
> */
> void seccomp_filter_release(struct task_struct *tsk)
> {
> - struct seccomp_filter *cur = tsk->seccomp.filter;
> + struct seccomp_filter *orig = tsk->seccomp.filter;
>
> + /* Detach task from its filter tree. */
> tsk->seccomp.filter = NULL;
> - __put_seccomp_filter(cur);
> + /* Notify about any unused filters in the task's former filter tree. */
> + __seccomp_filter_orphan(orig);
> + /* Finally drop all references to the task's former tree. */
> + __put_seccomp_filter(orig);
> }
I added __seccomp_filter_release() to do the filter-specific parts (the
two functions passing "orig" above, so that it can be reused later...
>
> /**
> @@ -419,18 +441,29 @@ static inline void seccomp_sync_threads(unsigned long flags)
> /* Synchronize all threads. */
> caller = current;
> for_each_thread(caller, thread) {
> + struct seccomp_filter *cur = thread->seccomp.filter;
> +
> /* Skip current, since it needs no changes. */
> if (thread == caller)
> continue;
>
> /* Get a task reference for the new leaf node. */
> get_seccomp_filter(caller);
> +
> + /*
> + * Notify everyone as we're forcing the thread
> + * to orphan its current filter tree.
> + */
> + __seccomp_filter_orphan(cur);
> +
> /*
> - * Drop the task reference to the shared ancestor since
> - * current's path will hold a reference. (This also
> - * allows a put before the assignment.)
> + * Drop the task's reference to the shared ancestor
> + * since current's path will hold a reference.
> + * (This also allows a put before the assignment.)
> */
> - __put_seccomp_filter(thread->seccomp.filter);
> + __put_seccomp_filter(cur);
I switched this around to just call the new __seccomp_release_filter()
(there's no need to open-code this and add "cur"). I also removed the
comment about the notification, because that's not possible: "thread"
shares the same filter hierarchy as "caller", so the counts on "cur"
cannot reach 0 (no notifications can ever happen due to TSYNC).
Everything else looks great! I've applied it to for-next/seccomp.
--
Kees Cook
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
