drivers/base/memory.c: Don't access uninitialized memmaps in soft_offline_page_store()

PATCH Applied David Hildenbrand 4 participants 6 messages Oct 10, 2019 – Oct 14, 2019

Subsystems (3)

Memory management Driver core Memory hot(un)plug

lore

Share & tools

lore

David Hildenbrand

author

2019-10-10 14:12 UTC #0 lore raw

PATCH 1/1 drivers/base/memory.c: Don't access uninitialized memmaps in soft_offline_page_store()

Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.

Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:

  :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
  [   23.097167] soft offline: 0x150000 page already poisoned

But the actual result depends on the garbage in the memmap.

soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.

Add a check against pfn_to_online_page() and similarly return -EIO.

Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 drivers/base/memory.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/base/memory.c +3 -0

index 6bea4f3f8040..55907c27075b 100644
@@ -540,6 +540,9 @@ static ssize_t soft_offline_page_store(struct device *dev,
 	pfn >>= PAGE_SHIFT;
 	if (!pfn_valid(pfn))
 		return -ENXIO;
+	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
+	if (!pfn_to_online_page(pfn))
+		return -EIO;
 	ret = soft_offline_page(pfn_to_page(pfn), 0);
 	return ret == 0 ? count : ret;
 }

Signature · 1 line

2.21.0

Naoya Horiguchi

Acked-by on v1 1/1

2019-10-11 06:16 UTC #1 lore raw

Quoted 33 lines · David Hildenbrand

Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.
Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:
  :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
  [   23.097167] soft offline: 0x150000 page already poisoned
But the actual result depends on the garbage in the memmap.
soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.
Add a check against pfn_to_online_page() and similarly return -EIO.
Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319
Cc Greg Kroah-Hartman <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 drivers/base/memory.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/base/memory.c +0 -0
index 6bea4f3f8040..55907c27075b 100644

