On Fri, Nov 02, 2018 at 11:02:35AM +0100, Oleg Nesterov wrote:

On 11/01, Tycho Andersen wrote:

quoted

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.

Hmm, you're right. The locking is residual from when the old ptrace()
API was also a thing: with that, you could attach a filter at any
point in the tree, so we needed to lock to prevent that. But now
that it's only possible to do it at the bottom, we don't need to lock.

quoted

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.

Yes, agreed.

Thanks!

Tycho