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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1557760846.6132.25.camel@lca.pw>
Date:   Mon, 13 May 2019 11:20:46 -0400
From:   Qian Cai <cai@....pw>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     akpm@...ux-foundation.org, brho@...gle.com, kernelfans@...il.com,
        dave.hansen@...el.com, rppt@...ux.ibm.com, peterz@...radead.org,
        mpe@...erman.id.au, mingo@...e.hu, osalvador@...e.de,
        luto@...nel.org, tglx@...utronix.de, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next v2] mm/hotplug: fix a null-ptr-deref during NUMA
 boot

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 unless someone is able to confirm it is not breaking NUMA
node physical hotplug.