Re: [PATCH v2 0/6] mm/memory_hotplug: Consider all zones when removing memory

[David Hildenbrand <david@redhat.com>]

On 26.08.19 16:53, Aneesh Kumar K.V wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> Working on virtio-mem, I was able to trigger a kernel BUG (with debug
>> options enabled) when removing memory that was never onlined. I was able
>> to reproduce with DIMMs. As far as I can see the same can also happen
>> without debug configs enabled, if we're unlucky and the uninitialized
>> memmap contains selected garbage .
>>
>> The root problem is that we should not try to derive the zone of memory we
>> are removing from the first PFN. The individual memory blocks of a DIMM
>> could be spanned by different ZONEs, multiple ZONES (after being offline and
>> re-onlined) or no ZONE at all (never onlined).
>>
>> Let's process all applicable zones when removing memory so we're on the
>> safe side. In the long term, we want to resize the zones when offlining
>> memory (and before removing ZONE_DEVICE memory), however, that will require
>> more thought (and most probably a new SECTION_ACTIVE / pfn_active()
>> thingy). More details about that in patch #3.
>>
>> Along with the fix, some related cleanups.
>>
>> v1 -> v2:
>> - Include "mm: Introduce for_each_zone_nid()"
>> - "mm/memory_hotplug: Pass nid instead of zone to __remove_pages()"
>> -- Pass the nid instead of the zone and use it to reduce the number of
>>    zones to process
>>
>> --- snip ---
>>
>> I gave this a quick test with a DIMM on x86-64:
>>
>> Start with a NUMA-less node 1. Hotplug a DIMM (512MB) to Node 1.
>> 1st memory block is not onlined. 2nd and 4th is onlined MOVABLE.
>> 3rd is onlined NORMAL.
>>
>> :/# echo "online_movable" > /sys/devices/system/memory/memory41/state
>> [...]
>> :/# echo "online_movable" > /sys/devices/system/memory/memory43/state
>> :/# echo "online_kernel" > /sys/devices/system/memory/memory42/state
>> :/# cat /sys/devices/system/memory/memory40/state
>> offline
>>
>> :/# cat /proc/zoneinfo
>> Node 1, zone   Normal
>>  [...]
>>         spanned  32768
>>         present  32768
>>         managed  32768
>>  [...]
>> Node 1, zone  Movable
>>  [...]
>>         spanned  98304
>>         present  65536
>>         managed  65536
>>  [...]
>>
>> Trigger hotunplug. If it succeeds (block 42 can be offlined):
>>
>> :/# cat /proc/zoneinfo
>>
>> Node 1, zone   Normal
>>   pages free     0
>>         min      0
>>         low      0
>>         high     0
>>         spanned  0
>>         present  0
>>         managed  0
>>         protection: (0, 0, 0, 0, 0)
>> Node 1, zone  Movable
>>   pages free     0
>>         min      0
>>         low      0
>>         high     0
>>         spanned  0
>>         present  0
>>         managed  0
>>         protection: (0, 0, 0, 0, 0)
>>
>> So all zones were properly fixed up and we don't access the memmap of the
>> first, never-onlined memory block (garbage). I am no longer able to trigger
>> the BUG. I did a similar test with an already populated node.
>>
> 
> I did report a variant of the issue at
> 
> https://lore.kernel.org/linux-mm/20190514025354.9108-1-aneesh.kumar@linux.ibm.com/
> 
> This patch series still doesn't handle the fact that struct page backing
> the start_pfn might not be initialized. ie, it results in crash like
> below

Okay, that's a related but different issue I think.

I can see that current shrink_zone_span() might read-access the
uninitialized struct page of a PFN if

1. The zone has holes and we check for "zone all holes". If we get
pfn_valid(pfn), we check if "page_zone(pfn_to_page(pfn)) != zone".

2. Via find_smallest_section_pfn() / find_biggest_section_pfn() find a
spanned pfn_valid(). We check
- pfn_to_nid(start_pfn) != nid
- zone != page_zone(pfn_to_page(start_pfn)

So we don't actually use the zone/nid, only use it to sanity check. That
might result in false-positives (not that bad).

It all boils down to shrink_zone_span() not working only on active
memory, for which the PFN is not only valid but also initialized
(something for which we need a new section flag I assume).

Which access triggers the issue you describe? pfn_to_nid()?

> 
>     pc: c0000000004bc1ec: shrink_zone_span+0x1bc/0x290
>     lr: c0000000004bc1e8: shrink_zone_span+0x1b8/0x290
>     sp: c0000000dac7f910
>    msr: 800000000282b033
>   current = 0xc0000000da2fa000
>   paca    = 0xc00000000fffb300   irqmask: 0x03   irq_happened: 0x01
>     pid   = 1224, comm = ndctl
> kernel BUG at /home/kvaneesh/src/linux/include/linux/mm.h:1088!
> Linux version 5.3.0-rc6-17495-gc7727d815970-dirty (kvaneesh@ltc-boston123) (gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)) #183 SMP Mon Aug 26 09:37:32 CDT 2019
> enter ? for help

Which exact kernel BUG are you hitting here? (my tree doesn't seem t
have any BUG statement around  include/linux/mm.h:1088)

> [c0000000dac7f980] c0000000004bc574 __remove_zone+0x84/0xd0
> [c0000000dac7f9d0] c0000000004bc920 __remove_section+0x100/0x170
> [c0000000dac7fa30] c0000000004bec98 __remove_pages+0x168/0x220
> [c0000000dac7fa90] c00000000007dff8 arch_remove_memory+0x38/0x110
> [c0000000dac7fb00] c00000000050cb0c devm_memremap_pages_release+0x24c/0x2f0
> [c0000000dac7fb90] c000000000cfec00 devm_action_release+0x30/0x50
> [c0000000dac7fbb0] c000000000cffe7c release_nodes+0x24c/0x2c0
> [c0000000dac7fc20] c000000000cf8988 device_release_driver_internal+0x168/0x230
> [c0000000dac7fc60] c000000000cf5624 unbind_store+0x74/0x190
> [c0000000dac7fcb0] c000000000cf42a4 drv_attr_store+0x44/0x60
> [c0000000dac7fcd0] c000000000617d44 sysfs_kf_write+0x74/0xa0


-- 

Thanks,

David / dhildenb