Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 28 May 2020 11:57:42 +0200
From:   Christian Brauner <christian.brauner@ubuntu.com>
To:     Jann Horn <jannh@google.com>
Cc:     kernel list <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Andy Lutomirski <luto@kernel.org>,
        Tycho Andersen <tycho@tycho.ws>,
        Matt Denton <mpdenton@google.com>,
        Sargun Dhillon <sargun@sargun.me>,
        Chris Palmer <palmer@google.com>,
        Aleksa Sarai <cyphar@cyphar.com>,
        Robert Sesek <rsesek@google.com>,
        Jeffrey Vander Stoep <jeffv@google.com>,
        Linux Containers <containers@lists.linux-foundation.org>
Subject: Re: [PATCH 1/2] seccomp: notify user trap about unused filter
Message-ID: <20200528095742.cjwemtucwgvhxnxv@wittgenstein>
References: <20200527111902.163213-1-christian.brauner@ubuntu.com>
 <CAG48ez3z4TTKHwQmZbeyb+egAdKsFb0kkReSLtJsA+9MP6VUBg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAG48ez3z4TTKHwQmZbeyb+egAdKsFb0kkReSLtJsA+9MP6VUBg@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 28, 2020 at 06:04:48AM +0200, Jann Horn wrote:
> On Wed, May 27, 2020 at 1:19 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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.
> [...]
> > To fix this, we introduce a new "live" reference counter that tracks the
> > live tasks making use of a given filter and 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 live taks using the object.
> [...]
> > + * @live: tasks that actually use this filter, only to be altered
> > + *          during fork(), exit()/free_task(), and filter installation
> 
> This comment is a bit off. Actually, @live counts the number of tasks
> that use the filter directly plus the number of dependent filters that
> have non-zero @live.

I'll update the comment.

> 
> [...]
> > +void seccomp_filter_notify(const struct task_struct *tsk)
> > +{
> > +       struct seccomp_filter *orig = tsk->seccomp.filter;
> > +
> > +       while (orig && refcount_dec_and_test(&orig->live)) {
> > +               if (waitqueue_active(&orig->wqh))
> > +                       wake_up_poll(&orig->wqh, EPOLLHUP);
> > +               orig = orig->prev;
> > +       }
> > +}
> 
> /me fetches the paint bucket
> 
> Maybe name this seccomp_filter_unuse() or
> seccomp_filter_unuse_notify() or something like that? The current name
> isn't very descriptive.

I think seccomp_filter_release() might be the right color. It would also
line-up nicely with:
- cgroup_release()
- exit_mm_release()
- exec_mm_release()
- futex_exec_release()
- ptrace_release_task()
and others.

Christian