lkml.org 
[lkml]   [2020]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [RFC PATCH 14/16] irq: Add support for core-wide protection of IRQ and softirq
    Date
    Vineeth, Joel!

    Vineeth Remanan Pillai <vpillai@digitalocean.com> writes:
    > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
    >
    > Lastly, we also check in the schedule loop if we are about to schedule
    > an untrusted process while the core is in such a state. This is possible
    > if a trusted thread enters the scheduler by way of yielding CPU. This
    > would involve no transitions through the irq_exit() point to do any
    > waiting, so we have to explicitly do the waiting there.

    Lot's of 'we' and 'this patch' in this changelog. Care to read
    Documentation/process/submitting-patches.rst ?

    > Every attempt is made to prevent a busy-wait unnecessarily, and in
    > testing on real-world ChromeOS usecases, it has not shown a performance
    > drop. In ChromeOS, with this and the rest of the core scheduling
    > patchset, we see around a 300% improvement in key press latencies into
    > Google docs when Camera streaming is running simulatenously (90th
    > percentile latency of ~150ms drops to ~50ms).
    >
    > This fetaure is controlled by the build time config option
    > CONFIG_SCHED_CORE_IRQ_PAUSE and is enabled by default. There is also a
    > kernel boot parameter 'sched_core_irq_pause' to enable/disable the
    > feature at boot time. Default is enabled at boot time.

    Enabled by default? At least that wants to depend on the enablement of
    core scheduling at boot time.

    >
    > +#ifdef CONFIG_SCHED_CORE_IRQ_PAUSE
    > +DEFINE_STATIC_KEY_TRUE(sched_core_irq_pause);
    > +static int __init set_sched_core_irq_pause(char *str)

    Gah. newlines are overrated, right? Glueing variable declarations and
    function declarations next to each other is unreadable.

    > +{
    > + unsigned long val = 0;
    > + if (!str)

    Missing newline between declaration and code. checkpatch.pl has told you
    so.

    > + return 0;
    > +
    > + val = simple_strtoul(str, &str, 0);

    Huch? Why are you using simple_strtoul() for parsing a boolean and
    what's the point of setting the **end argument to &str?

    > +
    > + if (val == 0)

    if (!val)

    is the usual kernel coding style.

    > + static_branch_disable(&sched_core_irq_pause);
    > +
    > + return 1;
    > +}
    > +__setup("sched_core_irq_pause=", set_sched_core_irq_pause);
    > +#endif
    > +
    > asmlinkage __visible void __softirq_entry __do_softirq(void)
    > {
    > unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
    > @@ -273,6 +291,16 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
    > /* Reset the pending bitmask before enabling irqs */
    > set_softirq_pending(0);
    >
    > +#ifdef CONFIG_SCHED_CORE_IRQ_PAUSE
    > + /*
    > + * Core scheduling mitigations require entry into softirq to send stall
    > + * IPIs to sibling hyperthreads if needed (ex, sibling is running
    > + * untrusted task). If we are here from irq_exit(), no IPIs are sent.
    > + */
    > + if (static_branch_likely(&sched_core_irq_pause))
    > + sched_core_irq_enter();

    So you already have a #ifdef CONFIG_SCHED_CORE_IRQ_PAUSE section
    above. Why on earth can't you just provide helper functions and stubs
    for the CONFIG_SCHED_CORE_IRQ_PAUSE=n case there instead of sprinkling
    #ifdeffery all over the place?

    > +#endif
    > +
    > local_irq_enable();

    How is __do_softirq() relevant at all?

    __do_softirq() is called from:

    1) the tail of interrupt handling
    2) the tail of local_bh_enable()
    3) the ksoftirq thread
    None of this is relevant for what you are trying to do:

    #1: Iscovered by the interrupt handling code simply because this nests
    into the interrupt handling.

    #2: local_bh_enable() only ends up there when invoked from thread
    context.

    This would only make sense if one sibling is in the kernel and the
    other in user space or guest mode. Isn't that forbidden by core
    scheduling in the first place?

    #3: See #2

    But what's worse is that this is called from an interrupt disabled
    region. So you brilliantly created a trivial source of livelocks.
    stomp_machine() being the most obvious candidate. But it's easy enough
    to come up with more subtle ones.

    The point is that you want to bring out the sibling thread from user
    space or guest mode when the other HT thread:

    A) invokes a syscall or traps into the hypervisor which is
    semantically equivalent (not code wise).

    B) handles an interrupt/exception

    So #A covers #2 and #3 above and #B covers #1 above.

    But you have the same livelock problem with doing this core wait thingy
    within any interrupt disabled region.

    What you really want to do is on syscall and interrupt/exception entry:

    if (other_sibling_in_user_or_guest())
    send_IPI();
    Of course it's conveniant to piggy pack that onto the reschedule IPI,
    but that sucks. What you want is a dedicated IPI which does:

    magic_ipi()
    set_thread_flag(TIF_CORE_SCHED_EXIT);

    And on the exit side you want to look at the entry code generalisation I
    just posted:

    https://lore.kernel.org/r/20200716182208.180916541@linutronix.de

    Add TIF_CORE_SCHED_EXIT to EXIT_TO_USER_MODE_WORK and to
    EXIT_TO_GUEST_MODE_WORK and handle it in exit_to_guest_mode_work() and
    exit_to_user_mode_loop() with interrupts enabled.

    Trying to do this in fully interrupt disabled context is just a recipe
    for disaster.

    The entry case condition wants to have a TIF bit as well, i.e.

    if (thread_test_bit(TIF_CORE_SCHED_REQUIRED) {
    sched_ipi_dance() {
    if (other_sibling_in_user_or_guest())
    send_IPI();
    }
    }
    I know you hate this, but we are not going to add tons of unholy hackery
    to make this "work" by some definition of work.

    The only way to make this work without making a complete mess and
    killing performance is to do this on a best effort basis. It really does
    not matter whether the result of the orchestration is perfect or
    not. What matters is that you destroy any predictability. If the
    orchestration terminates early occasionally then the resulting damage is
    a purely academic exercise.

    Making it work reliably without hard to debug livelocks and other
    horrors is not so academic.

    So please stop overengineering this and just take the pragmatic approach
    of making it mostly correct. That will prevent 99.999% of realistic
    attacks.

    It might not prevent the carefully orchestrated academic paper attack
    which utilizes the 0.001% failure rate by artificially enforcing a
    scenario which cannot be enforced in a real world environment.

    Thanks,

    tglx






    \
     
     \ /
      Last update: 2020-07-18 01:36    [W:2.226 / U:0.384 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site