Re: [PATCH 4/6] seccomp: add the synchronous mode for seccomp_unotify

[Andy Lutomirski <luto@kernel.org>]

On Tue, Mar 7, 2023 at 11:32 PM Andrei Vagin <avagin@google.com> wrote:
>
> seccomp_unotify allows more privileged processes do actions on behalf
> of less privileged processes.
>
> In many cases, the workflow is fully synchronous. It means a target
> process triggers a system call and passes controls to a supervisor
> process that handles the system call and returns controls to the target
> process. In this context, "synchronous" means that only one process is
> running and another one is waiting.
>
> There is the WF_CURRENT_CPU flag that is used to advise the scheduler to
> move the wakee to the current CPU. For such synchronous workflows, it
> makes context switches a few times faster.
>
> Right now, each interaction takes 12µs. With this patch, it takes about
> 3µs.

This is great, but:

>
> This change introduce the SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP flag that
> it used to enable the sync mode.

Other than being faster, what does this flag actually do in terms of
user-visible semantics?

>
> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  include/uapi/linux/seccomp.h |  4 ++++
>  kernel/seccomp.c             | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 0fdc6ef02b94..dbfc9b37fcae 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -115,6 +115,8 @@ struct seccomp_notif_resp {
>         __u32 flags;
>  };
>
> +#define SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP (1UL << 0)
> +
>  /* valid flags for seccomp_notif_addfd */
>  #define SECCOMP_ADDFD_FLAG_SETFD       (1UL << 0) /* Specify remote fd */
>  #define SECCOMP_ADDFD_FLAG_SEND                (1UL << 1) /* Addfd and return it, atomically */
> @@ -150,4 +152,6 @@ struct seccomp_notif_addfd {
>  #define SECCOMP_IOCTL_NOTIF_ADDFD      SECCOMP_IOW(3, \
>                                                 struct seccomp_notif_addfd)
>
> +#define SECCOMP_IOCTL_NOTIF_SET_FLAGS  SECCOMP_IOW(4, __u64)
> +
>  #endif /* _UAPI_LINUX_SECCOMP_H */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 9fca9345111c..d323edeae7da 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -143,9 +143,12 @@ struct seccomp_kaddfd {
>   *           filter->notify_lock.
>   * @next_id: The id of the next request.
>   * @notifications: A list of struct seccomp_knotif elements.
> + * @flags: A set of SECCOMP_USER_NOTIF_FD_* flags.
>   */
> +
>  struct notification {
>         atomic_t requests;
> +       u32 flags;
>         u64 next_id;
>         struct list_head notifications;
>  };
> @@ -1117,7 +1120,10 @@ static int seccomp_do_user_notification(int this_syscall,
>         INIT_LIST_HEAD(&n.addfd);
>
>         atomic_inc(&match->notif->requests);
> -       wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
> +       if (match->notif->flags & SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
> +               wake_up_poll_on_current_cpu(&match->wqh, EPOLLIN | EPOLLRDNORM);
> +       else
> +               wake_up_poll(&match->wqh, EPOLLIN | EPOLLRDNORM);
>
>         /*
>          * This is where we wait for a reply from userspace.
> @@ -1593,7 +1599,10 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
>         knotif->error = resp.error;
>         knotif->val = resp.val;
>         knotif->flags = resp.flags;
> -       complete(&knotif->ready);
> +       if (filter->notif->flags & SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
> +               complete_on_current_cpu(&knotif->ready);
> +       else
> +               complete(&knotif->ready);
>  out:
>         mutex_unlock(&filter->notify_lock);
>         return ret;
> @@ -1623,6 +1632,22 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
>         return ret;
>  }
>
> +static long seccomp_notify_set_flags(struct seccomp_filter *filter,
> +                                   unsigned long flags)
> +{
> +       long ret;
> +
> +       if (flags & ~SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP)
> +               return -EINVAL;
> +
> +       ret = mutex_lock_interruptible(&filter->notify_lock);
> +       if (ret < 0)
> +               return ret;
> +       filter->notif->flags = flags;
> +       mutex_unlock(&filter->notify_lock);
> +       return 0;
> +}
> +
>  static long seccomp_notify_addfd(struct seccomp_filter *filter,
>                                  struct seccomp_notif_addfd __user *uaddfd,
>                                  unsigned int size)
> @@ -1752,6 +1777,8 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
>         case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
>         case SECCOMP_IOCTL_NOTIF_ID_VALID:
>                 return seccomp_notify_id_valid(filter, buf);
> +       case SECCOMP_IOCTL_NOTIF_SET_FLAGS:
> +               return seccomp_notify_set_flags(filter, arg);
>         }
>
>         /* Extensible Argument ioctls */
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>