[v1] mm: memory_failure: unmap poisoned filio during migrate properly

[PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio

Posted by Wupeng Ma 1 year ago

From: Ma Wupeng <mawupeng1@huawei.com>

Commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to
be offlined) add page poison checks in do_migrate_range in order to make
offline hwpoisoned page possible by introducing isolate_lru_page and
try_to_unmap for hwpoisoned page. However folio lock must be held before
calling try_to_unmap. Add it to fix this problem.

Waring will be produced if filio is not locked during unmap:

  ------------[ cut here ]------------
  kernel BUG at ./include/linux/swapops.h:400!
  Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 4 UID: 0 PID: 411 Comm: bash Tainted: G        W          6.13.0-rc1-00016-g3c434c7ee82a-dirty #41
  Tainted: [W]=WARN
  Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
  pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : try_to_unmap_one+0xb08/0xd3c
  lr : try_to_unmap_one+0x3dc/0xd3c
  Call trace:
   try_to_unmap_one+0xb08/0xd3c (P)
   try_to_unmap_one+0x3dc/0xd3c (L)
   rmap_walk_anon+0xdc/0x1f8
   rmap_walk+0x3c/0x58
   try_to_unmap+0x88/0x90
   unmap_poisoned_folio+0x30/0xa8
   do_migrate_range+0x4a0/0x568
   offline_pages+0x5a4/0x670
   memory_block_action+0x17c/0x374
   memory_subsys_offline+0x3c/0x78
   device_offline+0xa4/0xd0
   state_store+0x8c/0xf0
   dev_attr_store+0x18/0x2c
   sysfs_kf_write+0x44/0x54
   kernfs_fop_write_iter+0x118/0x1a8
   vfs_write+0x3a8/0x4bc
   ksys_write+0x6c/0xf8
   __arm64_sys_write+0x1c/0x28
   invoke_syscall+0x44/0x100
   el0_svc_common.constprop.0+0x40/0xe0
   do_el0_svc+0x1c/0x28
   el0_svc+0x30/0xd0
   el0t_64_sync_handler+0xc8/0xcc
   el0t_64_sync+0x198/0x19c
  Code: f9407be0 b5fff320 d4210000 17ffff97 (d4210000)
  ---[ end trace 0000000000000000 ]---

Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/memory_hotplug.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 330668d37e44..9bedecfc3577 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1805,8 +1805,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
 			if (WARN_ON(folio_test_lru(folio)))
 				folio_isolate_lru(folio);
-			if (folio_mapped(folio))
+			if (folio_mapped(folio)) {
+				folio_lock(folio);
 				unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK | TTU_HWPOISON);
+				folio_unlock(folio);
+			}
+
 			continue;
 		}
 
-- 
2.25.1

Re: [PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio

Posted by David Hildenbrand 1 year ago

On 13.01.25 09:27, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> Commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to
> be offlined) add page poison checks in do_migrate_range in order to make
> offline hwpoisoned page possible by introducing isolate_lru_page and
> try_to_unmap for hwpoisoned page. However folio lock must be held before
> calling try_to_unmap. Add it to fix this problem.
> 
> Waring will be produced if filio is not locked during unmap:
> 
>    ------------[ cut here ]------------
>    kernel BUG at ./include/linux/swapops.h:400!
>    Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>    Modules linked in:
>    CPU: 4 UID: 0 PID: 411 Comm: bash Tainted: G        W          6.13.0-rc1-00016-g3c434c7ee82a-dirty #41
>    Tainted: [W]=WARN
>    Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>    pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>    pc : try_to_unmap_one+0xb08/0xd3c
>    lr : try_to_unmap_one+0x3dc/0xd3c
>    Call trace:
>     try_to_unmap_one+0xb08/0xd3c (P)
>     try_to_unmap_one+0x3dc/0xd3c (L)
>     rmap_walk_anon+0xdc/0x1f8
>     rmap_walk+0x3c/0x58
>     try_to_unmap+0x88/0x90
>     unmap_poisoned_folio+0x30/0xa8
>     do_migrate_range+0x4a0/0x568
>     offline_pages+0x5a4/0x670
>     memory_block_action+0x17c/0x374
>     memory_subsys_offline+0x3c/0x78
>     device_offline+0xa4/0xd0
>     state_store+0x8c/0xf0
>     dev_attr_store+0x18/0x2c
>     sysfs_kf_write+0x44/0x54
>     kernfs_fop_write_iter+0x118/0x1a8
>     vfs_write+0x3a8/0x4bc
>     ksys_write+0x6c/0xf8
>     __arm64_sys_write+0x1c/0x28
>     invoke_syscall+0x44/0x100
>     el0_svc_common.constprop.0+0x40/0xe0
>     do_el0_svc+0x1c/0x28
>     el0_svc+0x30/0xd0
>     el0t_64_sync_handler+0xc8/0xcc
>     el0t_64_sync+0x198/0x19c
>    Code: f9407be0 b5fff320 d4210000 17ffff97 (d4210000)
>    ---[ end trace 0000000000000000 ]---
> 
> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   mm/memory_hotplug.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 330668d37e44..9bedecfc3577 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1805,8 +1805,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>   		    (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>   			if (WARN_ON(folio_test_lru(folio)))
>   				folio_isolate_lru(folio);
> -			if (folio_mapped(folio))
> +			if (folio_mapped(folio)) {
> +				folio_lock(folio);
>   				unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK | TTU_HWPOISON);
> +				folio_unlock(folio);
> +			}

