Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Muchun Song]




> On Apr 28, 2026, at 20:34, Miaohe Lin <linmiaohe@huawei.com> wrote:
> On 2026/4/28 19:40, Muchun Song wrote:
>> 
>> 
>>> On Apr 28, 2026, at 19:37, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>> On 2026/4/28 16:52, Muchun Song wrote:
>>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
>>>> find_memory_block_by_id(), which requires device_hotplug_lock to
>>>> serialize the xarray lookup against memory block removal.
>>>> Take device_hotplug_lock around the lookup and nr_hwpoison update so
>>>> the memory block cannot disappear between xa_load() and get_device().
>>>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> Thanks for update.
>>>> ---
>>>> drivers/base/memory.c | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>>> index 6981b55d582a..f76aee29e9a5 100644
>>>> --- a/drivers/base/memory.c
>>>> +++ b/drivers/base/memory.c
>>>> @@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
>>>> void memblk_nr_poison_inc(unsigned long pfn)
>>>> {
>>>>    const unsigned long block_id = pfn_to_block_id(pfn);
>>>> -    struct memory_block *mem = find_memory_block_by_id(block_id);
>>>> +    struct memory_block *mem;
>>>> +    lock_device_hotplug();
>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() are both called from memory_failure() context.
>>> I'm afraid if memory_failure() is triggered while lock_device_hotplug is held, it will lead to
>>> deadlock. Or am I miss something?
>> 
>> I am curious is there any place where memory_failure() is called with holding lock_device_hotplug?
> 
> Sorry for dumb scenario, I was a bit too presumptuous. But there might be another possible deadlock:
> 
> remove_memory
>  lock_device_hotplug <-- first called here
>  try_remove_memory
>    remove_memory_block_devices
>      num_poisoned_pages_sub

Passing pfn = -1 here.

>        memblk_nr_poison_sub
>          lock_device_hotplug <-- deadlock here

No. Can’t reach here. No deadlock.

Thanks.

> 
> Hope I'm not mistaken again. :)
> 
> Thank.
> .

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Miaohe Lin]

On 2026/4/28 21:52, Muchun Song wrote:
> 
> 
> 
>> On Apr 28, 2026, at 20:34, Miaohe Lin <linmiaohe@huawei.com> wrote:
>> On 2026/4/28 19:40, Muchun Song wrote:
>>>
>>>
>>>> On Apr 28, 2026, at 19:37, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>>> On 2026/4/28 16:52, Muchun Song wrote:
>>>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
>>>>> find_memory_block_by_id(), which requires device_hotplug_lock to
>>>>> serialize the xarray lookup against memory block removal.
>>>>> Take device_hotplug_lock around the lookup and nr_hwpoison update so
>>>>> the memory block cannot disappear between xa_load() and get_device().
>>>>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>>> Thanks for update.
>>>>> ---
>>>>> drivers/base/memory.c | 10 ++++++++--
>>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>>>> index 6981b55d582a..f76aee29e9a5 100644
>>>>> --- a/drivers/base/memory.c
>>>>> +++ b/drivers/base/memory.c
>>>>> @@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
>>>>> void memblk_nr_poison_inc(unsigned long pfn)
>>>>> {
>>>>>    const unsigned long block_id = pfn_to_block_id(pfn);
>>>>> -    struct memory_block *mem = find_memory_block_by_id(block_id);
>>>>> +    struct memory_block *mem;
>>>>> +    lock_device_hotplug();
>>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() are both called from memory_failure() context.
>>>> I'm afraid if memory_failure() is triggered while lock_device_hotplug is held, it will lead to
>>>> deadlock. Or am I miss something?
>>>
>>> I am curious is there any place where memory_failure() is called with holding lock_device_hotplug?
>>
>> Sorry for dumb scenario, I was a bit too presumptuous. But there might be another possible deadlock:
>>
>> remove_memory
>>  lock_device_hotplug <-- first called here
>>  try_remove_memory
>>    remove_memory_block_devices
>>      num_poisoned_pages_sub
> 
> Passing pfn = -1 here.
> 
>>        memblk_nr_poison_sub
>>          lock_device_hotplug <-- deadlock here
> 
> No. Can’t reach here. No deadlock.

Right, I missed that. Thanks. But I'm still worried that there might be potential issues.
For example, this function could be called while lock_page is held. Acquiring lock_device_hotplug
while already holding lock_page might cause problems, though I haven't seen any specific issues yet.
Also there might be some other potential scenarios that haven't been considered. Hope I'm just
overthinking it. :)

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks.
.

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Oscar Salvador]

