lkml.org 
[lkml]   [2020]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10 14/14] powerpc: Use mm_context vas_windows counter to issue CP_ABORT
    Date
    Haren Myneni <haren@linux.ibm.com> writes:
    > set_thread_uses_vas() sets used_vas flag for a process that opened VAS
    > window and issue CP_ABORT during context switch for only that process.
    > In multi-thread application, windows can be shared. For example Thread A
    > can open a window and Thread B can run COPY/PASTE instructions to send
    > NX request which may cause corruption or snooping or a covert channel.
    > Also once this flag is set, continue to run CP_ABORT even the VAS window
    > is closed.
    >
    > So define vas-windows counter in process mm_context, increment this
    > counter for each window open and decrement it for window close. If
    > vas-windows is set, issue CP_ABORT during context switch. It means
    > clear the foreign real address mapping only if the process / thread uses
    > COPY/PASTE. Then disable it for that process if windows are not open.
    >
    > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
    > Reported-by: Nicholas Piggin <npiggin@gmail.com>
    > Suggested-by: Milton Miller <miltonm@us.ibm.com>
    > Suggested-by: Nicholas Piggin <npiggin@gmail.com>
    > ---
    > arch/powerpc/include/asm/book3s/64/mmu.h | 3 +++
    > arch/powerpc/include/asm/mmu_context.h | 22 ++++++++++++++++++++++
    > arch/powerpc/include/asm/processor.h | 1 -
    > arch/powerpc/kernel/process.c | 8 ++++++--
    > arch/powerpc/platforms/powernv/vas-window.c | 1 +
    > 5 files changed, 32 insertions(+), 3 deletions(-)

    This should presumably be tagged:

    Fixes: 9d2a4d71332c ("powerpc: Define set_thread_uses_vas()")

    I _think_ we don't need to backport it because currently there's no code
    in the kernel that will actually trigger the used_vas case, but please
    spell that out for me in the change log.

    > diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
    > index bb3deb7..f0a9ff6 100644
    > --- a/arch/powerpc/include/asm/book3s/64/mmu.h
    > +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
    > @@ -116,6 +116,9 @@ struct patb_entry {
    > /* Number of users of the external (Nest) MMU */
    > atomic_t copros;
    >
    > + /* Number of user space windows opened in process mm_context */
    > + atomic_t vas_windows;

    This should probably be a refcount_t.

    Which is an atomic that has wrappers that catch overflow/underflow cases
    for you, like you've open-coded below.

    > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
    > index 360367c..7fd249498 100644
    > --- a/arch/powerpc/include/asm/mmu_context.h
    > +++ b/arch/powerpc/include/asm/mmu_context.h
    > @@ -185,11 +185,33 @@ static inline void mm_context_remove_copro(struct mm_struct *mm)
    > dec_mm_active_cpus(mm);
    > }
    > }
    > +
    > +/*
    > + * vas_windows counter shows number of open windows in the mm
    > + * context. During context switch, use this counter to clear the
    > + * foreign real address mapping (CP_ABORT) for the thread / process
    > + * that intend to use COPY/PASTE. When a process closes all windows,
    > + * disable CP_ABORT which is expensive to run.
    > + */
    > +static inline void mm_context_add_vas_windows(struct mm_struct *mm)

    I think this would read better if it wasn't plural. "windows" implies
    you can add more than one at a time.

    So:

    static inline void mm_context_add_vas_window(struct mm_struct *mm)

    > +{
    > + atomic_inc(&mm->context.vas_windows);
    > +}
    > +
    > +static inline void mm_context_remove_vas_windows(struct mm_struct *mm)
    > +{
    > + int c = atomic_dec_if_positive(&mm->context.vas_windows);
    > +
    > + /* Detect imbalance between add and remove */
    > + WARN_ON(c < 0);

    ie. here.

    > +}
    > #else
    > static inline void inc_mm_active_cpus(struct mm_struct *mm) { }
    > static inline void dec_mm_active_cpus(struct mm_struct *mm) { }
    > static inline void mm_context_add_copro(struct mm_struct *mm) { }
    > static inline void mm_context_remove_copro(struct mm_struct *mm) { }
    > +static inline void mm_context_add_vas_windows(struct mm_struct *mm) { }
    > +static inline void mm_context_remove_vas_windows(struct mm_struct *mm) { }
    > #endif


    > diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
    > index eedcbfb..bfa336f 100644
    > --- a/arch/powerpc/include/asm/processor.h
    > +++ b/arch/powerpc/include/asm/processor.h
    > @@ -272,7 +272,6 @@ struct thread_struct {
    > unsigned mmcr0;
    >
    > unsigned used_ebb;
    > - unsigned int used_vas;
    > #endif
    > };
    >
    > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
    > index fad50db..a3ecaf9 100644
    > --- a/arch/powerpc/kernel/process.c
    > +++ b/arch/powerpc/kernel/process.c
    > @@ -1221,7 +1221,8 @@ struct task_struct *__switch_to(struct task_struct *prev,
    > * mappings, we must issue a cp_abort to clear any state and
    > * prevent snooping, corruption or a covert channel.
    > */
    > - if (current->thread.used_vas)
    > + if (current->mm &&
    > + atomic_read(&current->mm->context.vas_windows))
    > asm volatile(PPC_CP_ABORT);
    > }
    > #endif /* CONFIG_PPC_BOOK3S_64 */
    > @@ -1466,7 +1467,10 @@ int set_thread_uses_vas(void)

    set_thread_uses_vas() should probably just be moved into vas-window.c
    which is its only caller.

    > if (!cpu_has_feature(CPU_FTR_ARCH_300))
    > return -EINVAL;
    >
    > - current->thread.used_vas = 1;
    > + if (!current->mm)
    > + return -EINVAL;
    > +
    > + mm_context_add_vas_windows(current->mm);

    I needed to dig a bit to confirm this was the right place to call this.

    The call trace is:
    mm_context_add_vas_window()
    set_thread_uses_vas()
    vas_tx_win_open()
    coproc_ioc_tx_win_open()
    coproc_ioctl()

    > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
    > index 3ffad5a..33dfbbf 100644
    > --- a/arch/powerpc/platforms/powernv/vas-window.c
    > +++ b/arch/powerpc/platforms/powernv/vas-window.c
    > @@ -1333,6 +1333,7 @@ int vas_win_close(struct vas_window *window)
    > put_pid(window->pid);
    > if (window->mm) {
    > mm_context_remove_copro(window->mm);
    > + mm_context_remove_vas_windows(window->mm);
    > mmdrop(window->mm);
    > }
    > }

    And similarly here, this is:
    vas_win_close()
    coproc_release()


    cheers

    \
     
     \ /
      Last update: 2020-04-11 12:29    [W:3.796 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site