Re: [PATCH v11 06/12] seccomp: add system call filtering using BPF

On Mon, Feb 27, 2012 at 11:54 AM, Will Drewry [off-list ref] wrote:

On Mon, Feb 27, 2012 at 11:09 AM, Oleg Nesterov [off-list ref] wrote:

quoted

On 02/24, Will Drewry wrote:

quoted

+static struct seccomp_filter *get_seccomp_filter(struct seccomp_filter *orig)
+{
+     if (!orig)
+             return NULL;
+     /* Reference count is bounded by the number of total processes. */
+     atomic_inc(&orig->usage);
+     return orig;
+}
...
+void copy_seccomp(struct seccomp *child, const struct seccomp *parent)
+{
+     /* Other fields are handled by dup_task_struct. */
+     child->filter = get_seccomp_filter(parent->filter);
+}

This is purely cosmetic, but imho looks a bit confusing.

We do not copy seccomp->mode and this is correct, it was already copied
implicitely. So why do we copy ->filter? This is not "symmetrical", afaics
you can simply do

       void copy_seccomp(struct seccomp *child)
       {
               if (child->filter)
                       atomic_inc(child->filter->usage);

But once again, this is cosmetic, feel free to ignore.

Right now get_seccomp_filter does the NULL check, so really this could
be reduced to adding an external get_seccomp_filter(p->seccomp.filter)
in place of copy_seccomp().

As to removing the extra arg, that should be fine since the parent
can't drop its refcount when copy_seccomp is called.  At the very
least, I can make that change so it reads more cleanly.

I had various conflicting thoughts while looking over the refcounting:

- get_seccomp_filter is defined static, and has a single caller: copy_seccomp()
- put isn't static, and has a single caller: kernel/fork.c:free_task()
- having only get_/put_ touch ->usage seems cleaner to me
- seccomp_attach_filter touches ->usage without get_seccomp_filter
- having the initializing routine use atomic_set(..., 1) is a common pattern

In a fit of extreme bike-shedding, I can't decide which is more sensible:

- rename put_seccomp_filter to free_seccomp_filter and inline the
get_seccomp_filter logic into copy_seccomp().

or

- create a wrapper for put_seccomp_filter named free_seccomp_filter so
that get_/put_ can both be static.

-Kees

-- 
Kees Cook
ChromeOS Security