[PATCH] migration: Fix a potential guest memory corruption

[PATCH] migration: Fix a potential guest memory corruption

[Zhenzhong Duan]

Imagine a rare case, after a dirty page is sent to compression threads's
ring, dirty bitmap sync trigger right away and mark the same page dirty
again and sent. Then the new page may be overwriten by stale page in
compression threads's ring in the destination.

So we need to ensure there is only one copy of the same dirty page either
by flushing the ring after every bitmap sync or avoiding processing same
dirty page continuously.

I choose the 2nd which avoids the time consuming flush operation.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 migration/ram.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index dc1de9ddbc68..67b2035586bd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1551,7 +1551,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
     pss->postcopy_requested = false;
     pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
 
-    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
+    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page + 1);
     if (pss->complete_round && pss->block == rs->last_seen_block &&
         pss->page >= rs->last_page) {
         /*
@@ -1564,7 +1564,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
     if (!offset_in_ramblock(pss->block,
                             ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
         /* Didn't find anything in this RAM Block */
-        pss->page = 0;
+        pss->page = -1;
         pss->block = QLIST_NEXT_RCU(pss->block, next);
         if (!pss->block) {
             /*
@@ -2694,7 +2694,7 @@ static void ram_state_reset(RAMState *rs)
 {
     rs->last_seen_block = NULL;
     rs->last_sent_block = NULL;
-    rs->last_page = 0;
+    rs->last_page = -1;
     rs->last_version = ram_list.version;
     rs->xbzrle_enabled = false;
     postcopy_preempt_reset(rs);
@@ -2889,7 +2889,7 @@ void ram_postcopy_send_discard_bitmap(MigrationState *ms)
     /* Easiest way to make sure we don't resume in the middle of a host-page */
     rs->last_seen_block = NULL;
     rs->last_sent_block = NULL;
-    rs->last_page = 0;
+    rs->last_page = -1;
 
     postcopy_each_ram_send_discard(ms);
 
-- 
2.25.1

Re: [PATCH] migration: Fix a potential guest memory corruption

[Dr. David Alan Gilbert]

* Zhenzhong Duan (zhenzhong.duan@intel.com) wrote:

Hi,

> Imagine a rare case, after a dirty page is sent to compression threads's
> ring, dirty bitmap sync trigger right away and mark the same page dirty
> again and sent. Then the new page may be overwriten by stale page in
> compression threads's ring in the destination.

Yes, I think we had a similar problem in multifd.

> So we need to ensure there is only one copy of the same dirty page either
> by flushing the ring after every bitmap sync or avoiding processing same
> dirty page continuously.
> 
> I choose the 2nd which avoids the time consuming flush operation.

I'm not sure this guarantees it; it makes it much less likely but if
only a few pages are dirtied and you have lots of threads, I think the
same thing could still happy.

I think you're going to need to flush the ring after each sync.

Dave

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  migration/ram.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index dc1de9ddbc68..67b2035586bd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1551,7 +1551,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
>      pss->postcopy_requested = false;
>      pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
>  
> -    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
> +    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page + 1);
>      if (pss->complete_round && pss->block == rs->last_seen_block &&
>          pss->page >= rs->last_page) {
>          /*
> @@ -1564,7 +1564,7 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
>      if (!offset_in_ramblock(pss->block,
>                              ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
>          /* Didn't find anything in this RAM Block */
> -        pss->page = 0;
> +        pss->page = -1;
>          pss->block = QLIST_NEXT_RCU(pss->block, next);
>          if (!pss->block) {
>              /*
> @@ -2694,7 +2694,7 @@ static void ram_state_reset(RAMState *rs)
>  {
>      rs->last_seen_block = NULL;
>      rs->last_sent_block = NULL;
> -    rs->last_page = 0;
> +    rs->last_page = -1;
>      rs->last_version = ram_list.version;
>      rs->xbzrle_enabled = false;
>      postcopy_preempt_reset(rs);
> @@ -2889,7 +2889,7 @@ void ram_postcopy_send_discard_bitmap(MigrationState *ms)
>      /* Easiest way to make sure we don't resume in the middle of a host-page */
>      rs->last_seen_block = NULL;
>      rs->last_sent_block = NULL;
> -    rs->last_page = 0;
> +    rs->last_page = -1;
>  
>      postcopy_each_ram_send_discard(ms);
>  
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH] migration: Fix a potential guest memory corruption

[Duan, Zhenzhong]

>-----Original Message-----
>From: Dr. David Alan Gilbert <dgilbert@redhat.com>
>Sent: Tuesday, October 11, 2022 7:06 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; quintela@redhat.com
>Subject: Re: [PATCH] migration: Fix a potential guest memory corruption
>
>* Zhenzhong Duan (zhenzhong.duan@intel.com) wrote:
>
>Hi,
Hi,
Sorry for late response. Just back from vacation.

>
>> Imagine a rare case, after a dirty page is sent to compression
>> threads's ring, dirty bitmap sync trigger right away and mark the same
>> page dirty again and sent. Then the new page may be overwriten by
>> stale page in compression threads's ring in the destination.
>
>Yes, I think we had a similar problem in multifd.
Multifd flush operation multifd_send_sync_main() is called in each memory iteration
which is more aggressive than in compression. I think not an issue in multifd?

>
>> So we need to ensure there is only one copy of the same dirty page
>> either by flushing the ring after every bitmap sync or avoiding
>> processing same dirty page continuously.
>>
>> I choose the 2nd which avoids the time consuming flush operation.
>
>I'm not sure this guarantees it; it makes it much less likely but if only a few
>pages are dirtied and you have lots of threads, I think the same thing could
>still happy.
I didn't get it, imagine there are dirty page "A B C D E F G" in current RAMBLOCK.
1. Page "A B C D" are sent to compression thread.
2. dirty page sync triggers, update dirty map to "A B D E F G"
3. Page D is checked and sent to compression thread again, so there may be two copy of page D in compression thread, corruption!
4. Page "E F G" are sent to compression thread.
5. flush operation triggered at end of current RAMBLOCK.
6. In next iteration, page "A B" are sent.

After patch:
1. Page "A B C D" are sent to compression thread.
2. dirty page sync triggers, update dirty map to " A B D E F G"
3. Page after page D are checked and sent to compression thread which are Page "E F G".
5. flush operation triggered at end of current RAMBLOCK, ensures page D flushed.
6. In next iteration, page "A B D" are sent.

Thanks
Zhenzhong
>
>I think you're going to need to flush the ring after each sync.
>
>Dave
>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  migration/ram.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c index
>> dc1de9ddbc68..67b2035586bd 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1551,7 +1551,7 @@ static bool find_dirty_block(RAMState *rs,
>PageSearchStatus *pss, bool *again)
>>      pss->postcopy_requested = false;
>>      pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
>>
>> -    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
>> +    pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page
>> + + 1);
>>      if (pss->complete_round && pss->block == rs->last_seen_block &&
>>          pss->page >= rs->last_page) {
>>          /*
>> @@ -1564,7 +1564,7 @@ static bool find_dirty_block(RAMState *rs,
>PageSearchStatus *pss, bool *again)
>>      if (!offset_in_ramblock(pss->block,
>>                              ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
>>          /* Didn't find anything in this RAM Block */
>> -        pss->page = 0;
>> +        pss->page = -1;
>>          pss->block = QLIST_NEXT_RCU(pss->block, next);
>>          if (!pss->block) {
>>              /*
>> @@ -2694,7 +2694,7 @@ static void ram_state_reset(RAMState *rs)  {
>>      rs->last_seen_block = NULL;
>>      rs->last_sent_block = NULL;
>> -    rs->last_page = 0;
>> +    rs->last_page = -1;
>>      rs->last_version = ram_list.version;
>>      rs->xbzrle_enabled = false;
>>      postcopy_preempt_reset(rs);
>> @@ -2889,7 +2889,7 @@ void
>ram_postcopy_send_discard_bitmap(MigrationState *ms)
>>      /* Easiest way to make sure we don't resume in the middle of a host-page
>*/
>>      rs->last_seen_block = NULL;
>>      rs->last_sent_block = NULL;
>> -    rs->last_page = 0;
>> +    rs->last_page = -1;
>>
>>      postcopy_each_ram_send_discard(ms);
>>
>> --
>> 2.25.1
>>
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK