Messages in this thread | | | Date | Thu, 1 Sep 2022 15:58:02 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH 4/4] seccomp: add the synchronous mode for seccomp_unotify |
| |
On Tue, Aug 30, 2022 at 02:23:24PM -0700, Andrei Vagin wrote: > On Tue, Aug 30, 2022 at 3:43 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Mon, Aug 29, 2022 at 06:43:56PM -0700, Andrei Vagin wrote: > > > seccomp_unotify allows more privileged processes does 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. > > > > Seems like a nice idea though I leave it to the sched people to judge > > whether this is sane or not. So the supervisor which gets woken will be > > moved to the current cpu in this synchronous scenario. > > > > I have no strong opinions on this patch. There are two things I wonder > > about. First, how meaningful is that speed up given that the supervisor > > will most often do a lot of heavy-handed things anyway. > > I would not use the "most often" phrase in this case;). It is true for LXC-like > use cases when we need to handle rare syscalls. In this case, the performance > of this interface doesn't play a big role. But my use case is very different. I > have a prototype of the gVisor platform, where seccomp is used to trap > guest system calls. In this case, the difference between 12µs and 3µs is > tremendous.
Oh yeah, makes sense. I don't know enough about gVisor but I know we can trust your word! :)
> > The idea of WF_CURRENT_CPU is not mine. I spied it from the umcg series. > I took the second patch from that series without any changes. > > > > > Second, this flag is a very specific thing and I wonder how much > > userspace will really use this and what's more use this correctly. > > > > Just to note that LXD - one of the biggest user of this feature - isn't > > synchronous iiuc for example. Each container gets a separate seccomp > > supervisor thread (well, go routine but whatever) which exposes a socket > > that the container manager connects to and sends the seccomp > > notifications it received from its payload according to an api we > > established. And each notification is handled in a separate thread > > (again, go routine but whatever). > > It could be synchronous if seccomp events had been handled in [lxc monitor]. But > right now, [lxc monitor] is just a proxy. In this case, you are right, lxc will
Yep.
> not get any benefits by setting this flag. But we can look at this from another > side. If we add these changes, we will have another big user of the interface. I > think the number of gVisor containers that are started each day is comparable > with the number of LXC/LXD containers.
Sure, if there's users that would benefit from this then no reason to not consider it. It's just a lot of low-level knobs we give userspace here but I guess for the notifier it makes sense.
> > > > > > > > > This change introduce the SECCOMP_USER_NOTIF_FD_SYNC_WAKE_UP flag that > > > it used to enable the sync mode. > > > > > > Signed-off-by: Andrei Vagin <avagin@gmail.com> > > > --- > > > include/uapi/linux/seccomp.h | 4 ++++ > > > kernel/seccomp.c | 35 +++++++++++++++++++++++++++++++++-- > > > 2 files changed, 37 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; > > > }; > > > > > <snip> > > > > > > > #ifdef SECCOMP_ARCH_NATIVE > > > @@ -1117,7 +1120,10 @@ static int seccomp_do_user_notification(int this_syscall, > > > INIT_LIST_HEAD(&n.addfd); > > > > > > atomic_add(1, &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); > > > > (We're accumulating a lot of conditional wake primitives in the notifier.) > > > > I am not sure that I understand what you mean here.
I just meant that we have
if (wait_killable) err = wait_for_completion_killable(&n.ready); else err = wait_for_completion_interruptible(&n.ready);
and now also
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);
which is a bit unpleasant but nothing that would mean we can't do this.
| |