linux-kernel - Re: [PATCH] lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 May 2021 11:54:56 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Yejune Deng <yejune.deng@...il.com>, peterz@...radead.org,
        tglx@...utronix.de, bristot@...hat.com
Cc:     linux-kernel@...r.kernel.org, Yejune Deng <yejunedeng@...il.com>
Subject: Re: [PATCH] lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed

On 08/05/21 15:25, Yejune Deng wrote:
> Use is_percpu_thread() instead of 'current->nr_cpus_allowed == 1',
> is_percpu_thread() is includes boths SMP and SP. It also more readable.
> The comments are no needed.
>
> Signed-off-by: Yejune Deng <yejunedeng@...il.com>
> ---
>  lib/smp_processor_id.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
> index 1c1dbd3..046ac62 100644
> --- a/lib/smp_processor_id.c
> +++ b/lib/smp_processor_id.c
> @@ -19,11 +19,7 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
>       if (irqs_disabled())
>               goto out;
>
> -	/*
> -	 * Kernel threads bound to a single CPU can safely use
> -	 * smp_processor_id():
> -	 */
> -	if (current->nr_cpus_allowed == 1)
> +	if (is_percpu_thread())
>               goto out;

That's not entirely equivalent as this adds (p->flags & PF_NO_SETAFFINITY)
to the condition. Per-CPU kthreads will have it set, so that at least
matches the existing comment, but it's not an innocent change.

I'm thinking this might actually be something we want, as not having
this flag + having preemption enabled means this could race with
sched_setaffinity(), and it would be nice to detect this regardless of the
current affinity.

I ran this on my Juno and the idle thread gets in the way as it only has

  PF_KTHREAD | PF_IDLE

So you'd need something like the below (which is purely for instrumentation
/ assertion purposes).

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4a0668acd876..2a6fdf3e7061 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7445,7 +7445,7 @@ void init_idle(struct task_struct *idle, int cpu)
 
 	idle->state = TASK_RUNNING;
 	idle->se.exec_start = sched_clock();
-	idle->flags |= PF_IDLE;
+	idle->flags |= PF_IDLE | PF_NO_SETAFFINITY;
 
 	scs_task_reset(idle);
 	kasan_unpoison_task_stack(idle);