Re: [PATCH] mm/memory_hotplug: Fix leftover use of struct page during hotplug

[Oscar Salvador <osalvador@techadventures.net>]

On Thu, May 10, 2018 at 02:02:00PM +0200, Michal Hocko wrote:
> On Fri 04-05-18 17:50:51, Jonathan Cameron wrote:
> [...]
> > Exact path to the problem is as follows:
> > 
> > mm/memory_hotplug.c : add_memory_resource
> > The node is not online so we enter the
> > if (new_node) twice, on the second such block there is a call to
> > link_mem_sections which calls into
> > drivers/node.c: link_mem_sections which calls
> > drivers/node.c: register_mem_sect_under_node which calls
> > get_nid_for_pfn and keeps trying until the output of that matches
> > the expected node (passed all the way down from add_memory_resource)
> 
> I am sorry but I am still confused. Why don't we create sysfs files from
> __add_pages
>   __add_section
>     hotplug_memory_register
>       register_mem_sect_under_node

IIUC the problem is that at the point we are calling register_mem_sect_under_node(),
pages are not initialized yet.

While walking the pfns in register_mem_sect_under_node(),
we might check for the node-id of the pfn if check_nid is true.

if (check_nid) {
	page_nid = get_nid_for_pfn(pfn);
	if (page_nid < 0)
		continue;
	if (page_nid != nid)
		continue;
}

I think the problem is in:

get_nid_for_pfn()->pfn_to_nid()->page_to_nid()

static inline int page_to_nid(const struct page *page)
{
	struct page *p = (struct page *)page;

	return (PF_POISONED_CHECK(p)->flags >> NODES_PGSHIFT) & NODES_MASK;
}

We access a field of the page, but these are not initialiazed, so it can
contain anything.
Because of that we can just get a wrong id, making the loop to not pass the
below check.

if (check_nid) {
        page_nid = get_nid_for_pfn(pfn);
        if (page_nid < 0)
                continue;
        if (page_nid != nid)
                continue;
}

create_sys_fs ...

and we do not carry on creating the sysfs.


Oscar Salvador