[SRU][X/B/D/E] [PATCH 0/1] PM / hibernate: fix potential memory corruption

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][X/B/D/E] [PATCH 0/1] PM / hibernate: fix potential memory corruption

Andrea Righi
BugLink: https://bugs.launchpad.net/bugs/1847118

[Impact]

A caching bug in the hibernation code can lead to potential memory
corruptions on resume.

The hibernation code is representing all the allocated pages in memory
(pfn) using a list of extents, inside each extent it uses a radix tree
and each node in the tree contains a bitmap. This structure is used to
save the memory image to disk.

To speed up lookups in this structure the kernel is caching the position
of the previous lookup in the form (current_extent, current_node).
However, if two consecutive lookups are distant enough from each other,
the extent can change, but the kernel can still use the cached node
(current_node), accessing the wrong bitmap and ending up saving to disk
the wrong pfn's.

[Test Case]

Bug has been reproduced in Xenial and Bionic trying to hibernate a large
instance with a lot of RAM (100GB+).

But we also wrote a custom kernel module to better isolate the code that
triggers the problem: https://code.launchpad.net/~arighi/+git/mybitmap

This module has exactly the same code as the hibernation code, but it
can be used as a fast test case to reproduce the problem without
actually triggering a real hibernation/resume cycle.

[Fix]

This bug can be fixed by properly invalidating the cached pair (extent,
node) when the next lookup falls in a different extent or a different
node.

[Regression Potential]

