lkml.org 
[lkml]   [2015]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v7 07/11] arch/x86: enable task isolation functionality
    On Mon, Sep 28, 2015 at 2:57 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote:
    > On 09/28/2015 04:59 PM, Andy Lutomirski wrote:
    >>
    >> On Mon, Sep 28, 2015 at 11:17 AM, Chris Metcalf <cmetcalf@ezchip.com>
    >> wrote:
    >>>
    >>> In prepare_exit_to_usermode(), we would like to call
    >>> task_isolation_enter() on every return to userspace, and like
    >>> other work items, we would like to recheck for more work after
    >>> calling it, since it will enable interrupts internally.
    >>>
    >>> However, if task_isolation_enter() is the only work item,
    >>> and it has already been called once, we don't want to continue
    >>> calling it in a loop. We don't have a dedicated TIF flag for
    >>> task isolation, and it wouldn't make sense to have one, since
    >>> we'd want to set it before starting exit every time, and then
    >>> clear it the first time around the loop.
    >>>
    >>> Instead, we change the loop structure somewhat, so that we
    >>> have a more inclusive set of flags that are tested for on the
    >>> first entry to the function (including TIF_NOHZ), and if any
    >>> of those flags are set, we enter the loop. And, we do the
    >>> task_isolation() test unconditionally at the bottom of the loop,
    >>> but then when making the decision to loop back, we just use the
    >>> set of flags that doesn't include TIF_NOHZ. That way we only
    >>> loop if there is other work to do, but then if that work
    >>> is done, we again unconditionally call task_isolation_enter().
    >>>
    >>> In syscall_trace_enter_phase1(), we try to add the necessary
    >>> support for strict-mode detection of syscalls in an optimized
    >>> way, by letting the code remain unchanged if we are not using
    >>> TASK_ISOLATION, but otherwise calling enter_from_user_mode()
    >>> under the first time we see _TIF_NOHZ, and then waiting until
    >>> after we do the secure computing work to actually clear the bit
    >>> from the "work" variable and call task_isolation_syscall().
    >>>
    >>> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
    >>> ---
    >>> arch/x86/entry/common.c | 47
    >>> ++++++++++++++++++++++++++++++++++++-----------
    >>> 1 file changed, 36 insertions(+), 11 deletions(-)
    >>>
    >>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
    >>> index 80dcc9261ca3..0f74389c6f3b 100644
    >>> --- a/arch/x86/entry/common.c
    >>> +++ b/arch/x86/entry/common.c
    >>> @@ -21,6 +21,7 @@
    >>> #include <linux/context_tracking.h>
    >>> #include <linux/user-return-notifier.h>
    >>> #include <linux/uprobes.h>
    >>> +#include <linux/isolation.h>
    >>>
    >>> #include <asm/desc.h>
    >>> #include <asm/traps.h>
    >>> @@ -81,7 +82,8 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs
    >>> *regs, u32 arch)
    >>> */
    >>> if (work & _TIF_NOHZ) {
    >>> enter_from_user_mode();
    >>> - work &= ~_TIF_NOHZ;
    >>> + if (!IS_ENABLED(CONFIG_TASK_ISOLATION))
    >>> + work &= ~_TIF_NOHZ;
    >>> }
    >>> #endif
    >>>
    >>> @@ -131,6 +133,13 @@ unsigned long syscall_trace_enter_phase1(struct
    >>> pt_regs *regs, u32 arch)
    >>> }
    >>> #endif
    >>>
    >>> + /* Now check task isolation, if needed. */
    >>> + if (IS_ENABLED(CONFIG_TASK_ISOLATION) && (work & _TIF_NOHZ)) {
    >>> + work &= ~_TIF_NOHZ;
    >>> + if (task_isolation_strict())
    >>> + task_isolation_syscall(regs->orig_ax);
    >>> + }
    >>> +
    >>
    >> This is IMO rather nasty. Can you try to find a way to do this
    >> without making the control flow depend on config options?
    >
    >
    > Well, I suppose this is the best argument for testing for task
    > isolation before seccomp :-)
    >
    > Honestly, if not, it's tricky to see how to do better; I did spend
    > some time looking at it. One possibility is to just unconditionally
    > clear _TIF_NOHZ before testing "work == 0", so that we can
    > test (work & TIF_NOHZ) once early and once after seccomp.
    > This presumably costs a cycle in the no-nohz-full case.
    >
    > So maybe just do it before seccomp...
    >
    >> What guarantees that TIF_NOHZ is an acceptable thing to check?
    >
    >
    > Well, TIF_NOHZ is set on all tasks whenever we are running with
    > nohz_full enabled anywhere, so testing it lets us do stuff on
    > the fastpath without slowing down the fastpath much.
    > See context_tracking_cpu_set().
    >
    >
    >>> /* Do our best to finish without phase 2. */
    >>> if (work == 0)
    >>> return ret; /* seccomp and/or nohz only (ret == 0 here)
    >>> */
    >>> @@ -217,10 +226,26 @@ static struct thread_info
    >>> *pt_regs_to_thread_info(struct pt_regs *regs)
    >>> /* Called with IRQs disabled. */
    >>> __visible void prepare_exit_to_usermode(struct pt_regs *regs)
    >>> {
    >>> + u32 cached_flags;
    >>> +
    >>> if (WARN_ON(!irqs_disabled()))
    >>> local_irq_disable();
    >>>
    >>> /*
    >>> + * We may want to enter the loop here unconditionally to make
    >>> + * sure to do some work at least once. Test here for all
    >>> + * possible conditions that might make us enter the loop,
    >>> + * and return immediately if none of them are set.
    >>> + */
    >>> + cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);
    >>> + if (!(cached_flags & (TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
    >>> + _TIF_UPROBE | _TIF_NEED_RESCHED |
    >>> + _TIF_USER_RETURN_NOTIFY | _TIF_NOHZ))) {
    >>> + user_enter();
    >>> + return;
    >>> + }
    >>> +
    >>
    >> Too complicated and too error prone.
    >>
    >> In any event, I don't think that the property you actually want is for
    >> the loop to be entered once. I think the property you want is that
    >> we're isolated by the time we're finished. Why not just check that
    >> directly in the loop condition?
    >
    >
    > So something like this (roughly):
    >
    > if (!(cached_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
    > _TIF_UPROBE | _TIF_NEED_RESCHED |
    > _TIF_USER_RETURN_NOTIFY)) &&
    > + task_isolation_done())
    > break;
    >
    > i.e. just add the one extra call? That could work, I suppose.
    > In the body we would then keep the proposed logic that unconditionally
    > calls task_isolation_enter().

    Yeah, I think so.

    >> Does anything here guarantee forward progress or at least give
    >> reasonable confidence that we'll make forward progress?
    >
    >
    > A given task can get stuck in the kernel if it has a lengthy far-future
    > alarm() type situation, or if there are multiple task-isolated tasks
    > scheduled onto the same core, but that only affects those tasks;
    > other tasks on the same core, and the system as a whole, are OK.

    Why are we treating alarms as something that should defer entry to
    userspace? I think it would be entirely reasonable to set an alarm
    for ten minutes, ask for isolation, and then think hard for ten
    minutes.

    A bigger issue would be if there's an RT task that asks for isolation
    and a bunch of other stuff (most notably KVM hosts) running with
    uncontrained affinity at full load. If task_isolation_enter always
    sleeps, then your KVM host will get scheduled, and it'll ask for a
    user return notifier on the way out, and you might just loop forever.
    Can this happen?

    ISTM something's suboptimal with the inner workings of all this if
    task_isolation_enter needs to sleep to wait for an event that isn't
    scheduled for the immediate future (e.g. already queued up as an
    interrupt).

    --Andy


    \
     
     \ /
      Last update: 2015-09-29 01:01    [W:2.659 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site