Re: [PATCH] mm/memcg: fix NULL nodeinfo[] dereference on late-onlined nodes
Prakash Gupta <[email protected]>
| Newsgroups | org.kernel.vger.cgroups,org.kernel.vger.linux-arm-msm,org.kernel.vger.linux-kernel,org.kernel.vger.stable,org.kvack.linux-mm |
|---|---|
| Message-ID | <[email protected]> |
On 8/10/2026 1:28 PM, Muchun Song wrote: > > >> On Aug 10, 2026, at 13:46, Prakash Gupta <[email protected]> wrote: >> >> memcg->nodeinfo[] entries are allocated only for nodes present at >> css_alloc time. When a node is onlined after a memcg is created its >> nodeinfo[] slot remains NULL. Two call sites dereference these slots >> unconditionally: > > I don't think the premise of this patch is correct. > > memcg->nodeinfo[] is not allocated only for nodes that are present or > online at css_alloc time. mem_cgroup_alloc() allocates per-node info with > for_each_node(), and for_each_node() iterates N_POSSIBLE nodes: > > for_each_node(node) > alloc_mem_cgroup_per_node_info(memcg, node); > You are right. I rechecked and nodeinfo[] is allocated for all possible nodes at mem_cgroup_alloc() time, not just currently online ones. > If memcg->nodeinfo[nid] is NULL on your system, that looks like a > violation of this invariant, or possibly a downstream-specific change, > bad nid, allocation/lifetime issue, or memory corruption. I don't think > the generic explanation that "the node was onlined after the memcg was > created" is sufficient. > Agreed. I will investigate further before resubmitting that part. >> >> lruvec_stat_mod_folio() calls mem_cgroup_lruvec() which reads >> memcg->nodeinfo[pgdat->node_id] without a NULL check. On a system >> where a node is onlined after the memcg is created, any folio stat >> update for that node crashes with a NULL pointer dereference: >> >> Unable to handle kernel paging request at virtual address ffffffbebf7e1908 >> pc : lruvec_stat_mod_folio+0xf0/0x444 [6.18.21-android17-5] >> lr : lruvec_stat_mod_folio+0x88/0x444 >> Call trace: >> lruvec_stat_mod_folio+0xf0/0x444 >> folio_add_new_anon_rmap+0xac/0x2b8 >> do_wp_page+0x768/0xc80 >> handle_mm_fault+0x37c/0x8c4 >> do_page_fault+0x140/0xa1c >> do_mem_abort+0x54/0x74 >> el0_da+0x48/0x8c >> el0t_64_sync_handler+0x20/0x130 >> el0t_64_sync+0x1c4/0x1c8 >> >> __invalidate_reclaim_iterators() iterates for_each_node() and reads >> from->nodeinfo[nid]->iter without checking for NULL. for_each_node() >> visits all possible nodes, so this is reachable whenever a node is >> onlined after the memcg was created. >> >> Fix lruvec_stat_mod_folio() by checking nodeinfo[pgdat->node_id] >> directly and falling back to mod_node_page_state() when NULL, mirroring >> the existing !memcg early-return path. The fallback must not go through >> mod_lruvec_state() since that calls mod_memcg_lruvec_state() which uses >> container_of() to recover the mem_cgroup_per_node from the lruvec >> pointer; passing &pgdat->__lruvec there produces a garbage pointer. >> When nodeinfo[nid] is NULL the memcg has no per-node accounting >> structure for that node, so node-level accounting is correct. >> >> Fix __invalidate_reclaim_iterators() by skipping NULL nodeinfo[] slots. >> >> Also switch lruvec_stat_mod_folio() to folio_memcg_check() which uses >> READ_ONCE() to safely read folio->memcg_data in an unlocked context. >> >> Fixes: 6c77b607ee26 ("mm: kill lock|unlock_page_memcg()") > > The Fixes tag also seems odd. 6c77b607ee26 ("mm: kill > lock|unlock_page_memcg()") only removed/renamed the lock_page_memcg() > wrappers and does not appear to change nodeinfo[] allocation or memory > hotplug handling. Could you explain how that commit introduced the NULL > nodeinfo condition? > It did not. It seems I picked up the commit based on git blame on function as there were two related bugs reports that I was conflating into one patch as listed below, will fix that in v2. Variant A (missed to mentioned in commit msg) — NULL dereference: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000528 ESR = 0x0000000096000005 (read fault) Workqueue: events delayed_fput pc : lruvec_stat_mod_folio+0x5c/0x444 [6.18.21-android17-5] lr : lruvec_stat_mod_folio+0x2c/0x444 As you suggested this may need more investigation. Variant B (the crash included in the patch) — stale pointer: Unable to handle kernel paging request at virtual address ffffffbebf7e1908 ESR = 0x0000000096000045 (write fault) pc : __lruvec_stat_mod_folio+0xf0/0x444 [6.18.21-android17-5] lr : __lruvec_stat_mod_folio+0x88/0x444 Call trace: __lruvec_stat_mod_folio+0xf0/0x444 folio_add_new_anon_rmap+0xac/0x2b8 do_wp_page+0x768/0xc80 handle_mm_fault+0x37c/0x8c4 do_page_fault+0x140/0xa1c __lruvec_stat_mod_folio+0xf0 places it after the !memcg NULL check.This is consistent with folio_memcg() returning a stale memcg pointer that passes the NULL check. folio_memcg_check() uses READ_ONCE(folio->memcg_data) which seems the correct API for unlocked contexts. folio_memcg_check() has been available since becacb04fdd4 ("mm: memcg: add folio_memcg_check()") but lruvec_stat_mod_folio() was never updated to use it. I should also note that this crash is difficult to reproduce in a controlled environment — the analysis is based on crashdump inspection. I have not been able to construct a reliable reproducer so far. let me know your thoughts on this, accordingly I can send a v2 to cover only Variant B fix as explained above. >> Cc: [email protected] >> Assisted-by: pi:claude-sonnet-4-5 > > Given the Assisted-by tag, I assume some of the analysis may have been > tool-assisted. That's fine, but the author still needs to validate the > reasoning against the actual code before submission. > Agree, I will be more careful in next submission. > Did you confirm that the relevant allocation and hotplug paths were > manually checked against the affected tree? In particular, I wonder > whether this behavior depends on downstream changes around > mem_cgroup_alloc(), for_each_node(), node_possible_map setup, or memory > hotplug nid validation. > I checked all four paths against the affected tree (6.18.21-android17-5). Only difference in mem_cgroup_alloc() are cosmetic and do not touch the nodeinfo[] allocation path. The NULL nodeinfo[nid] in Variant A is not explained by any downstream change — the root cause remain unknown. Thank you for the review. Thanks, Prakash