Re: [RFC 0/3] seccomp trap to userspace

[Andy Lutomirski]

On Thu, Mar 15, 2018 at 5:25 PM, Christian Brauner
<christian.brauner@xxxxxxxxxxxxx> wrote:
> On Thu, Mar 15, 2018 at 05:11:32PM +0000, Andy Lutomirski wrote:
>> On Thu, Mar 15, 2018 at 5:05 PM, Serge E. Hallyn <serge@xxxxxxxxxx> wrote:
>> > Quoting Andy Lutomirski (luto@xxxxxxxxxx):
>> >> On Thu, Mar 15, 2018 at 4:09 PM, Christian Brauner
>> >> <christian.brauner@xxxxxxxxxxxxx> wrote:
>> >> > On Sun, Feb 04, 2018 at 11:49:43AM +0100, Tycho Andersen wrote:
>> >> >> Several months ago at Linux Plumber's, we had a discussion about adding a
>> >> >> feature to seccomp which would allow seccomp to trigger a notification for some
>> >> >> other process. Here's a draft of that feature.
>> >> >>
>> >> >> Patch 1 contains the bulk of it, patches 2 & 3 offer an alternative way to
>> >> >> acquire the fd that receives notifications via ptrace (the method in patch 1
>> >> >> poses some problems). Other suggestions for how to acquire one of these fds
>> >> >> would be welcome.
>> >> >>
>> >> >> Take a close look at the synchronization. I think I've got it right, but I
>> >> >> probably don't :)
>> >> >>
>> >> >> Thanks!
>> >> >>
>> >> >> Tycho Andersen (3):
>> >> >>   seccomp: add a return code to trap to userspace
>> >> >>   seccomp: hoist out filter resolving logic
>> >> >>   seccomp: add a way to get a listener fd from ptrace
>> >> >>
>> >> >>  arch/Kconfig                                  |   7 +
>> >> >>  include/linux/seccomp.h                       |  14 +-
>> >> >>  include/uapi/linux/ptrace.h                   |   1 +
>> >> >>  include/uapi/linux/seccomp.h                  |  18 +-
>> >> >>  kernel/ptrace.c                               |   4 +
>> >> >>  kernel/seccomp.c                              | 467 ++++++++++++++++++++++++--
>> >> >>  tools/testing/selftests/seccomp/seccomp_bpf.c | 180 +++++++++-
>> >> >>  7 files changed, 653 insertions(+), 38 deletions(-)
>> >> >
>> >> > Hey,
>> >> >
>> >> > So, I've been following the discussion silently in the background and I
>> >> > see that it got sidetracked into seccomp + ebpf. While I can see that
>> >> > there is value in adding epbf support to seccomp I'd really like to see
>> >> > this decoupled from this patchset. Afaict, this patchset would just work
>> >> > fine without the ebpf portion (but I might be just have missed the
>> >> > point). So if possible I would like to see a second version of this with
>> >> > the comments accounted for and - if possible - have this up for merging
>> >> > independent of the ebpf patchset that's floating around.
>> >> >
>> >>
>> >> The issue is that it might be (and, then again, might not be) nicer to
>> >> to *synchronously* call out to the monitor in the filter.  eBPF can do
>> >> that very cleanly, whereas classic BPF can't.
>> >
>> > Hm, synchronously - that brings to mind a thought...  I should re-look at
>> > Tycho's patches first, but, if I'm in a container, start some syscall that
>> > gets trapped to userspace, then I hit ctrl-c.  I'd like to be able to have
>> > the handler be interrupted and have it return -EINTR.  Is that going to
>> > be possible with the synchronous approach?
>>
>> I think so, but it should be possible with the classic async approach
>> too.  The main issue is the difference between a classic filter like
>> this (pseudocode):
>>
>> if (nr == SYS_mount) return TRAP_TO_USERSPACE;
>>
>> and the eBPF variant:
>>
>> if (nr == SYS_mount) trap_to_userspace();
>>
>> I admit that it's still not 100% clear to me that the latter is
>> genuinely more useful than the former.
>
> We've just discussed this on irc and the fact that most problems can be
> addressed by interfaces we already have makes it questionable what ebpf
> brings to the game here. Especially since the discussion gave the
> impression that if ebpf ever makes it to seccomp it will basically be
> because it allows a nice implementation of the trap to userspace. If
> it's even unclear whether it is really the better choice for this task
> then we could consider to no try and make this patchset use it. (I
> probably sound way more polemic than I intend to.)
>

No argument from me.

To be clear, I don't think that trap to userspace should block on eBPF
at all.  It was just a coincidence that both patches showed up around
the same time, and if they actually work well together, then it could
make sense to combine them.  But if trap to userspace ends up working
perfectly without eBPF, that's fine too.