Re: [PATCH 4.19 091/195] padata: Remove broken queue flushing
From: Yang Yingliang
Date: Fri Feb 14 2020 - 05:22:01 EST
Hi,
On 2020/2/10 20:32, Greg Kroah-Hartman wrote:
From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
[ Upstream commit 07928d9bfc81640bab36f5190e8725894d93b659 ]
The function padata_flush_queues is fundamentally broken because
it cannot force padata users to complete the request that is
underway. IOW padata has to passively wait for the completion
of any outstanding work.
As it stands flushing is used in two places. Its use in padata_stop
is simply unnecessary because nothing depends on the queues to
be flushed afterwards.
The other use in padata_replace is more substantial as we depend
on it to free the old pd structure. This patch instead uses the
pd->refcnt to dynamically free the pd structure once all requests
are complete.
Fixes: 2b73b07ab8a4 ("padata: Flush the padata queues actively")
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---
kernel/padata.c | 46 ++++++++++++----------------------------------
1 file changed, 12 insertions(+), 34 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 6c06b3039faed..11c5f9c8779ea 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -35,6 +35,8 @@
#define MAX_OBJ_NUM 1000
+static void padata_free_pd(struct parallel_data *pd);
+
static int padata_index_to_cpu(struct parallel_data *pd, int cpu_index)
{
int cpu, target_cpu;
@@ -334,6 +336,7 @@ static void padata_serial_worker(struct work_struct *serial_work)
struct padata_serial_queue *squeue;
struct parallel_data *pd;
LIST_HEAD(local_list);
+ int cnt;
local_bh_disable();
squeue = container_of(serial_work, struct padata_serial_queue, work);
@@ -343,6 +346,8 @@ static void padata_serial_worker(struct work_struct *serial_work)
list_replace_init(&squeue->serial.list, &local_list);
spin_unlock(&squeue->serial.lock);
+ cnt = 0;
+
while (!list_empty(&local_list)) {
struct padata_priv *padata;
@@ -352,9 +357,12 @@ static void padata_serial_worker(struct work_struct *serial_work)
list_del_init(&padata->list);
padata->serial(padata);
- atomic_dec(&pd->refcnt);
+ cnt++;
}
local_bh_enable();
+
+ if (atomic_sub_and_test(cnt, &pd->refcnt))
+ padata_free_pd(pd);
}
/**
@@ -501,8 +509,7 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
timer_setup(&pd->timer, padata_reorder_timer, 0);
atomic_set(&pd->seq_nr, -1);
atomic_set(&pd->reorder_objects, 0);
- atomic_set(&pd->refcnt, 0);
- pd->pinst = pinst;
This patch remove this assignment, it's cause a null-ptr-deref when
using pd->pinst in padata_reorder().
[39135.886908] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000010
[39135.886909] Mem abort info:
[39135.886910] ESR = 0x96000004
[39135.886912] Exception class = DABT (current EL), IL = 32 bits
[39135.886913] SET = 0, FnV = 0
[39135.886913] EA = 0, S1PTW = 0
[39135.886914] Data abort info:
[39135.886915] ISV = 0, ISS = 0x00000004
[39135.886915] CM = 0, WnR = 0
[39135.886918] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000c66d7ef5
[39135.886919] [0000000000000010] pgd=0000000000000000
[39135.886922] Internal error: Oops: 96000004 [#1] SMP
[39135.897190] Modules linked in: authenc pcrypt crypto_user cfg80211
rfkill ib_isert iscsi_target_mod ib_srpt target_core_mod dm_mirror
dm_region_hash rpcrdma dm_log ib_srp scsi_transport_srp sunrpc dm_mod
ib_ipoib rdma_ucm ib_uverbs ib_umad ib_iser rdma_cm ib_cm iw_cm
aes_ce_blk crypto_simd cryptd aes_ce_cipher hns_roce_hw_v2 ghash_ce
sha2_ce sha256_arm64 hns_roce ib_core sha1_ce sbsa_gwdt hi_sfc sg mtd
ipmi_ssif sch_fq_codel ip_tables realtek hclge hinic hns3 ipmi_si
hisi_sas_v3_hw hibmc_drm hnae3 hisi_sas_main ipmi_devintf ipmi_msghandler
[39135.997870] Process kworker/0:2 (pid: 789, stack limit =
0x0000000047f55ba6)
[39136.012707] CPU: 0 PID: 789 Comm: kworker/0:2 Kdump: loaded Not
tainted 4.19.103+ #1
[39136.029010] Hardware name: Huawei TaiShan 2280 V2/BC82AMDD, BIOS 1.08
12/14/2019
[39136.044587] Workqueue: pencrypt padata_parallel_worker
[39136.055396] pstate: 00c00009 (nzcv daif +PAN +UAO)
[39136.065479] pc : padata_reorder+0x144/0x2e0
[39136.074274] lr : padata_reorder+0x124/0x2e0
[39136.083070] sp : ffff0000149cbc90
[39136.090036] x29: ffff0000149cbc90 x28: 0000000000000001
[39136.101215] x27: ffffa02fd14af080 x26: ffffa02fd5fe1600
[39136.112392] x25: ffff5df7bf4c0ac8 x24: ffffa02fd5fe1628
[39136.123569] x23: 0000000000000000 x22: ffff00000828a258
[39136.134747] x21: ffffa02fd5fe1618 x20: ffff000009a79788
[39136.145924] x19: ffff5df7bf4c0ac8 x18: 00000000bef9a3f7
[39136.157102] x17: 0000000066bb7710 x16: 000000009a34db62
[39136.168280] x15: 0000000000342311 x14: 0000000037c9c538
[39136.179458] x13: 00000000deb82818 x12: 000000007abb6477
[39136.190638] x11: 000000006e0b05e5 x10: 00000000ccde2d6a
[39136.201817] x9 : 0000000000000000 x8 : a544a826aa446d6a
[39136.212996] x7 : e5050b6ea3a6345c x6 : ffffa02fd74ef030
[39136.224174] x5 : 0000000000000010 x4 : ffff5df7bf4c0ac8
[39136.235354] x3 : ffff5df7bf4c0ad8 x2 : ffff5df7bf4c0ac8
[39136.246532] x1 : ffff5df7bf4c0ad8 x0 : 0000000000000000
[39136.257712] Call trace:
[39136.262851] padata_reorder+0x144/0x2e0
[39136.270922] padata_do_serial+0xc8/0x128
[39136.279177] pcrypt_aead_enc+0x60/0x70 [pcrypt]
[39136.288708] padata_parallel_worker+0xd8/0x138
[39136.298056] process_one_work+0x1bc/0x4b8
[39136.306489] worker_thread+0x164/0x580
[39136.314374] kthread+0x134/0x138
[39136.321163] ret_from_fork+0x10/0x18
[39136.328681] Code: f900033b 52800000 91004261 089ffc20 (f9400ae1)
[39136.341508] ---[ end trace fc1b4f00385f0fee ]---
[39136.351221] Kernel panic - not syncing: Fatal exception in interrupt
[39136.364591] SMP: stopping secondary CPUs
[39136.372863] Kernel Offset: disabled
[39138.438722] CPU features: 0x12,a2200a38
[39138.446797] Memory Limit: none
[39138.463025] Starting crashdump kernel...
[39138.471295] Bye!
+ atomic_set(&pd->refcnt, 1);
spin_lock_init(&pd->lock);
return pd;
@@ -526,31 +533,6 @@ static void padata_free_pd(struct parallel_data *pd)
kfree(pd);
}
-/* Flush all objects out of the padata queues. */
-static void padata_flush_queues(struct parallel_data *pd)
-{
- int cpu;
- struct padata_parallel_queue *pqueue;
- struct padata_serial_queue *squeue;
-
- for_each_cpu(cpu, pd->cpumask.pcpu) {
- pqueue = per_cpu_ptr(pd->pqueue, cpu);
- flush_work(&pqueue->work);
- }
-
- del_timer_sync(&pd->timer);
-
- if (atomic_read(&pd->reorder_objects))
- padata_reorder(pd);
-
- for_each_cpu(cpu, pd->cpumask.cbcpu) {
- squeue = per_cpu_ptr(pd->squeue, cpu);
- flush_work(&squeue->work);
- }
-
- BUG_ON(atomic_read(&pd->refcnt) != 0);
-}
-
static void __padata_start(struct padata_instance *pinst)
{
pinst->flags |= PADATA_INIT;
@@ -564,10 +546,6 @@ static void __padata_stop(struct padata_instance *pinst)
pinst->flags &= ~PADATA_INIT;
synchronize_rcu();
-
- get_online_cpus();
- padata_flush_queues(pinst->pd);
- put_online_cpus();
}
/* Replace the internal control structure with a new one. */
@@ -588,8 +566,8 @@ static void padata_replace(struct padata_instance *pinst,
if (!cpumask_equal(pd_old->cpumask.cbcpu, pd_new->cpumask.cbcpu))
notification_mask |= PADATA_CPU_SERIAL;
- padata_flush_queues(pd_old);
- padata_free_pd(pd_old);
+ if (atomic_dec_and_test(&pd_old->refcnt))
+ padata_free_pd(pd_old);
if (notification_mask)
blocking_notifier_call_chain(&pinst->cpumask_change_notifier,