mbox series

[0/9] kthread detection cleanup

Message ID 20190814104131.20190-1-mark.rutland@arm.com
Headers show
Series kthread detection cleanup | expand

Message

Mark Rutland Aug. 14, 2019, 10:41 a.m. UTC 
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.

This series is based on v5.3-rc3, and I've pushed it out to my
sched/kthread-cleanup branch on kernel.org [1,2].

Thanks,
Mark.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git sched/kthread-cleanup
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=sched/kthread-cleanup

Mark Rutland (9):
  sched/core: add is_kthread() helper
  sched: treewide: use is_kthread()
  kmemleak: correctly check for kthreads
  arm/perf: correctly check for kthreads
  arm64: correctly check for ktrheads
  sparc/perf: correctly check for kthreads
  x86/lbr: correctly check for kthreads
  x86/fpu: correctly check for kthreads
  samples/kretprobe: correctly check for kthreads

 arch/alpha/kernel/process.c         |  2 +-
 arch/arc/kernel/process.c           |  2 +-
 arch/arm/kernel/perf_callchain.c    |  2 +-
 arch/arm/kernel/process.c           |  2 +-
 arch/arm/mm/init.c                  |  2 +-
 arch/arm64/kernel/process.c         |  6 +++---
 arch/c6x/kernel/process.c           |  2 +-
 arch/csky/kernel/process.c          |  2 +-
 arch/h8300/kernel/process.c         |  2 +-
 arch/hexagon/kernel/process.c       |  2 +-
 arch/ia64/kernel/process.c          |  2 +-
 arch/m68k/kernel/process.c          |  2 +-
 arch/microblaze/kernel/process.c    |  2 +-
 arch/mips/kernel/process.c          |  4 ++--
 arch/nds32/kernel/process.c         |  4 ++--
 arch/nios2/kernel/process.c         |  2 +-
 arch/openrisc/kernel/process.c      |  2 +-
 arch/parisc/kernel/process.c        |  2 +-
 arch/powerpc/kernel/process.c       |  2 +-
 arch/riscv/kernel/process.c         |  2 +-
 arch/s390/kernel/process.c          |  2 +-
 arch/sh/kernel/process_32.c         |  2 +-
 arch/sh/kernel/process_64.c         |  2 +-
 arch/sparc/kernel/perf_event.c      |  2 +-
 arch/sparc/kernel/process_32.c      |  2 +-
 arch/sparc/kernel/process_64.c      |  2 +-
 arch/um/kernel/process.c            |  2 +-
 arch/unicore32/kernel/process.c     |  2 +-
 arch/x86/events/intel/lbr.c         |  2 +-
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/kernel/fpu/core.c          |  2 +-
 arch/x86/kernel/process_32.c        |  2 +-
 arch/x86/kernel/process_64.c        |  2 +-
 arch/xtensa/kernel/process.c        |  2 +-
 block/blk-cgroup.c                  |  2 +-
 drivers/tty/sysrq.c                 |  2 +-
 fs/coredump.c                       |  2 +-
 fs/file_table.c                     |  4 ++--
 fs/namespace.c                      |  2 +-
 fs/proc/base.c                      |  4 ++--
 include/linux/cgroup.h              |  2 +-
 include/linux/sched.h               | 13 ++++++++++++-
 kernel/cgroup/freezer.c             |  4 ++--
 kernel/events/core.c                |  2 +-
 kernel/exit.c                       |  2 +-
 kernel/fork.c                       |  6 +++---
 kernel/freezer.c                    |  4 ++--
 kernel/futex.c                      |  2 +-
 kernel/kthread.c                    |  6 +++---
 kernel/livepatch/transition.c       |  2 +-
 kernel/ptrace.c                     |  2 +-
 kernel/sched/core.c                 |  8 ++++----
 kernel/sched/idle.c                 |  2 +-
 kernel/sched/wait.c                 |  2 +-
 kernel/signal.c                     |  2 +-
 kernel/stacktrace.c                 |  2 +-
 lib/is_single_threaded.c            |  2 +-
 mm/kmemleak.c                       |  6 +++---
 mm/memcontrol.c                     |  2 +-
 mm/oom_kill.c                       |  4 ++--
 mm/page_alloc.c                     |  2 +-
 mm/vmacache.c                       |  2 +-
 mm/vmscan.c                         |  2 +-
 samples/kprobes/kretprobe_example.c |  2 +-
 security/smack/smack_access.c       |  2 +-
 security/smack/smack_lsm.c          |  4 ++--
 security/yama/yama_lsm.c            |  2 +-
 67 files changed, 97 insertions(+), 86 deletions(-)

-- 
2.11.0

Comments

Valentin Schneider Aug. 14, 2019, 1:43 p.m. UTC | #1 
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