The comment above says "have elevated reference counts", but I I wonder 
if this code could race with un-poisoning (although probably a rare event).


If there is an elevated reference already, why not move that chunk after 
the folio_try_get() and just drop the comment that describes the implied 
magic?

I mean, migration of hwpoison is dangerous either way, so that's not the 
biggest problem I guess :)

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio

Posted by Miaohe Lin 1 year ago

On 2025/1/13 20:35, David Hildenbrand wrote:
> On 13.01.25 09:27, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> Commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to
>> be offlined) add page poison checks in do_migrate_range in order to make
>> offline hwpoisoned page possible by introducing isolate_lru_page and
>> try_to_unmap for hwpoisoned page. However folio lock must be held before
>> calling try_to_unmap. Add it to fix this problem.
>>
>> Waring will be produced if filio is not locked during unmap:
>>
>>    ------------[ cut here ]------------
>>    kernel BUG at ./include/linux/swapops.h:400!
>>    Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>    Modules linked in:
>>    CPU: 4 UID: 0 PID: 411 Comm: bash Tainted: G        W          6.13.0-rc1-00016-g3c434c7ee82a-dirty #41
>>    Tainted: [W]=WARN
>>    Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>>    pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>    pc : try_to_unmap_one+0xb08/0xd3c
>>    lr : try_to_unmap_one+0x3dc/0xd3c
>>    Call trace:
>>     try_to_unmap_one+0xb08/0xd3c (P)
>>     try_to_unmap_one+0x3dc/0xd3c (L)
>>     rmap_walk_anon+0xdc/0x1f8
>>     rmap_walk+0x3c/0x58
>>     try_to_unmap+0x88/0x90
>>     unmap_poisoned_folio+0x30/0xa8
>>     do_migrate_range+0x4a0/0x568
>>     offline_pages+0x5a4/0x670
>>     memory_block_action+0x17c/0x374
>>     memory_subsys_offline+0x3c/0x78
>>     device_offline+0xa4/0xd0
>>     state_store+0x8c/0xf0
>>     dev_attr_store+0x18/0x2c
>>     sysfs_kf_write+0x44/0x54
>>     kernfs_fop_write_iter+0x118/0x1a8
>>     vfs_write+0x3a8/0x4bc
>>     ksys_write+0x6c/0xf8
>>     __arm64_sys_write+0x1c/0x28
>>     invoke_syscall+0x44/0x100
>>     el0_svc_common.constprop.0+0x40/0xe0
>>     do_el0_svc+0x1c/0x28
>>     el0_svc+0x30/0xd0
>>     el0t_64_sync_handler+0xc8/0xcc
>>     el0t_64_sync+0x198/0x19c
>>    Code: f9407be0 b5fff320 d4210000 17ffff97 (d4210000)
>>    ---[ end trace 0000000000000000 ]---
>>
>> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 330668d37e44..9bedecfc3577 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1805,8 +1805,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>               (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>               if (WARN_ON(folio_test_lru(folio)))
>>                   folio_isolate_lru(folio);
>> -            if (folio_mapped(folio))
>> +            if (folio_mapped(folio)) {
>> +                folio_lock(folio);
>>                   unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK | TTU_HWPOISON);
>> +                folio_unlock(folio);
>> +            }
> 
> The comment above says "have elevated reference counts", but I I wonder if this code could race with un-poisoning (although probably a rare event).