On Wed, Apr 29, 2026 at 11:08:51AM +0800, Miaohe Lin wrote:
> Right, I missed that. Thanks. But I'm still worried that there might be potential issues.
> For example, this function could be called while lock_page is held. Acquiring lock_device_hotplug
> while already holding lock_page might cause problems, though I haven't seen any specific issues yet.
> Also there might be some other potential scenarios that haven't been considered. Hope I'm just
> overthinking it. :)

lock_device_hotplug is a mutex lock, and we already take other mutex locks while
holding lock_folio in other paths, so I am not sure I see what should be special
in this case.


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Muchun Song]

> On Apr 29, 2026, at 11:32, Oscar Salvador <osalvador@suse.de> wrote:
> 
> On Wed, Apr 29, 2026 at 11:08:51AM +0800, Miaohe Lin wrote:
>> Right, I missed that. Thanks. But I'm still worried that there might be potential issues.
>> For example, this function could be called while lock_page is held. Acquiring lock_device_hotplug
>> while already holding lock_page might cause problems, though I haven't seen any specific issues yet.
>> Also there might be some other potential scenarios that haven't been considered. Hope I'm just
>> overthinking it. :)
> 
> lock_device_hotplug is a mutex lock, and we already take other mutex locks while
> holding lock_folio in other paths, so I am not sure I see what should be special
> in this case.

Hi Oscar and Miaohe,

I saw sashiko's report [1] related to folio lock and lock_device_hotplug.
Seems it is possible. You can correct me if I am wrong.

[1] https://sashiko.dev/#/patchset/20260428085219.1316047-1-songmuchun%40bytedance.com

We could fix this by calling action_result() without holding folio lock.
What do you think?

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ee42d4361309..bc76066547ce 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2534,8 +2534,9 @@ int memory_failure(unsigned long pfn, int flags)
         * Abort on fail: __filemap_remove_folio() assumes unmapped page.
         */
        if (!hwpoison_user_mappings(folio, p, pfn, flags)) {
+               folio_unlock(folio);
                res = action_result(pfn, MF_MSG_UNMAP_FAILED, MF_FAILED);
-               goto unlock_page;
+               goto unlock_mutex;
        }

        /*
@@ -2543,16 +2544,15 @@ int memory_failure(unsigned long pfn, int flags)
         */
        if (folio_test_lru(folio) && !folio_test_swapcache(folio) &&
            folio->mapping == NULL) {
+               folio_unlock(folio);
                res = action_result(pfn, MF_MSG_TRUNCATED_LRU, MF_IGNORED);
-               goto unlock_page;
+               goto unlock_mutex;
        }

 identify_page_state:
        res = identify_page_state(pfn, p, page_flags);
        mutex_unlock(&mf_mutex);
        return res;
-unlock_page:
-       folio_unlock(folio);
 unlock_mutex:
        mutex_unlock(&mf_mutex);
        return res;

> 
> 
> -- 
> Oscar Salvador
> SUSE Labs

[PATCH v2 0/3] mm: Fix memory block leaks and locking

[Muchun Song]

This series fixes two memory block device reference leaks and one
locking issue around the per-memory_block hwpoison counter.

Changes in v2:
- Add Acked-by from Oscar Salvador to patch 1.
- Keep the series as pure bug fixes; defer
  memory_block_get_by_id()/memory_block_put cleanup to a separate
  series.
