Re: [PATCH v2] mm: fix null-ptr-deref in kswapd_is_running()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2022/8/28 6:08, Andrew Morton wrote:
> On Sat, 27 Aug 2022 19:19:59 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> 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?
Thanks Andrew, it's better.
>
> --- 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;
> _
>
> .