AFAICS, this will open the race window with unpoisoning:

unpoison_memory		do_migrate_range
			  folio_try_get
  if (folio_ref_count(folio) > 1)
    return -EBUSY;

But this should be benign one and userspace can simply retry later.

Thanks.
.

Re: [PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio

Posted by mawupeng 1 year ago


On 2025/1/13 20:35, David Hildenbrand wrote:
> On 13.01.25 09:27, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> Commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to
>> be offlined) add page poison checks in do_migrate_range in order to make
>> offline hwpoisoned page possible by introducing isolate_lru_page and
>> try_to_unmap for hwpoisoned page. However folio lock must be held before
>> calling try_to_unmap. Add it to fix this problem.
>>
>> Waring will be produced if filio is not locked during unmap:
>>
>>    ------------[ cut here ]------------
>>    kernel BUG at ./include/linux/swapops.h:400!
>>    Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>    Modules linked in:
>>    CPU: 4 UID: 0 PID: 411 Comm: bash Tainted: G        W          6.13.0-rc1-00016-g3c434c7ee82a-dirty #41
>>    Tainted: [W]=WARN
>>    Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>>    pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>    pc : try_to_unmap_one+0xb08/0xd3c
>>    lr : try_to_unmap_one+0x3dc/0xd3c
>>    Call trace:
>>     try_to_unmap_one+0xb08/0xd3c (P)
>>     try_to_unmap_one+0x3dc/0xd3c (L)
>>     rmap_walk_anon+0xdc/0x1f8
>>     rmap_walk+0x3c/0x58
>>     try_to_unmap+0x88/0x90
>>     unmap_poisoned_folio+0x30/0xa8
>>     do_migrate_range+0x4a0/0x568
>>     offline_pages+0x5a4/0x670
>>     memory_block_action+0x17c/0x374
>>     memory_subsys_offline+0x3c/0x78
>>     device_offline+0xa4/0xd0
>>     state_store+0x8c/0xf0
>>     dev_attr_store+0x18/0x2c
>>     sysfs_kf_write+0x44/0x54
>>     kernfs_fop_write_iter+0x118/0x1a8
>>     vfs_write+0x3a8/0x4bc
>>     ksys_write+0x6c/0xf8
>>     __arm64_sys_write+0x1c/0x28
>>     invoke_syscall+0x44/0x100
>>     el0_svc_common.constprop.0+0x40/0xe0
>>     do_el0_svc+0x1c/0x28
>>     el0_svc+0x30/0xd0
>>     el0t_64_sync_handler+0xc8/0xcc
>>     el0t_64_sync+0x198/0x19c
>>    Code: f9407be0 b5fff320 d4210000 17ffff97 (d4210000)
>>    ---[ end trace 0000000000000000 ]---
>>
>> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 330668d37e44..9bedecfc3577 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1805,8 +1805,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>               (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>               if (WARN_ON(folio_test_lru(folio)))
>>                   folio_isolate_lru(folio);
>> -            if (folio_mapped(folio))
>> +            if (folio_mapped(folio)) {
>> +                folio_lock(folio);
>>                   unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK | TTU_HWPOISON);
>> +                folio_unlock(folio);
>> +            }
> 
> The comment above says "have elevated reference counts", but I I wonder if this code could race with un-poisoning (although probably a rare event).
> 
> 
> If there is an elevated reference already, why not move that chunk after the folio_try_get() and just drop the comment that describes the implied magic?
> 
> I mean, migration of hwpoison is dangerous either way, so that's not the biggest problem I guess :)

AFAICT during memory_failure, the refcount will not be cleared for poisoned page in order to keep it away from free buddy pages.
So move that chunk after the folio_try_get() do seems nice and poisoned free pages do seems weird to me.

>