- Add Reviewed-by from Miaohe Lin to patch 2.
- Add device_hotplug_lock handling in patch 3.

Muchun Song (3):
  mm/memory_hotplug: fix memory block reference leak on remove
  drivers/base/memory: fix memory block reference leak in poison
    accounting
  drivers/base/memory: fix locking for poison accounting lookup

 drivers/base/memory.c | 18 ++++++++++++++----
 mm/memory_hotplug.c   |  2 ++
 2 files changed, 16 insertions(+), 4 deletions(-)


base-commit: 7080e32d3f09d8688c4a87d81bdcc71f7f606b16
-- 
2.20.1

[PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Muchun Song]

memblk_nr_poison_inc() and memblk_nr_poison_sub() call
find_memory_block_by_id(), which requires device_hotplug_lock to
serialize the xarray lookup against memory block removal.

Take device_hotplug_lock around the lookup and nr_hwpoison update so
the memory block cannot disappear between xa_load() and get_device().

Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
Cc: stable@vger.kernel.org
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 drivers/base/memory.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 6981b55d582a..f76aee29e9a5 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
 void memblk_nr_poison_inc(unsigned long pfn)
 {
 	const unsigned long block_id = pfn_to_block_id(pfn);
-	struct memory_block *mem = find_memory_block_by_id(block_id);
+	struct memory_block *mem;
 
+	lock_device_hotplug();
+	mem = find_memory_block_by_id(block_id);
 	if (mem) {
 		atomic_long_inc(&mem->nr_hwpoison);
 		put_device(&mem->dev);
 	}
+	unlock_device_hotplug();
 }
 
 void memblk_nr_poison_sub(unsigned long pfn, long i)
 {
 	const unsigned long block_id = pfn_to_block_id(pfn);
-	struct memory_block *mem = find_memory_block_by_id(block_id);
+	struct memory_block *mem;
 
+	lock_device_hotplug();
+	mem = find_memory_block_by_id(block_id);
 	if (mem) {
 		atomic_long_sub(i, &mem->nr_hwpoison);
 		put_device(&mem->dev);
 	}
+	unlock_device_hotplug();
 }
 
 static unsigned long memblk_nr_poison(struct memory_block *mem)
-- 
2.20.1

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Oscar Salvador]

On Tue, Apr 28, 2026 at 04:52:19PM +0800, Muchun Song wrote:
> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
> find_memory_block_by_id(), which requires device_hotplug_lock to
> serialize the xarray lookup against memory block removal.
> 
> Take device_hotplug_lock around the lookup and nr_hwpoison update so
> the memory block cannot disappear between xa_load() and get_device().
> 
> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

It might have made sense to join both patches? Anyway:

Acked-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Muchun Song]

> On Apr 28, 2026, at 17:17, Oscar Salvador <osalvador@suse.de> wrote:
> 
> On Tue, Apr 28, 2026 at 04:52:19PM +0800, Muchun Song wrote:
>> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
>> find_memory_block_by_id(), which requires device_hotplug_lock to
>> serialize the xarray lookup against memory block removal.
>> 
>> Take device_hotplug_lock around the lookup and nr_hwpoison update so
>> the memory block cannot disappear between xa_load() and get_device().
>> 
>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> It might have made sense to join both patches? Anyway:

Either way works for me. I’ve been following the 'one thing per
patch' principle. If I still need to update v3, I can merge them;
otherwise, I’d prefer to keep it as is. I'm a little lazy. :)

> 
> Acked-by: Oscar Salvador <osalvador@suse.de>

Thanks.

> 
> 
> -- 
> Oscar Salvador
> SUSE Labs

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Miaohe Lin]

On 2026/4/28 16:52, Muchun Song wrote:
> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
> find_memory_block_by_id(), which requires device_hotplug_lock to
> serialize the xarray lookup against memory block removal.
> 
> Take device_hotplug_lock around the lookup and nr_hwpoison update so
> the memory block cannot disappear between xa_load() and get_device().
> 
> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

