Re: [PATCH -next v2] mm/hotplug: fix a null-ptr-deref during NUMA boot

[Michal Hocko]

On Mon 13-05-19 11:20:46, Qian Cai wrote:
> On Mon, 2019-05-13 at 16:04 +0200, Michal Hocko wrote:
> > On Mon 13-05-19 09:43:59, Qian Cai wrote:
> > > On Mon, 2019-05-13 at 14:41 +0200, Michal Hocko wrote:
> > > > On Sun 12-05-19 01:48:29, Qian Cai wrote:
> > > > > The linux-next commit ("x86, numa: always initialize all possible
> > > > > nodes") introduced a crash below during boot for systems with a
> > > > > memory-less node. This is due to CPUs that get onlined during SMP boot,
> > > > > but that onlining triggers a page fault in bus_add_device() during
> > > > > device registration:
> > > > > 
> > > > > 	error = sysfs_create_link(&bus->p->devices_kset->kobj,
> > > > > 
> > > > > bus->p is NULL. That "p" is the subsys_private struct, and it should
> > > > > have been set in,
> > > > > 
> > > > > 	postcore_initcall(register_node_type);
> > > > > 
> > > > > but that happens in do_basic_setup() after smp_init().
> > > > > 
> > > > > The old code had set this node online via alloc_node_data(), so when it
> > > > > came time to do_cpu_up() -> try_online_node(), the node was already up
> > > > > and nothing happened.
> > > > > 
> > > > > Now, it attempts to online the node, which registers the node with
> > > > > sysfs, but that can't happen before the 'node' subsystem is registered.
> > > > > 
> > > > > Since kernel_init() is running by a kernel thread that is in
> > > > > SYSTEM_SCHEDULING state, fixed this by skipping registering with sysfs
> > > > > during the early boot in __try_online_node().
> > > > 
> > > > Relying on SYSTEM_SCHEDULING looks really hackish. Why cannot we simply
> > > > drop try_online_node from do_cpu_up? Your v2 remark below suggests that
> > > > we need to call node_set_online because something later on depends on
> > > > that. Btw. why do we even allocate a pgdat from this path? This looks
> > > > really messy.
> > > 
> > > See the commit cf23422b9d76 ("cpu/mem hotplug: enable CPUs online before
> > > local
> > > memory online")
> > > 
> > > It looks like try_online_node() in do_cpu_up() is needed for memory hotplug
> > > which is to put its node online if offlined and then hotadd_new_pgdat()
> > > calls
> > > build_all_zonelists() to initialize the zone list.
> > 
> > Well, do we still have to followthe logic that the above (unreviewed)
> > commit has established? The hotplug code in general made a lot of ad-hoc
> > design decisions which had to be revisited over time. If we are not
> > allocating pgdats for newly added memory then we should really make sure
> > to do so at a proper time and hook. I am not sure about CPU vs. memory
> > init ordering but even then I would really prefer if we could make the
> > init less obscure and _documented_.
> 
> I don't know, but I think it is a good idea to keep the existing logic rather
> than do a big surgery

Adding more hacks just doesn't make the situation any better.

> unless someone is able to confirm it is not breaking NUMA
> node physical hotplug.

I have a machine to test whole node offline. I am just busy to prepare a
patch myself. I can have it tested though.

-- 
Michal Hocko
SUSE Labs