• Oscar Salvador's avatar
    arch/x86/mm/numa: Do not initialize nodes twice · 1ca75fa7
    Oscar Salvador authored
    On x86, prior to ("mm: handle uninitialized numa nodes gracecully"), NUMA
    nodes could be allocated at three different places.
    
     - numa_register_memblks
     - init_cpu_to_node
     - init_gi_nodes
    
    All these calls happen at setup_arch, and have the following order:
    
    setup_arch
      ...
      x86_numa_init
       numa_init
        numa_register_memblks
      ...
      init_cpu_to_node
       init_memory_less_node
        alloc_node_data
        free_area_init_memoryless_node
      init_gi_nodes
       init_memory_less_node
        alloc_node_data
        free_area_init_memoryless_node
    
    numa_register_memblks() is only interested in those nodes which have
    memory, so it skips over any memoryless node it founds.  Later on, when
    we have read ACPI's SRAT table, we call init_cpu_to_node() and
    init_gi_nodes(), which initialize any memoryless node we might have that
    have either CPU or Initiator affinity, meaning we allocate pg_data_t
    struct for them and we mark them as ONLINE.
    
    So far so good, but the thing is that after ("mm: handle uninitialized
    numa nodes gracefully"), we allocate all possible NUMA nodes in
    free_area_init(), meaning we have a picture like the following:
    
    setup_arch
      x86_numa_init
       numa_init
        numa_register_memblks  <-- allocate non-memoryless node
      x86_init.paging.pagetable_init
       ...
        free_area_init
         free_area_init_memoryless <-- allocate memoryless node
      init_cpu_to_node
       alloc_node_data             <-- allocate memoryless node with CPU
       free_area_init_memoryless_node
      init_gi_nodes
       alloc_node_data             <-- allocate memoryless node with Initiator
       free_area_init_memoryless_node
    
    free_area_init() already allocates all possible NUMA nodes, but
    init_cpu_to_node() and init_gi_nodes() are clueless about that, so they
    go ahead and allocate a new pg_data_t struct without checking anything,
    meaning we end up allocating twice.
    
    It should be mad clear that this only happens in the case where
    memoryless NUMA node happens to have a CPU/Initiator affinity.
    
    So get rid of init_memory_less_node() and just set the node online.
    
    Note that setting the node online is needed, otherwise we choke down the
    chain when bringup_nonboot_cpus() ends up calling
    __try_online_node()->register_one_node()->...  and we blow up in
    bus_add_device().  As can be seen here:
    
      BUG: kernel NULL pointer dereference, address: 0000000000000060
      #PF: supervisor read access in kernel mode
      #PF: error_code(0x0000) - not-present page
      PGD 0 P4D 0
      Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI
      CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.0-rc4-1-default+ #45
      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/4
      RIP: 0010:bus_add_device+0x5a/0x140
      Code: 8b 74 24 20 48 89 df e8 84 96 ff ff 85 c0 89 c5 75 38 48 8b 53 50 48 85 d2 0f 84 bb 00 004
      RSP: 0000:ffffc9000022bd10 EFLAGS: 00010246
      RAX: 0000000000000000 RBX: ffff888100987400 RCX: ffff8881003e4e19
      RDX: ffff8881009a5e00 RSI: ffff888100987400 RDI: ffff888100987400
      RBP: 0000000000000000 R08: ffff8881003e4e18 R09: ffff8881003e4c98
      R10: 0000000000000000 R11: ffff888100402bc0 R12: ffffffff822ceba0
      R13: 0000000000000000 R14: ffff888100987400 R15: 0000000000000000
      FS:  0000000000000000(0000) GS:ffff88853fc00000(0000) knlGS:0000000000000000
      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
      CR2: 0000000000000060 CR3: 000000000200a001 CR4: 00000000001706b0
      Call Trace:
       device_add+0x4c0/0x910
       __register_one_node+0x97/0x2d0
       __try_online_node+0x85/0xc0
       try_online_node+0x25/0x40
       cpu_up+0x4f/0x100
       bringup_nonboot_cpus+0x4f/0x60
       smp_init+0x26/0x79
       kernel_init_freeable+0x130/0x2f1
       kernel_init+0x17/0x150
       ret_from_fork+0x22/0x30
    
    The reason is simple, by the time bringup_nonboot_cpus() gets called, we
    did not register the node_subsys bus yet, so we crash when
    bus_add_device() tries to dereference bus()->p.
    
    The following shows the order of the calls:
    
    kernel_init_freeable
     smp_init
      bringup_nonboot_cpus
       ...
         bus_add_device()      <- we did not register node_subsys yet
     do_basic_setup
      do_initcalls
       postcore_initcall(register_node_type);
        register_node_type
         subsys_system_register
          subsys_register
           bus_register         <- register node_subsys bus
    
    Why setting the node online saves us then? Well, simply because
    __try_online_node() backs off when the node is online, meaning we do not
    end up calling register_one_node() in the first place.
    
    This is subtle, broken and deserves a deep analysis and thought about
    how to put this into shape, but for now let us have this easy fix for
    the leaking memory issue.
    
    [osalvador@suse.de: add comments]
      Link: https://lkml.kernel.org/r/20220221142649.3457-1-osalvador@suse.de
    
    Link: https://lkml.kernel.org/r/20220218224302.5282-2-osalvador@suse.de
    Fixes: da4490c958ad ("mm: handle uninitialized numa nodes gracefully")
    Signed-off-by: default avatarOscar Salvador <osalvador@suse.de>
    Acked-by: default avatarMichal Hocko <mhocko@suse.com>
    Cc: David Hildenbrand <david@redhat.com>
    Cc: Rafael Aquini <raquini@redhat.com>
    Cc: Dave Hansen <dave.hansen@linux.intel.com>
    Cc: Wei Yang <richard.weiyang@gmail.com>
    Cc: Dennis Zhou <dennis@kernel.org>
    Cc: Alexey Makhalov <amakhalov@vmware.com>
    Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
    Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
    1ca75fa7
page_alloc.c 264 KB