Re: stable 5.4.135 REGRESSION / once-a-day WARNING: at kthread_is_per_cpu+0x1c/0x20

[Patrick Schaaf <bof@bof.de>]

On Tue, Aug 31, 2021 at 9:01 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> 3a7956e25e1d ("kthread: Fix PF_KTHREAD vs to_kthread() race") munged
> into 5.4.135
>
> Never even seen a compiler, please tests.

Thanks a lot, Peter. With the addition of a little semicolon in one
place, I successfully built 5.4.143 with the patch applied, in both my
host and vm configs.

I now have two machines with identical workloads (webserver, database)
running, one with unpatched 5.4.143, the other with 5.4.143 and that
patch. Expectation is to see the WARNING on the former, but not on the
latter. Will report in a day or three about the outcome.

regards
  Patrick

Tested-By: Patrick Schaaf <bof@bof.de>

diff --git a/kernel/kthread.c b/kernel/kthread.c
index b2bac5d929d2..22750a8af83e 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -76,6 +76,25 @@ static inline struct kthread *to_kthread(struct
task_struct *k)
        return (__force void *)k->set_child_tid;
 }

+/*
+ * Variant of to_kthread() that doesn't assume @p is a kthread.
+ *
+ * Per construction; when:
+ *
+ *   (p->flags & PF_KTHREAD) && p->set_child_tid
+ *
+ * the task is both a kthread and struct kthread is persistent. However
+ * PF_KTHREAD on it's own is not, kernel_thread() can exec() (See umh.c and
+ * begin_new_exec()).
+ */
+static inline struct kthread *__to_kthread(struct task_struct *p)
+{
+       void *kthread = (__force void *)p->set_child_tid;
+       if (kthread && !(p->flags & PF_KTHREAD))
+               kthread = NULL;
+       return kthread;
+}
+
 void free_kthread_struct(struct task_struct *k)
 {
        struct kthread *kthread;
@@ -176,10 +195,11 @@ void *kthread_data(struct task_struct *task)
  */
 void *kthread_probe_data(struct task_struct *task)
 {
-       struct kthread *kthread = to_kthread(task);
+       struct kthread *kthread = __to_kthread(task);
        void *data = NULL;

-       probe_kernel_read(&data, &kthread->data, sizeof(data));
+       if (kthread)
+               probe_kernel_read(&data, &kthread->data, sizeof(data));
        return data;
 }

@@ -490,9 +510,9 @@ void kthread_set_per_cpu(struct task_struct *k, int cpu)
        set_bit(KTHREAD_IS_PER_CPU, &kthread->flags);
 }

-bool kthread_is_per_cpu(struct task_struct *k)
+bool kthread_is_per_cpu(struct task_struct *p)
 {
-       struct kthread *kthread = to_kthread(k);
+       struct kthread *kthread = __to_kthread(p);
        if (!kthread)
                return false;

@@ -1272,11 +1292,9 @@ EXPORT_SYMBOL(kthread_destroy_worker);
  */
 void kthread_associate_blkcg(struct cgroup_subsys_state *css)
 {
-       struct kthread *kthread;
+       struct kthread *kthread = __to_kthread(current);
+

-       if (!(current->flags & PF_KTHREAD))
-               return;
-       kthread = to_kthread(current);
        if (!kthread)
                return;

@@ -1298,13 +1316,10 @@ EXPORT_SYMBOL(kthread_associate_blkcg);
  */
 struct cgroup_subsys_state *kthread_blkcg(void)
 {
-       struct kthread *kthread;
+       struct kthread *kthread = __to_kthread(current);

-       if (current->flags & PF_KTHREAD) {
-               kthread = to_kthread(current);
-               if (kthread)
-                       return kthread->blkcg_css;
-       }
+       if (kthread)
+               return kthread->blkcg_css;
        return NULL;
 }
 EXPORT_SYMBOL(kthread_blkcg);
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 74cb20f32f72..87d9fad9d01d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7301,7 +7301,7 @@ int can_migrate_task(struct task_struct *p,
struct lb_env *env)
                return 0;

        /* Disregard pcpu kthreads; they are where they need to be. */
-       if ((p->flags & PF_KTHREAD) && kthread_is_per_cpu(p))
+       if (kthread_is_per_cpu(p))
                return 0;

        if (!cpumask_test_cpu(env->dst_cpu, p->cpus_ptr)) {