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

[Kefeng Wang]

On 2022/8/28 6:08, Andrew Morton wrote:
> 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?
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;
> _
>
> .