On Sat, 27 Aug 2022 19:19:59 +0800 Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:
> The kswapd_run/stop() will set pgdat->kswapd to NULL, which
> could race with kswapd_is_running() in kcompactd(),
>
> kswapd_run/stop() kcompactd()
> kswapd_is_running()
> pgdat->kswapd // error or nomal ptr
> verify pgdat->kswapd
> // load non-NULL
> pgdat->kswapd
> pgdat->kswapd = NULL
> task_is_running(pgdat->kswapd)
> // Null pointer derefence
>
> The KASAN report the null-ptr-deref shown below,
>
> vmscan: Failed to start kswapd on node 0
> ...
> BUG: KASAN: null-ptr-deref in kcompactd+0x440/0x504
> Read of size 8 at addr 0000000000000024 by task kcompactd0/37
>
> CPU: 0 PID: 37 Comm: kcompactd0 Kdump: loaded Tainted: G OE 5.10.60 #1
> Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> Call trace:
> dump_backtrace+0x0/0x394
> show_stack+0x34/0x4c
> dump_stack+0x158/0x1e4
> __kasan_report+0x138/0x140
> kasan_report+0x44/0xdc
> __asan_load8+0x94/0xd0
> kcompactd+0x440/0x504
> kthread+0x1a4/0x1f0
> ret_from_fork+0x10/0x18
>
> For now, kswapd/kcompactd_run() and kswapd/kcompactd_stop() protected
> by mem_hotplug_begin/done(), but without kcompactd(). It is no need to
> involve memory hotplug lock in kcompactd(), so let's add new mutex to
> protect pgdat->kswapd accessed concurrently, also because kcompactd task
> will check the state of kswapd task, it's better to call kcompactd_stop()
> before kswapd_stop() to reduce lock conflicts.
>
Looks right to me.
I think the below will make the code a little more maintainable?
--- a/include/linux/memory_hotplug.h~mm-fix-null-ptr-deref-in-kswapd_is_running-fix
+++ a/include/linux/memory_hotplug.h
@@ -215,6 +215,7 @@ void put_online_mems(void);
void mem_hotplug_begin(void);
void mem_hotplug_done(void);
+/* See kswapd_is_running() */
static inline void pgdat_kswapd_lock(pg_data_t *pgdat)
{
mutex_lock(&pgdat->kswapd_lock);
--- a/mm/compaction.c~mm-fix-null-ptr-deref-in-kswapd_is_running-fix
+++ a/mm/compaction.c
@@ -1980,6 +1980,12 @@ static inline bool is_via_compact_memory
return order == -1;
}
+/*
+ * Determine whether kswapd is (or recently was!) running on this node.
+ *
+ * pgdat_kswapd_lock() pins pgdat->kswapd, so a concurrent kswapd_stop() can't
+ * zero it.
+ */
static bool kswapd_is_running(pg_data_t *pgdat)
{
bool running;
_
