Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace

[Oleg Nesterov <oleg@redhat.com>]

On 11/01, Tycho Andersen wrote:
>
> On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:
> >
> > Somehow I no longer understand why do you need to take all locks. Isn't
> > the first filter's notify_lock enough? IOW,
> >
> > 		for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> > 			if (cur->notif)
> > 				return ERR_PTR(-EBUSY);
> > 			first = cur;
> > 		}
> >
> > 		if (first)
> > 			mutex_lock(&first->notify_lock);
> >
> > 		... initialize filter->notif ...
> >
> > 	out:
> > 		if (first)
> > 			mutex_unlock(&first->notify_lock);
> >
> > 		return ret;
>
> The idea here is to prevent people from "nesting" notify filters. So
> if any filter in the chain has a listener attached, it refuses to
> install another filter with a listener.

Yes, I understand, so we need to check cur->notif. My point was, we do not
need to take all the locks in the ->prev chain, we need only one:
first->notify_lock.

But you know what? today I think that we do not need any locking at all,
all we need is the lockless

	for (cur = current->seccomp.filter; cur; cur = cur->prev)
		if (cur->notif)
			return ERR_PTR(-EBUSY);

at the start, nothing more.

> But it just occurred to me that we don't handle the TSYNC case
> correctly by doing it this way,

Why? Perhaps I missed your point, but TSYNC case looks fine. I mean, if 2
threads do seccomp_set_mode_filter(NEW_LISTENER | TSYNC) then only one can
win the race and succeed, but this has nothing to do with init_listener(),
we rely on ->siglock and is_ancestor() check.

No?

Oleg.