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.
