Messages in this thread Patch in this message | | | Date | Tue, 23 Aug 2022 22:47:33 +0800 | Subject | Re: [PATCH] mm: fix pgdat->kswap accessed concurrently | From | Kefeng Wang <> |
| |
On 2022/8/23 9:07, Kefeng Wang wrote: > > On 2022/8/21 4:59, Andrew Morton wrote: >> On Sat, 20 Aug 2022 15:33:04 +0800 Muchun Song >> <muchun.song@linux.dev> wrote: >> >>> >>>> + if (IS_ERR(t)) { >>>> /* failure at boot is fatal */ >>>> BUG_ON(system_state < SYSTEM_RUNNING); >>>> pr_err("Failed to start kswapd on node %d\n", nid); >>>> - pgdat->kswapd = NULL; >>>> + WRITE_ONCE(pgdat->kswapd, NULL); >>>> + } else { >>>> + WRITE_ONCE(pgdat->kswapd, t); >>>> } >>>> } >>> IIUC, the race is like the followings: >>> >>> CPU 0: CPU 1: >>> >>> kswapd_run() >>> pgdat->kswapd = kthread_run() >>> if (IS_ERR(pgdat->kswapd)) >>> kswapd_is_running >>> // load pgdat->kswapd and it is NOT NULL. >>> pgdat->kswapd = NULL >>> task_is_running(pgdat->kswapd); // NULL >>> pointer dereference >>> >> But don't we still have a bug? Sure, kswapd_is_running() will no >> longer deref a null pointer. But it now runs kswapd_is_running() >> against a task which has exited - a use-after-free?
The UAF is caused by race between kswapd_stop() and kcompactd(), right?
so kcompactd() should be stop before kswapd_stop() to avoid the above UAF.
$ git diff diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index fad6d1f2262a..2fd45ccbce45 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1940,8 +1940,8 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
node_states_clear_node(node, &arg); if (arg.status_change_nid >= 0) { - kswapd_stop(node); kcompactd_stop(node); + kswapd_stop(node); }
writeback_set_ratelimit(); > we could add get/put_task_struct() to avoid the UAF, will update, > thanks.
sorry, the task refcount won't fix anything.
> .
| |