| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <b088d063-058a-2557-812c-6d5659f9d67b@au1.ibm.com>
Date: Fri, 20 Apr 2018 18:43:55 +1000
From: Andrew Donnellan <andrew.donnellan@....ibm.com>
To: "Alastair D'Silva" <alastair@....ibm.com>,
linuxppc-dev@...ts.ozlabs.org
Cc: linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
mikey@...ling.org, vaibhav@...ux.vnet.ibm.com,
aneesh.kumar@...ux.vnet.ibm.com, malat@...ian.org,
felix@...ux.vnet.ibm.com, pombredanne@...b.com,
sukadev@...ux.vnet.ibm.com, npiggin@...il.com,
gregkh@...uxfoundation.org, arnd@...db.de,
fbarrat@...ux.vnet.ibm.com, corbet@....net,
"Alastair D'Silva" <alastair@...ilva.org>,
Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
Christophe Lombard <clombard@...ux.vnet.ibm.com>
Subject: Re: [PATCH v2 3/7] powerpc: use task_pid_nr() for TID allocation
[+ Sukadev, Christophe]
On 18/04/18 11:08, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@...ilva.org>
>
> The current implementation of TID allocation, using a global IDR, may
> result in an errant process starving the system of available TIDs.
> Instead, use task_pid_nr(), as mentioned by the original author. The
> scenario described which prevented it's use is not applicable, as
> set_thread_tidr can only be called after the task struct has been
> populated.
>
> Signed-off-by: Alastair D'Silva <alastair@...ilva.org>
So it's too late in the evening for me to completely get my head around
what's going on here enough to give my Reviewed-by:, but my current
thinking is:
- In the first version of the patch to add TIDR support
(https://patchwork.ozlabs.org/patch/799494/), it was originally proposed
to call assign_thread_id() (as it was then called) from copy_thread()
- The comment block documents the reason why we can't use task_pid_nr()
but assumes that we're trying to assign a TIDR from within copy_thread()
- The final patch that was accepted
(https://patchwork.ozlabs.org/patch/835552/,
ec233ede4c8654894610ea54f4dae7adc954ac62) instead sets the TIDR to 0
from copy_thread(), so the original reasoning regarding not using
task_pid_nr() within copy_thread() is no longer applicable.
Sukadev: does this sound right?
Andrew
> ---
> arch/powerpc/include/asm/switch_to.h | 1 -
> arch/powerpc/kernel/process.c | 97 +-----------------------------------
> 2 files changed, 1 insertion(+), 97 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
> index be8c9fa23983..5b03d8a82409 100644
> --- a/arch/powerpc/include/asm/switch_to.h
> +++ b/arch/powerpc/include/asm/switch_to.h
> @@ -94,6 +94,5 @@ static inline void clear_task_ebb(struct task_struct *t)
> extern int set_thread_uses_vas(void);
>
> extern int set_thread_tidr(struct task_struct *t);
> -extern void clear_thread_tidr(struct task_struct *t);
>
> #endif /* _ASM_POWERPC_SWITCH_TO_H */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 3b00da47699b..87f047fd2762 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1496,103 +1496,12 @@ int set_thread_uses_vas(void)
> }
>
> #ifdef CONFIG_PPC64
> -static DEFINE_SPINLOCK(vas_thread_id_lock);
> -static DEFINE_IDA(vas_thread_ida);
> -
> -/*
> - * We need to assign a unique thread id to each thread in a process.
> - *
> - * This thread id, referred to as TIDR, and separate from the Linux's tgid,
> - * is intended to be used to direct an ASB_Notify from the hardware to the
> - * thread, when a suitable event occurs in the system.
> - *
> - * One such event is a "paste" instruction in the context of Fast Thread
> - * Wakeup (aka Core-to-core wake up in the Virtual Accelerator Switchboard
> - * (VAS) in POWER9.
> - *
> - * To get a unique TIDR per process we could simply reuse task_pid_nr() but
> - * the problem is that task_pid_nr() is not yet available copy_thread() is
> - * called. Fixing that would require changing more intrusive arch-neutral
> - * code in code path in copy_process()?.
> - *
> - * Further, to assign unique TIDRs within each process, we need an atomic
> - * field (or an IDR) in task_struct, which again intrudes into the arch-
> - * neutral code. So try to assign globally unique TIDRs for now.
> - *
> - * NOTE: TIDR 0 indicates that the thread does not need a TIDR value.
> - * For now, only threads that expect to be notified by the VAS
> - * hardware need a TIDR value and we assign values > 0 for those.
> - */
> -#define MAX_THREAD_CONTEXT ((1 << 16) - 1)
> -static int assign_thread_tidr(void)
> -{
> - int index;
> - int err;
> - unsigned long flags;
> -
> -again:
> - if (!ida_pre_get(&vas_thread_ida, GFP_KERNEL))
> - return -ENOMEM;
> -
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - err = ida_get_new_above(&vas_thread_ida, 1, &index);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> -
> - if (err == -EAGAIN)
> - goto again;
> - else if (err)
> - return err;
> -
> - if (index > MAX_THREAD_CONTEXT) {
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - ida_remove(&vas_thread_ida, index);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> - return -ENOMEM;
> - }
> -
> - return index;
> -}
> -
> -static void free_thread_tidr(int id)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&vas_thread_id_lock, flags);
> - ida_remove(&vas_thread_ida, id);
> - spin_unlock_irqrestore(&vas_thread_id_lock, flags);
> -}
> -
> -/*
> - * Clear any TIDR value assigned to this thread.
> - */
> -void clear_thread_tidr(struct task_struct *t)
> -{
> - if (!t->thread.tidr)
> - return;
> -
> - if (!cpu_has_feature(CPU_FTR_P9_TIDR)) {
> - WARN_ON_ONCE(1);
> - return;
> - }
> -
> - mtspr(SPRN_TIDR, 0);
> - free_thread_tidr(t->thread.tidr);
> - t->thread.tidr = 0;
> -}
> -
> -void arch_release_task_struct(struct task_struct *t)
> -{
> - clear_thread_tidr(t);
> -}
> -
> /*
> * Assign a unique TIDR (thread id) for task @t and set it in the thread
> * structure. For now, we only support setting TIDR for 'current' task.
> */
> int set_thread_tidr(struct task_struct *t)
> {
> - int rc;
> -
> if (!cpu_has_feature(CPU_FTR_P9_TIDR))
> return -EINVAL;
>
> @@ -1602,11 +1511,7 @@ int set_thread_tidr(struct task_struct *t)
> if (t->thread.tidr)
> return 0;
>
> - rc = assign_thread_tidr();
> - if (rc < 0)
> - return rc;
> -
> - t->thread.tidr = rc;
> + t->thread.tidr = (u16)task_pid_nr(t);
> mtspr(SPRN_TIDR, t->thread.tidr);
>
> return 0;
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@....ibm.com IBM Australia Limited
Powered by blists - more mailing lists