linux-kernel - Re: [PATCH 0/9] kthread detection cleanup

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:   Wed, 14 Aug 2019 14:43:17 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org
Cc:     ak@...ux.intel.com, akpm@...ux-foundation.org,
        bigeasy@...utronix.de, bp@...e.de, catalin.marinas@....com,
        davem@...emloft.net, hch@....de, kan.liang@...el.com,
        mingo@...nel.org, peterz@...radead.org, riel@...riel.com,
        will@...nel.org, Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [PATCH 0/9] kthread detection cleanup

On 14/08/2019 11:41, Mark Rutland wrote:
> A reasonable amount of kernel code looks at task_struct::mm to determine
> whether a thread is a kthread or a real user task. This isn't quite right,
> since kthreads can have a non-NULL mm when calling use_mm().
> 
> The correct way to check whether a task is a kthread is to check whether
> PF_KTHREAD is set in task_struct::flags, but doing so is a bit unwieldy.
> 
> To make this a bit nicer, this series adds a new is_kthread(tsk) helper,
> converts existing code to make use of it, and fixes up a number of erroneous
> checks of current->mm.  Hopefully this will push people to use the right
> approach in future.
> 
> I'm sure there are other instances in the kernel tree where we don't check this
> correctly. In this series I'm just trying to fix the instances I'm reasonably
> confident are incorrect.
> 

I've only found one extra location in kernel/sched/ that seems like it
wants to use is_kthread(), included below. I can send it out separately
after this gets merged.

-----8<-----
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4c4a0494c39b..0956972f6ea7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1964,11 +1964,10 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
 out:
 	if (state != cpuset) {
 		/*
-		 * Don't tell them about moving exiting tasks or
-		 * kernel threads (both mm NULL), since they never
-		 * leave kernel.
+		 * Don't tell them about moving exiting tasks (NULL mm) or
+		 * kernel threads since they never leave kernel.
 		 */
-		if (p->mm && printk_ratelimit()) {
+		if ((!is_kthread(p) && p->mm) && printk_ratelimit()) {
 			printk_deferred("process %d (%s) no longer affine to cpu%d\n",
 					task_pid_nr(p), p->comm, cpu);
 		}
----->8-----

Regarding sched/fair.c, I am quite convinced all of the p->mm checks are
genuine (as in they don't care if it's a kthread or not) - those are all
about NUMA balancing. Maybe Rik/Mel can correct me if I'm wrong.

Cheers,
Valentin