padata: Put CPU offline callback in ONLINE section to allow failure

syzbot reported the following warning:

    DEAD callback error for CPU1
    WARNING: kernel/cpu.c:1463 at _cpu_down+0x759/0x1020 kernel/cpu.c:1463, CPU#0: syz.0.1960/14614

at commit 4ae12d8bd9a8 ("Merge tag 'kbuild-fixes-7.0-2' of git://git.kernel.org/pub/scm/linux/kernel/git/kbuild/linux")
which tglx traced to padata_cpu_dead() given it's the only
sub-CPUHP_TEARDOWN_CPU callback that returns an error.

Failure isn't allowed in hotplug states before CPUHP_TEARDOWN_CPU
so move the CPU offline callback to the ONLINE section where failure is
possible.

Fixes: 894c9ef9780c ("padata: validate cpumask without removed CPU during offline")
Reported-by: syzbot+123e1b70473ce213f3af@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/69af0a05.050a0220.310d8.002f.GAE@google.com/
Debugged-by: Thomas Gleixner <tglx@kernel.org>
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 62cd7b35a29c94f214fec3b55f9462eceffad694..22ba327ec2278c132572950848ade2b814787eb5 100644 (file)
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -92,7 +92,6 @@ enum cpuhp_state {
        CPUHP_NET_DEV_DEAD,
        CPUHP_IOMMU_IOVA_DEAD,
        CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
-       CPUHP_PADATA_DEAD,
        CPUHP_AP_DTPM_CPU_DEAD,
        CPUHP_RANDOM_PREPARE,
        CPUHP_WORKQUEUE_PREP,

diff --git a/include/linux/padata.h b/include/linux/padata.h
index 765f2778e264a5dbd624b24184844dd4db12fd95..b6232bea6edf5972331701e3a28f798f72fbe193 100644 (file)
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -149,23 +149,23 @@ struct padata_mt_job {
 /**
  * struct padata_instance - The overall control structure.
  *
- * @cpu_online_node: Linkage for CPU online callback.
- * @cpu_dead_node: Linkage for CPU offline callback.
+ * @cpuhp_node: Linkage for CPU hotplug callbacks.
  * @parallel_wq: The workqueue used for parallel work.
  * @serial_wq: The workqueue used for serial work.
  * @pslist: List of padata_shell objects attached to this instance.
  * @cpumask: User supplied cpumasks for parallel and serial works.
+ * @validate_cpumask: Internal cpumask used to validate @cpumask during hotplug.
  * @kobj: padata instance kernel object.
  * @lock: padata instance lock.
  * @flags: padata flags.
  */
 struct padata_instance {
-       struct hlist_node               cpu_online_node;
-       struct hlist_node               cpu_dead_node;
+       struct hlist_node               cpuhp_node;
        struct workqueue_struct         *parallel_wq;
        struct workqueue_struct         *serial_wq;
        struct list_head                pslist;
        struct padata_cpumask           cpumask;
+       cpumask_var_t                   validate_cpumask;
        struct kobject                   kobj;
        struct mutex                     lock;
        u8                               flags;

diff --git a/kernel/padata.c b/kernel/padata.c
index 9e7cfa5ed55bcf35bed4f4c21d6e10df32b4bcf3..0d3ea1b68b1f703e487f2b8814f0499f172d1c49 100644 (file)
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -535,7 +535,8 @@ static void padata_init_reorder_list(struct parallel_data *pd)
 }
 
 /* Allocate and initialize the internal cpumask dependend resources. */
-static struct parallel_data *padata_alloc_pd(struct padata_shell *ps)
+static struct parallel_data *padata_alloc_pd(struct padata_shell *ps,
+                                            int offlining_cpu)
 {
        struct padata_instance *pinst = ps->pinst;
        struct parallel_data *pd;
@@ -561,6 +562,10 @@ static struct parallel_data *padata_alloc_pd(struct padata_shell *ps)
 
        cpumask_and(pd->cpumask.pcpu, pinst->cpumask.pcpu, cpu_online_mask);
        cpumask_and(pd->cpumask.cbcpu, pinst->cpumask.cbcpu, cpu_online_mask);
+       if (offlining_cpu >= 0) {
+               __cpumask_clear_cpu(offlining_cpu, pd->cpumask.pcpu);
+               __cpumask_clear_cpu(offlining_cpu, pd->cpumask.cbcpu);
+       }
 
        padata_init_reorder_list(pd);
        padata_init_squeues(pd);
@@ -607,11 +612,11 @@ static void __padata_stop(struct padata_instance *pinst)
 }
 
 /* Replace the internal control structure with a new one. */
-static int padata_replace_one(struct padata_shell *ps)
+static int padata_replace_one(struct padata_shell *ps, int offlining_cpu)
 {
        struct parallel_data *pd_new;
 
-       pd_new = padata_alloc_pd(ps);
+       pd_new = padata_alloc_pd(ps, offlining_cpu);
        if (!pd_new)
                return -ENOMEM;
 
@@ -621,7 +626,7 @@ static int padata_replace_one(struct padata_shell *ps)
        return 0;
 }
 
-static int padata_replace(struct padata_instance *pinst)
+static int padata_replace(struct padata_instance *pinst, int offlining_cpu)
 {
        struct padata_shell *ps;
        int err = 0;
@@ -629,7 +634,7 @@ static int padata_replace(struct padata_instance *pinst)
        pinst->flags |= PADATA_RESET;
 
        list_for_each_entry(ps, &pinst->pslist, list) {
-               err = padata_replace_one(ps);
+               err = padata_replace_one(ps, offlining_cpu);
                if (err)
                        break;
        }
@@ -646,9 +651,21 @@ static int padata_replace(struct padata_instance *pinst)
 
 /* If cpumask contains no active cpu, we mark the instance as invalid. */
 static bool padata_validate_cpumask(struct padata_instance *pinst,
-                                   const struct cpumask *cpumask)
+                                   const struct cpumask *cpumask,
+                                   int offlining_cpu)
 {
-       if (!cpumask_intersects(cpumask, cpu_online_mask)) {
+       cpumask_copy(pinst->validate_cpumask, cpu_online_mask);
+
+       /*
+        * @offlining_cpu is still in cpu_online_mask, so remove it here for
+        * validation.  Using a sub-CPUHP_TEARDOWN_CPU hotplug state where
+        * @offlining_cpu wouldn't be in the online mask doesn't work because
+        * padata_cpu_offline() can fail but such a state doesn't allow failure.
+        */
+       if (offlining_cpu >= 0)
+               __cpumask_clear_cpu(offlining_cpu, pinst->validate_cpumask);
+
+       if (!cpumask_intersects(cpumask, pinst->validate_cpumask)) {
                pinst->flags |= PADATA_INVALID;
                return false;
        }
@@ -664,13 +681,13 @@ static int __padata_set_cpumasks(struct padata_instance *pinst,
        int valid;
        int err;
 
-       valid = padata_validate_cpumask(pinst, pcpumask);
+       valid = padata_validate_cpumask(pinst, pcpumask, -1);
        if (!valid) {
                __padata_stop(pinst);
                goto out_replace;
        }
 
-       valid = padata_validate_cpumask(pinst, cbcpumask);
+       valid = padata_validate_cpumask(pinst, cbcpumask, -1);
        if (!valid)
                __padata_stop(pinst);
 
@@ -678,7 +695,7 @@ out_replace:
        cpumask_copy(pinst->cpumask.pcpu, pcpumask);
        cpumask_copy(pinst->cpumask.cbcpu, cbcpumask);
 
-       err = padata_setup_cpumasks(pinst) ?: padata_replace(pinst);
+       err = padata_setup_cpumasks(pinst) ?: padata_replace(pinst, -1);
 
        if (valid)
                __padata_start(pinst);
@@ -730,26 +747,6 @@ EXPORT_SYMBOL(padata_set_cpumask);
 
 #ifdef CONFIG_HOTPLUG_CPU
 
-static int __padata_add_cpu(struct padata_instance *pinst, int cpu)
-{
-       int err = padata_replace(pinst);
-
-       if (padata_validate_cpumask(pinst, pinst->cpumask.pcpu) &&
-           padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
-               __padata_start(pinst);
-
-       return err;
-}
-
-static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
-{
-       if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
-           !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
-               __padata_stop(pinst);
-
-       return padata_replace(pinst);
-}
-
 static inline int pinst_has_cpu(struct padata_instance *pinst, int cpu)
 {
        return cpumask_test_cpu(cpu, pinst->cpumask.pcpu) ||
@@ -761,27 +758,39 @@ static int padata_cpu_online(unsigned int cpu, struct hlist_node *node)
        struct padata_instance *pinst;
        int ret;
 
-       pinst = hlist_entry_safe(node, struct padata_instance, cpu_online_node);
+       pinst = hlist_entry_safe(node, struct padata_instance, cpuhp_node);
        if (!pinst_has_cpu(pinst, cpu))
                return 0;
 
        mutex_lock(&pinst->lock);
-       ret = __padata_add_cpu(pinst, cpu);
+
+       ret = padata_replace(pinst, -1);
+
+       if (padata_validate_cpumask(pinst, pinst->cpumask.pcpu, -1) &&
+           padata_validate_cpumask(pinst, pinst->cpumask.cbcpu, -1))
+               __padata_start(pinst);
+
        mutex_unlock(&pinst->lock);
        return ret;
 }
 
-static int padata_cpu_dead(unsigned int cpu, struct hlist_node *node)
+static int padata_cpu_offline(unsigned int cpu, struct hlist_node *node)
 {
        struct padata_instance *pinst;
        int ret;
 
-       pinst = hlist_entry_safe(node, struct padata_instance, cpu_dead_node);
+       pinst = hlist_entry_safe(node, struct padata_instance, cpuhp_node);
        if (!pinst_has_cpu(pinst, cpu))
                return 0;
 
        mutex_lock(&pinst->lock);
-       ret = __padata_remove_cpu(pinst, cpu);
+
+       if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu, cpu) ||
+           !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu, cpu))
+               __padata_stop(pinst);
+
+       ret = padata_replace(pinst, cpu);
+
        mutex_unlock(&pinst->lock);
        return ret;
 }
@@ -792,15 +801,14 @@ static enum cpuhp_state hp_online;
 static void __padata_free(struct padata_instance *pinst)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-       cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD,
-                                           &pinst->cpu_dead_node);
-       cpuhp_state_remove_instance_nocalls(hp_online, &pinst->cpu_online_node);
+       cpuhp_state_remove_instance_nocalls(hp_online, &pinst->cpuhp_node);
 #endif
 
        WARN_ON(!list_empty(&pinst->pslist));
 
        free_cpumask_var(pinst->cpumask.pcpu);
        free_cpumask_var(pinst->cpumask.cbcpu);
+       free_cpumask_var(pinst->validate_cpumask);
        destroy_workqueue(pinst->serial_wq);
        destroy_workqueue(pinst->parallel_wq);
        kfree(pinst);
@@ -961,10 +969,10 @@ struct padata_instance *padata_alloc(const char *name)
 
        if (!alloc_cpumask_var(&pinst->cpumask.pcpu, GFP_KERNEL))
                goto err_free_serial_wq;
-       if (!alloc_cpumask_var(&pinst->cpumask.cbcpu, GFP_KERNEL)) {
-               free_cpumask_var(pinst->cpumask.pcpu);
-               goto err_free_serial_wq;
-       }
+       if (!alloc_cpumask_var(&pinst->cpumask.cbcpu, GFP_KERNEL))
+               goto err_free_p_mask;
+       if (!alloc_cpumask_var(&pinst->validate_cpumask, GFP_KERNEL))
+               goto err_free_cb_mask;
 
        INIT_LIST_HEAD(&pinst->pslist);
 
@@ -972,7 +980,7 @@ struct padata_instance *padata_alloc(const char *name)
        cpumask_copy(pinst->cpumask.cbcpu, cpu_possible_mask);
 
        if (padata_setup_cpumasks(pinst))
-               goto err_free_masks;
+               goto err_free_v_mask;
 
        __padata_start(pinst);
 
@@ -981,18 +989,19 @@ struct padata_instance *padata_alloc(const char *name)
 
 #ifdef CONFIG_HOTPLUG_CPU
        cpuhp_state_add_instance_nocalls_cpuslocked(hp_online,
-                                                   &pinst->cpu_online_node);
-       cpuhp_state_add_instance_nocalls_cpuslocked(CPUHP_PADATA_DEAD,
-                                                   &pinst->cpu_dead_node);
+                                                   &pinst->cpuhp_node);
 #endif
 
        cpus_read_unlock();
 
        return pinst;
 
-err_free_masks:
-       free_cpumask_var(pinst->cpumask.pcpu);
+err_free_v_mask:
+       free_cpumask_var(pinst->validate_cpumask);
+err_free_cb_mask:
        free_cpumask_var(pinst->cpumask.cbcpu);
+err_free_p_mask:
+       free_cpumask_var(pinst->cpumask.pcpu);
 err_free_serial_wq:
        destroy_workqueue(pinst->serial_wq);
 err_put_cpus:
@@ -1035,7 +1044,7 @@ struct padata_shell *padata_alloc_shell(struct padata_instance *pinst)
        ps->pinst = pinst;
 
        cpus_read_lock();
-       pd = padata_alloc_pd(ps);
+       pd = padata_alloc_pd(ps, -1);
        cpus_read_unlock();
 
        if (!pd)
@@ -1084,31 +1093,24 @@ void __init padata_init(void)
        int ret;
 
        ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, "padata:online",
-                                     padata_cpu_online, NULL);
+                                     padata_cpu_online, padata_cpu_offline);
        if (ret < 0)
                goto err;
        hp_online = ret;
-
-       ret = cpuhp_setup_state_multi(CPUHP_PADATA_DEAD, "padata:dead",
-                                     NULL, padata_cpu_dead);
-       if (ret < 0)
-               goto remove_online_state;
 #endif
 
        possible_cpus = num_possible_cpus();
        padata_works = kmalloc_objs(struct padata_work, possible_cpus);
        if (!padata_works)
-               goto remove_dead_state;
+               goto remove_online_state;
 
        for (i = 0; i < possible_cpus; ++i)
                list_add(&padata_works[i].pw_list, &padata_free_works);
 
        return;
 
-remove_dead_state:
-#ifdef CONFIG_HOTPLUG_CPU
-       cpuhp_remove_multi_state(CPUHP_PADATA_DEAD);
 remove_online_state:
+#ifdef CONFIG_HOTPLUG_CPU
        cpuhp_remove_multi_state(hp_online);
 err:
 #endif