drivers/base/memory.c static ssize_t soft_offline_page_store(struct device *dev, · line 545

Quoted 6 lines

drivers/base/memory.c +3 -0

@@ -540,6 +540,9 @@ static ssize_t soft_offline_page_store(struct device *dev,
 	pfn >>= PAGE_SHIFT;
 	if (!pfn_valid(pfn))
 		return -ENXIO;
+	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
+	if (!pfn_to_online_page(pfn))
+		return -EIO;

Acked-by Naoya Horiguchi <[email protected]>

I think this check could be placed in soft_offline_page(), but that requires a few more unrelated lines of changes due to the mismatch on type of parameter between memory_failure() and soft_offline_page(), This is not your problem, and I plan to do some cleanup on related interfaces, so this patch is fine.

- Naoya Horiguchi

David Hildenbrand

author

2019-10-11 09:51 UTC #2 lore raw

Quoted 43 lines · Naoya Horiguchi

Quoted 33 lines · David Hildenbrand
Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.
Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:
   :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
   [   23.097167] soft offline: 0x150000 page already poisoned
But the actual result depends on the garbage in the memmap.
soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.
Add a check against pfn_to_online_page() and similarly return -EIO.
Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319
Cc Greg Kroah-Hartman <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
  drivers/base/memory.c | 3 +++
  1 file changed, 3 insertions(+)
drivers/base/memory.c +0 -0
index 6bea4f3f8040..55907c27075b 100644
drivers/base/memory.c static ssize_t soft_offline_page_store(struct device *dev, · line 545
Quoted 6 lines
drivers/base/memory.c +3 -0
@@ -540,6 +540,9 @@ static ssize_t soft_offline_page_store(struct device *dev,
  	pfn >>= PAGE_SHIFT;
  	if (!pfn_valid(pfn))
  		return -ENXIO;
+	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
+	if (!pfn_to_online_page(pfn))
+		return -EIO;
Acked-by Naoya Horiguchi <[email protected]>

Quoted 5 lines

I think this check could be placed in soft_offline_page(), but that requires a few more unrelated lines of changes due to the mismatch on type of parameter between memory_failure() and soft_offline_page(), This is not your problem, and I plan to do some cleanup on related interfaces, so this patch is fine.

Thanks,

well I think when you come via madvise(), you are always guaranteed to hold a reasonable page in your hands. Only when converting from arbitrary pfns, we have to watch out. But yeah, feel free to cc me on cleanups :)

Quoted 2 lines

- Naoya Horiguchi

Signature · 3 lines

Thanks,

David / dhildenb

Andrew Morton

maintainer

2019-10-11 22:16 UTC #3 lore raw

In reply to David Hildenbrand

Quoted 12 lines · On Thu, 10 Oct 2019 16:12:00 +0200 David Hildenbrand <[email protected]> wrote:

Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.
Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:
  :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
  [   23.097167] soft offline: 0x150000 page already poisoned
But the actual result depends on the garbage in the memmap.

Quoted 7 lines

soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.
Add a check against pfn_to_online_page() and similarly return -EIO.
Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319

Should this be cc:stable?

What is the relationship between this and some similar fixes in the series "mm/memory_hotplug: Shrink zones before removing memory", v6?

Should any of the patches in "mm/memory_hotplug: Shrink zones before removing memory", v6 be cc:stable?

David Hildenbrand

author

2019-10-14 08:30 UTC #4 lore raw

Quoted 18 lines · Andrew Morton

Quoted 16 lines · On Thu, 10 Oct 2019 16:12:00 +0200 David Hildenbrand <[email protected]> wrote:
Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.
Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:
   :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
   [   23.097167] soft offline: 0x150000 page already poisoned
But the actual result depends on the garbage in the memmap.
soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.

Quoted 5 lines

Quoted 3 lines
Add a check against pfn_to_online_page() and similarly return -EIO.
Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319
Should this be cc:stable?

I think yes, more on that below.

Quoted 3 lines

What is the relationship between this and some similar fixes in the series "mm/memory_hotplug: Shrink zones before removing memory", v6?

In general, they all have the same root cause. With f1dd2cd13c4b, we started to initialize the memmap when onlining memory, however, we at least zeroed it out when adding the memory. With d0dc12e86b319 we removed the zeroing, and added conditional poisoning instead.

All these BUGs can be reproduced by adding memory and keeping some memory blocks offline. Most distributions either online memory directly in the kernel when added or userspace onlines it via udev rules. s390x is special, because there we don't online memory blocks as default in user space. So on !s390x systems, these BUGs are quite hard to reproduce.

With "mm/memory_hotplug: Shrink zones before removing memory" these BUGs get easier to reproduce, because it is now sufficient to offline a memory block that was already onlined.

Also, devmem with "driver reserved memory" (for which part we don't initialize the memmap) is able to trigger these BUGs, but that feature is more recent AFAIK.

So, cc:stable, I am not sure if it applies to all patches. Some really only trigger when page poisoning is active, but don't result in any damage (as so far observed). We really produce damage in case we de-reference the NID/zone via the garbage memmap (and probably when doing a page_to_pfn(pfn_to_page(gargabe_page))).

But here, it is quite hard to tell what could happen, so I guess, if in doubt, better add cc:stable?

Quoted 4 lines

Should any of the patches in "mm/memory_hotplug: Shrink zones before removing memory", v6 be cc:stable?

I'll go over all patches and reply to the relevant ones.

So for this patch, please add:

Cc [email protected] # v4.13+

Signature · 3 lines

Thanks,

David / dhildenb

Michal Hocko

Acked-by on v1 1/1

2019-10-14 13:29 UTC #5 lore raw

In reply to David Hildenbrand

Quoted 16 lines · David Hildenbrand

Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING. They should not get touched.
Right now, when trying to soft-offline a PFN that resides on a memory block that was never onlined, one gets a misleading error with CONFIG_PAGE_POISONING:
  :/# echo 5637144576 > /sys/devices/system/memory/soft_offline_page
  [   23.097167] soft offline: 0x150000 page already poisoned
But the actual result depends on the garbage in the memmap.
soft_offline_page() can only work with online pages, it returns -EIO in case of ZONE_DEVICE. Make sure to only forward pages that are online (iow, managed by the buddy) and, therefore, have an initialized memmap.

Quoted 8 lines

Add a check against pfn_to_online_page() and similarly return -EIO.
Fixes f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded memory to zones until online") # visible after d0dc12e86b319
Cc Greg Kroah-Hartman <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Andrew Morton <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 20 lines

---
 drivers/base/memory.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/base/memory.c +3 -0

index 6bea4f3f8040..55907c27075b 100644
@@ -540,6 +540,9 @@ static ssize_t soft_offline_page_store(struct device *dev,
 	pfn >>= PAGE_SHIFT;
 	if (!pfn_valid(pfn))
 		return -ENXIO;
+	/* Only online pages can be soft-offlined (esp., not ZONE_DEVICE). */
+	if (!pfn_to_online_page(pfn))
+		return -EIO;
 	ret = soft_offline_page(pfn_to_page(pfn), 0);
 	return ret == 0 ? count : ret;
 }

Signature · 1 line

2.21.0

Signature · 2 lines

Michal Hocko
SUSE Labs