The fix has been sent to the LKML for review/feedback
(https://lkml.org/lkml/2019/9/25/393), we have not received any feedback
so far, but the bug is pretty clear and well tested on the affected
platforms. Moreover, the code is isolated to the hibernation area, so
the overall regression potential is minimal.

----------------------------------------------------------------
Andy Whitcroft (1):
      PM / hibernate: memory_bm_find_bit -- tighten node optimisation

 kernel/power/snapshot.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[X] [PATCH 1/1] UBUNTU: SAUCE: PM / hibernate: memory_bm_find_bit -- tighten node optimisation

Andrea Righi
From: Andy Whitcroft <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1847118

When looking for a bit by number we make use of the cached result from the
preceding lookup to speed up operation.  Firstly we check if the requested
pfn is within the cached zone and if not lookup the new zone.  We then
check if the offset for that pfn falls within the existing cached node.
This happens regardless of whether the node is within the zone we are
now scanning.  With certain memory layouts it is possible for this to
false trigger creating a temporary alias for the pfn to a different bit.
This leads the hibernation code to free memory which it was never allocated
with the expected fallout.

Ensure the zone we are scanning matches the cached zone before considering
the cached node.

Deep thanks go to Andrea for many, many, many hours of hacking and testing
that went into cornering this bug.

Reported-by: Andrea Righi <[hidden email]>
Tested-by: Andrea Righi <[hidden email]>
Signed-off-by: Andy Whitcroft <[hidden email]>
---
 kernel/power/snapshot.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index f155c62..334191d 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -662,8 +662,14 @@ zone_found:
  * node for our pfn.
  */
 
+ /*
+ * If the zone we wish to scan is the the current zone and the
+ * pfn falls into the current node then we do not need to walk
+ * the tree.
+ */
  node = bm->cur.node;
- if (((pfn - zone->start_pfn) & ~BM_BLOCK_MASK) == bm->cur.node_pfn)
+ if (zone == bm->cur.zone &&
+    ((pfn - zone->start_pfn) & ~BM_BLOCK_MASK) == bm->cur.node_pfn)
  goto node_found;
 
  node      = zone->rtree;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[B/D/E] [PATCH 1/1] UBUNTU: SAUCE: PM / hibernate: memory_bm_find_bit -- tighten node optimisation

Andrea Righi
In reply to this post by Andrea Righi
From: Andy Whitcroft <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1847118

When looking for a bit by number we make use of the cached result from the
preceding lookup to speed up operation.  Firstly we check if the requested
pfn is within the cached zone and if not lookup the new zone.  We then
check if the offset for that pfn falls within the existing cached node.
This happens regardless of whether the node is within the zone we are
now scanning.  With certain memory layouts it is possible for this to
false trigger creating a temporary alias for the pfn to a different bit.
This leads the hibernation code to free memory which it was never allocated
with the expected fallout.

Ensure the zone we are scanning matches the cached zone before considering
the cached node.

Deep thanks go to Andrea for many, many, many hours of hacking and testing
that went into cornering this bug.

Reported-by: Andrea Righi <[hidden email]>
Tested-by: Andrea Righi <[hidden email]>
Signed-off-by: Andy Whitcroft <[hidden email]>
---
 kernel/power/snapshot.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index bce0464..1ca1286 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -736,8 +736,15 @@ static int memory_bm_find_bit(struct memory_bitmap *bm, unsigned long pfn,
  * We have found the zone. Now walk the radix tree to find the leaf node
  * for our PFN.
  */
+
+ /*
+ * If the zone we wish to scan is the the current zone and the
+ * pfn falls into the current node then we do not need to walk
+ * the tree.
+ */
  node = bm->cur.node;
- if (((pfn - zone->start_pfn) & ~BM_BLOCK_MASK) == bm->cur.node_pfn)
+ if (zone == bm->cur.zone &&
+    ((pfn - zone->start_pfn) & ~BM_BLOCK_MASK) == bm->cur.node_pfn)
  goto node_found;
 
  node      = zone->rtree;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][X/B/D/E] [PATCH 0/1] PM / hibernate: fix potential memory corruption

Colin Ian King-2
In reply to this post by Andrea Righi
On 07/10/2019 17:35, Andrea Righi wrote:

> BugLink: https://bugs.launchpad.net/bugs/1847118
>
> [Impact]
>
> A caching bug in the hibernation code can lead to potential memory
> corruptions on resume.
>
> The hibernation code is representing all the allocated pages in memory
> (pfn) using a list of extents, inside each extent it uses a radix tree
> and each node in the tree contains a bitmap. This structure is used to
> save the memory image to disk.
>
> To speed up lookups in this structure the kernel is caching the position
> of the previous lookup in the form (current_extent, current_node).
> However, if two consecutive lookups are distant enough from each other,
> the extent can change, but the kernel can still use the cached node
> (current_node), accessing the wrong bitmap and ending up saving to disk
> the wrong pfn's.
>
> [Test Case]
>
> Bug has been reproduced in Xenial and Bionic trying to hibernate a large
> instance with a lot of RAM (100GB+).
>
> But we also wrote a custom kernel module to better isolate the code that
> triggers the problem: https://code.launchpad.net/~arighi/+git/mybitmap
>
> This module has exactly the same code as the hibernation code, but it
> can be used as a fast test case to reproduce the problem without
> actually triggering a real hibernation/resume cycle.
>
> [Fix]
>
> This bug can be fixed by properly invalidating the cached pair (extent,
> node) when the next lookup falls in a different extent or a different
> node.
>
> [Regression Potential]
>
> The fix has been sent to the LKML for review/feedback
> (https://lkml.org/lkml/2019/9/25/393), we have not received any feedback
> so far, but the bug is pretty clear and well tested on the affected
> platforms. Moreover, the code is isolated to the hibernation area, so
> the overall regression potential is minimal.
>
> ----------------------------------------------------------------
> Andy Whitcroft (1):
>       PM / hibernate: memory_bm_find_bit -- tighten node optimisation
>
>  kernel/power/snapshot.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
>

I've been following this issue, the fix looks good to me.  Perhaps we
need to ping upstream again on this before the fix gets lost.

Acked-by: Colin Ian King <[hidden email]>

Colin

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][X/B/D/E] [PATCH 0/1] PM / hibernate: fix potential memory corruption

Thadeu Lima de Souza Cascardo-3
In reply to this post by Andrea Righi
Limited to hibernation/resume path and tested.

Acked-by: Thadeu Lima de Souza Cascardo <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED/cmnt: [SRU][X/B/D/E] [PATCH 0/1] PM / hibernate: fix potential memory corruption

Kleber Souza
In reply to this post by Andrea Righi
On 07.10.19 18:35, Andrea Righi wrote:

> BugLink: https://bugs.launchpad.net/bugs/1847118
>
> [Impact]
>
> A caching bug in the hibernation code can lead to potential memory
> corruptions on resume.
>
> The hibernation code is representing all the allocated pages in memory
> (pfn) using a list of extents, inside each extent it uses a radix tree
> and each node in the tree contains a bitmap. This structure is used to
> save the memory image to disk.
>
> To speed up lookups in this structure the kernel is caching the position
> of the previous lookup in the form (current_extent, current_node).
> However, if two consecutive lookups are distant enough from each other,
> the extent can change, but the kernel can still use the cached node
> (current_node), accessing the wrong bitmap and ending up saving to disk
> the wrong pfn's.
>
> [Test Case]
>
> Bug has been reproduced in Xenial and Bionic trying to hibernate a large
> instance with a lot of RAM (100GB+).
>
> But we also wrote a custom kernel module to better isolate the code that
> triggers the problem: https://code.launchpad.net/~arighi/+git/mybitmap
>
> This module has exactly the same code as the hibernation code, but it
> can be used as a fast test case to reproduce the problem without
> actually triggering a real hibernation/resume cycle.
>
> [Fix]
>
> This bug can be fixed by properly invalidating the cached pair (extent,
> node) when the next lookup falls in a different extent or a different
> node.
>
> [Regression Potential]
>
> The fix has been sent to the LKML for review/feedback
> (https://lkml.org/lkml/2019/9/25/393), we have not received any feedback
> so far, but the bug is pretty clear and well tested on the affected
> platforms. Moreover, the code is isolated to the hibernation area, so
> the overall regression potential is minimal.
>
> ----------------------------------------------------------------
> Andy Whitcroft (1):
>       PM / hibernate: memory_bm_find_bit -- tighten node optimisation
>
>  kernel/power/snapshot.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
>

This patch has been accepted upstream and applied to linux-next, so I have
removed the "UBUNTU: SAUCE:" tags from the subject, added the s-o-b
block from upstream and added the provenance:

({backported/cherry picked} from commit 39800d8fc4083cfe5c8f69333336bf03f9571070 linux-next)

Applied to xenial, bionic, disco and eoan master-next branches.

Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team