Thanks for update.

> ---
>  drivers/base/memory.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 6981b55d582a..f76aee29e9a5 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
>  void memblk_nr_poison_inc(unsigned long pfn)
>  {
>  	const unsigned long block_id = pfn_to_block_id(pfn);
> -	struct memory_block *mem = find_memory_block_by_id(block_id);
> +	struct memory_block *mem;
>  
> +	lock_device_hotplug();

memblk_nr_poison_inc() and memblk_nr_poison_sub() are both called from memory_failure() context.
I'm afraid if memory_failure() is triggered while lock_device_hotplug is held, it will lead to
deadlock. Or am I miss something?

Thanks.
.

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Muchun Song]

> On Apr 28, 2026, at 19:37, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2026/4/28 16:52, Muchun Song wrote:
>> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
>> find_memory_block_by_id(), which requires device_hotplug_lock to
>> serialize the xarray lookup against memory block removal.
>> 
>> Take device_hotplug_lock around the lookup and nr_hwpoison update so
>> the memory block cannot disappear between xa_load() and get_device().
>> 
>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> Thanks for update.
> 
>> ---
>> drivers/base/memory.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 6981b55d582a..f76aee29e9a5 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
>> void memblk_nr_poison_inc(unsigned long pfn)
>> {
>> 	const unsigned long block_id = pfn_to_block_id(pfn);
>> - 	struct memory_block *mem = find_memory_block_by_id(block_id);
>> + 	struct memory_block *mem;
>> 
>> + 	lock_device_hotplug();
> 
> memblk_nr_poison_inc() and memblk_nr_poison_sub() are both called from memory_failure() context.
> I'm afraid if memory_failure() is triggered while lock_device_hotplug is held, it will lead to
> deadlock. Or am I miss something?

I am curious is there any place where memory_failure() is called with holding lock_device_hotplug?

Thanks.

> 
> Thanks.
> .

Re: [PATCH v2 3/3] drivers/base/memory: fix locking for poison accounting lookup

[Miaohe Lin]

On 2026/4/28 19:40, Muchun Song wrote:
> 
> 
>> On Apr 28, 2026, at 19:37, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> On 2026/4/28 16:52, Muchun Song wrote:
>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() call
>>> find_memory_block_by_id(), which requires device_hotplug_lock to
>>> serialize the xarray lookup against memory block removal.
>>>
>>> Take device_hotplug_lock around the lookup and nr_hwpoison update so
>>> the memory block cannot disappear between xa_load() and get_device().
>>>
>>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>
>> Thanks for update.
>>
>>> ---
>>> drivers/base/memory.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>> index 6981b55d582a..f76aee29e9a5 100644
>>> --- a/drivers/base/memory.c
>>> +++ b/drivers/base/memory.c
>>> @@ -1228,23 +1228,29 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
>>> void memblk_nr_poison_inc(unsigned long pfn)
>>> {
>>> 	const unsigned long block_id = pfn_to_block_id(pfn);
>>> - 	struct memory_block *mem = find_memory_block_by_id(block_id);
>>> + 	struct memory_block *mem;
>>>
>>> + 	lock_device_hotplug();
>>
>> memblk_nr_poison_inc() and memblk_nr_poison_sub() are both called from memory_failure() context.
>> I'm afraid if memory_failure() is triggered while lock_device_hotplug is held, it will lead to
>> deadlock. Or am I miss something?
> 
> I am curious is there any place where memory_failure() is called with holding lock_device_hotplug?

Sorry for dumb scenario, I was a bit too presumptuous. But there might be another possible deadlock:

remove_memory
  lock_device_hotplug <-- first called here
  try_remove_memory
    remove_memory_block_devices
      num_poisoned_pages_sub
        memblk_nr_poison_sub
          lock_device_hotplug <-- deadlock here

Hope I'm not mistaken again. :)

Thank.
.