Re: [PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio

Posted by David Hildenbrand 1 year ago

On 14.01.25 10:08, mawupeng wrote:
> 
> 
> On 2025/1/13 20:35, David Hildenbrand wrote:
>> On 13.01.25 09:27, Wupeng Ma wrote:
>>> From: Ma Wupeng <mawupeng1@huawei.com>
>>>
>>> Commit b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to
>>> be offlined) add page poison checks in do_migrate_range in order to make
>>> offline hwpoisoned page possible by introducing isolate_lru_page and
>>> try_to_unmap for hwpoisoned page. However folio lock must be held before
>>> calling try_to_unmap. Add it to fix this problem.
>>>
>>> Waring will be produced if filio is not locked during unmap:
>>>
>>>     ------------[ cut here ]------------
>>>     kernel BUG at ./include/linux/swapops.h:400!
>>>     Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>>     Modules linked in:
>>>     CPU: 4 UID: 0 PID: 411 Comm: bash Tainted: G        W          6.13.0-rc1-00016-g3c434c7ee82a-dirty #41
>>>     Tainted: [W]=WARN
>>>     Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>>>     pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>     pc : try_to_unmap_one+0xb08/0xd3c
>>>     lr : try_to_unmap_one+0x3dc/0xd3c
>>>     Call trace:
>>>      try_to_unmap_one+0xb08/0xd3c (P)
>>>      try_to_unmap_one+0x3dc/0xd3c (L)
>>>      rmap_walk_anon+0xdc/0x1f8
>>>      rmap_walk+0x3c/0x58
>>>      try_to_unmap+0x88/0x90
>>>      unmap_poisoned_folio+0x30/0xa8
>>>      do_migrate_range+0x4a0/0x568
>>>      offline_pages+0x5a4/0x670
>>>      memory_block_action+0x17c/0x374
>>>      memory_subsys_offline+0x3c/0x78
>>>      device_offline+0xa4/0xd0
>>>      state_store+0x8c/0xf0
>>>      dev_attr_store+0x18/0x2c
>>>      sysfs_kf_write+0x44/0x54
>>>      kernfs_fop_write_iter+0x118/0x1a8
>>>      vfs_write+0x3a8/0x4bc
>>>      ksys_write+0x6c/0xf8
>>>      __arm64_sys_write+0x1c/0x28
>>>      invoke_syscall+0x44/0x100
>>>      el0_svc_common.constprop.0+0x40/0xe0
>>>      do_el0_svc+0x1c/0x28
>>>      el0_svc+0x30/0xd0
>>>      el0t_64_sync_handler+0xc8/0xcc
>>>      el0t_64_sync+0x198/0x19c
>>>     Code: f9407be0 b5fff320 d4210000 17ffff97 (d4210000)
>>>     ---[ end trace 0000000000000000 ]---
>>>
>>> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>> ---
>>>    mm/memory_hotplug.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 330668d37e44..9bedecfc3577 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1805,8 +1805,12 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>                (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
>>>                if (WARN_ON(folio_test_lru(folio)))
>>>                    folio_isolate_lru(folio);
>>> -            if (folio_mapped(folio))
>>> +            if (folio_mapped(folio)) {
>>> +                folio_lock(folio);
>>>                    unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK | TTU_HWPOISON);
>>> +                folio_unlock(folio);
>>> +            }
>>
>> The comment above says "have elevated reference counts", but I I wonder if this code could race with un-poisoning (although probably a rare event).
>>
>>
>> If there is an elevated reference already, why not move that chunk after the folio_try_get() and just drop the comment that describes the implied magic?
>>
>> I mean, migration of hwpoison is dangerous either way, so that's not the biggest problem I guess :)
> 
> AFAICT during memory_failure, the refcount will not be cleared for poisoned page in order to keep it away from free buddy pages.
> So move that chunk after the folio_try_get() do seems nice and poisoned free pages do seems weird to me.

If the "free" poisoned pages have a raised refcount, folio_try_get() 
would be able to grab them to essentially skip them (not mapped).

If the "free" poisoned pages don't have a raised refcount (impossible 
right now IIRC), folio_try_get() would skip, them, which would be the 
right thing to do.

So I think that should work.

-- 
Cheers,

David / dhildenb

[PATCH 1/2] mm: memory_hotplug: add TTU_HWPOISON for poisoned folio during migrate
[PATCH 2/2] hwpoison, memory_hotplug: lock folio before unmap hwpoisoned folio