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

[Tycho Andersen <tycho@tycho.ws>]

On Fri, Nov 02, 2018 at 11:02:35AM +0100, Oleg Nesterov wrote:
> 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.

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.

> > 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