On 11/01, Tycho Andersen wrote:

On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:

quoted

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.