Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp user notifier

[Rodrigo Campos <rodrigo@kinvolk.io>]

On Fri, Apr 29, 2022 at 4:32 AM Sargun Dhillon <sargun@sargun.me> wrote:
> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> index 539e9d4a4860..204cf5ba511a 100644
> --- a/Documentation/userspace-api/seccomp_filter.rst
> +++ b/Documentation/userspace-api/seccomp_filter.rst
> @@ -271,6 +271,14 @@ notifying process it will be replaced. The supervisor can also add an FD, and
>  respond atomically by using the ``SECCOMP_ADDFD_FLAG_SEND`` flag and the return
>  value will be the injected file descriptor number.
>
> +The notifying process can be preempted, resulting in the notification being
> +aborted. This can be problematic when trying to take actions on behalf of the
> +notifying process that are long-running and typically retryable (mounting a
> +filesytem). Alternatively, the at filter installation time, the
> +``SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV`` flag can be set. This flag makes it
> +such that when a user notification is received by the supervisor, the notifying
> +process will ignore non-fatal signals until the response is sent.

Maybe:

This flags ignores non-fatal signals that arrive after the supervisor
received the notification

I mean, I want to make it clear that if a signal arrives before the
notification was received by the supervisor, then it will be
interrupted anyways.


> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index db10e73d06e0..9291b0843cb2 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1485,6 +1512,9 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>                 mutex_lock(&filter->notify_lock);
>                 knotif = find_notification(filter, unotif.id);
>                 if (knotif) {
> +                       /* Reset the process to make sure it's not stuck */
> +                       if (should_sleep_killable(filter, knotif))
> +                               complete(&knotif->ready);
>                         knotif->state = SECCOMP_NOTIFY_INIT;
>                         up(&filter->notif->request);

(I couldn't git-am this locally, so maybe I'm injecting this at the
wrong parts mentally when looking at the other code for more context.
Sorry if that is the case :))

Why do we need to complete() only in this error path? As far as I can
see this is on the error path where the copy to userspace failed and
we want to reset this notification.

I think that is wrong, we want to wake up the other side not just on
the error path, but on the non-error path (in fact, do we want to do
this on the error path? It seems like a no-op, but don't see any
reason to do it).

We _need_ to call complete() in the non error path here so the other
side wakes up and switches to a killable wait. As we are not doing
this (for the non error path), this will basically not achieve a
wait_killable() at all.

I think this was probably an oversight adapting the patch from last
year. Is it possble? Because it seems that in the previous version we
sent last year[1] (if you can link them next time it will be way
simpler :)) you had a new ioctl() and the call to complete() was
handled there, in seccomp_notify_set_wait_killable(). Now, as this is
part of the filter (and as I said last year, I think this way looks
better) that call to complete() was completely forgotten.

Is it possible that this is not really working as intended, then? Am I
missing something?


Best,
Rodrigo


[1]: https://lore.kernel.org/all/20210430204939.5152-3-sargun@sargun.me/