[PATCH 1/2] mm/memory_hotplug: fix memory block reference leak on remove

[PATCH 1/2] mm/memory_hotplug: fix memory block reference leak on remove

[Muchun Song]

remove_memory_blocks_and_altmaps() looks up each memory block with
find_memory_block(), which acquires a reference to the memory block
device.

That reference is never dropped on this path, resulting in a leaked
device reference when removing memory blocks and their altmaps. Drop
the reference after retrieving mem->altmap and clearing mem->altmap,
before removing the memory block device.

Fixes: 6b8f0798b85a ("mm/memory_hotplug: split memmap_on_memory requests across memblocks")
Cc: stable@vger.kernel.org
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memory_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2a943ec57c85..4426abb05655 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1422,6 +1422,7 @@ static void remove_memory_blocks_and_altmaps(u64 start, u64 size)
 
 		altmap = mem->altmap;
 		mem->altmap = NULL;
+		put_device(&mem->dev);
 
 		remove_memory_block_devices(cur_start, memblock_size);
 

base-commit: 7080e32d3f09d8688c4a87d81bdcc71f7f606b16
-- 
2.20.1

[PATCH 2/2] drivers/base/memory: fix memory block reference leak in poison accounting

[Muchun Song]

memblk_nr_poison_inc() and memblk_nr_poison_sub() look up a memory
block via find_memory_block_by_id(), which acquires a reference to the
memory block device.

Both helpers use the returned memory block without dropping that
reference, leaking the device reference on each successful lookup. Drop
the reference after updating nr_hwpoison.

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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index f806a683b767..6981b55d582a 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -1230,8 +1230,10 @@ 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);
 
-	if (mem)
+	if (mem) {
 		atomic_long_inc(&mem->nr_hwpoison);
+		put_device(&mem->dev);
+	}
 }
 
 void memblk_nr_poison_sub(unsigned long pfn, long i)
@@ -1239,8 +1241,10 @@ 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);
 
-	if (mem)
+	if (mem) {
 		atomic_long_sub(i, &mem->nr_hwpoison);
+		put_device(&mem->dev);
+	}
 }
 
 static unsigned long memblk_nr_poison(struct memory_block *mem)
-- 
2.20.1

Re: [PATCH 2/2] drivers/base/memory: fix memory block reference leak in poison accounting

[Miaohe Lin]

On 2026/4/26 22:44, Muchun Song wrote:
> memblk_nr_poison_inc() and memblk_nr_poison_sub() look up a memory
> block via find_memory_block_by_id(), which acquires a reference to the
> memory block device.
> 
> Both helpers use the returned memory block without dropping that
> reference, leaking the device reference on each successful lookup. Drop
> the reference after updating nr_hwpoison.
> 
> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

This patch looks good to me with one question below:

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

> ---
>  drivers/base/memory.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index f806a683b767..6981b55d582a 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -1230,8 +1230,10 @@ 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);
>  
> -	if (mem)
> +	if (mem) {
>  		atomic_long_inc(&mem->nr_hwpoison);
> +		put_device(&mem->dev);

Comment above find_memory_block_by_id says it's called under device_hotplug_lock.

/*
 * A reference for the returned memory block device is acquired.
 *
 * Called under device_hotplug_lock.
 */
struct memory_block *find_memory_block_by_id(unsigned long block_id)

But device_hotplug_lock is missing here. Should we add it?

Thanks.
.

Re: [PATCH 2/2] drivers/base/memory: fix memory block reference leak in poison accounting

[Muchun Song]

> On Apr 27, 2026, at 17:13, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2026/4/26 22:44, Muchun Song wrote:
>> memblk_nr_poison_inc() and memblk_nr_poison_sub() look up a memory
>> block via find_memory_block_by_id(), which acquires a reference to the
>> memory block device.
>> 
>> Both helpers use the returned memory block without dropping that
>> reference, leaking the device reference on each successful lookup. Drop
>> the reference after updating nr_hwpoison.
>> 
>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> This patch looks good to me with one question below:
> 
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Thanks.

> 
>> ---
>> drivers/base/memory.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index f806a683b767..6981b55d582a 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -1230,8 +1230,10 @@ 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);
>> 
>> - 	if (mem)
>> + 	if (mem) {
>> 		atomic_long_inc(&mem->nr_hwpoison);
>> + 		put_device(&mem->dev);
> 
> Comment above find_memory_block_by_id says it's called under device_hotplug_lock.
> 
> /*
> * A reference for the returned memory block device is acquired.
> *
> * Called under device_hotplug_lock.
> */
> struct memory_block *find_memory_block_by_id(unsigned long block_id)
> 
> But device_hotplug_lock is missing here. Should we add it?

Yes. Otherwise mem can be freed concurrently. sashiko.dev reported
the issue as well.

Thanks.

https://sashiko.dev/#/patchset/20260426144447.817722-1-songmuchun%40bytedance.com

> 
> Thanks.
> .

Re: [PATCH 2/2] drivers/base/memory: fix memory block reference leak in poison accounting

[David Hildenbrand (Arm)]

On 4/27/26 11:16, Muchun Song wrote:
> 
> 
>> On Apr 27, 2026, at 17:13, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> On 2026/4/26 22:44, Muchun Song wrote:
>>> memblk_nr_poison_inc() and memblk_nr_poison_sub() look up a memory
>>> block via find_memory_block_by_id(), which acquires a reference to the
>>> memory block device.
>>>
>>> Both helpers use the returned memory block without dropping that
>>> reference, leaking the device reference on each successful lookup. Drop
>>> the reference after updating nr_hwpoison.
>>>
>>> Fixes: 5033091de814 ("mm/hwpoison: introduce per-memory_block hwpoison counter")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>
>> This patch looks good to me with one question below:
>>
>> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Thanks.
> 
>>
>>> ---
>>> drivers/base/memory.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>> index f806a683b767..6981b55d582a 100644
>>> --- a/drivers/base/memory.c
>>> +++ b/drivers/base/memory.c
>>> @@ -1230,8 +1230,10 @@ 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);
>>>
>>> - 	if (mem)
>>> + 	if (mem) {
>>> 		atomic_long_inc(&mem->nr_hwpoison);
>>> + 		put_device(&mem->dev);
>>
>> Comment above find_memory_block_by_id says it's called under device_hotplug_lock.
>>
>> /*
>> * A reference for the returned memory block device is acquired.
>> *
>> * Called under device_hotplug_lock.
>> */
>> struct memory_block *find_memory_block_by_id(unsigned long block_id)
>>
>> But device_hotplug_lock is missing here. Should we add it?
> 
> Yes. Otherwise mem can be freed concurrently.

I guess that is rather unlikely to happen, given that we just worked on online
memory. But sure, if we can take that lock there easily, then let's do that.

-- 
Cheers,

David

Re: [PATCH 1/2] mm/memory_hotplug: fix memory block reference leak on remove

[Oscar Salvador]

On Sun, Apr 26, 2026 at 10:44:46PM +0800, Muchun Song wrote:
> remove_memory_blocks_and_altmaps() looks up each memory block with
> find_memory_block(), which acquires a reference to the memory block
> device.
> 
> That reference is never dropped on this path, resulting in a leaked
> device reference when removing memory blocks and their altmaps. Drop
> the reference after retrieving mem->altmap and clearing mem->altmap,
> before removing the memory block device.
> 
> Fixes: 6b8f0798b85a ("mm/memory_hotplug: split memmap_on_memory requests across memblocks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

The change looks good but some comments below:

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

The outcome of leaking the reference is that the final call to put_device()
in device_unregister() leaves the memory block device linked in the
system under /sys/ ? (besides not deleting the struct I guess)

> ---
>  mm/memory_hotplug.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2a943ec57c85..4426abb05655 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1422,6 +1422,7 @@ static void remove_memory_blocks_and_altmaps(u64 start, u64 size)
>  
>  		altmap = mem->altmap;
>  		mem->altmap = NULL;
> +		put_device(&mem->dev);

1) Six months from now we might not remember why we need to call
   put_device() here.

   I would put a comment like remove_memory_block has:

   "/* drop the ref. we got via find_memory_block() */" or something like
   that.

2) I kind of dislike having an internal put_device() lingering here in
   memory-hotplug code, it feels like it does not really belong here.
   Ideally we should have a high-level function in drivers/base/memory.c
   that calls put_device itself.
   Something like "put_memblock_dev", dunno, names are hard.
 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 1/2] mm/memory_hotplug: fix memory block reference leak on remove

[Muchun Song]

> On Apr 27, 2026, at 15:49, Oscar Salvador <osalvador@suse.de> wrote:
> 
> On Sun, Apr 26, 2026 at 10:44:46PM +0800, Muchun Song wrote:
>> remove_memory_blocks_and_altmaps() looks up each memory block with
>> find_memory_block(), which acquires a reference to the memory block
>> device.
>> 
>> That reference is never dropped on this path, resulting in a leaked
>> device reference when removing memory blocks and their altmaps. Drop
>> the reference after retrieving mem->altmap and clearing mem->altmap,
>> before removing the memory block device.
>> 
>> Fixes: 6b8f0798b85a ("mm/memory_hotplug: split memmap_on_memory requests across memblocks")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> 
> The change looks good but some comments below:
> 
> Acked-by: Oscar Salvador <osalvador@suse.de>

Thanks.

> 
> The outcome of leaking the reference is that the final call to put_device()
> in device_unregister() leaves the memory block device linked in the
> system under /sys/ ? (besides not deleting the struct I guess)

I think so.

> 
>> ---
>> mm/memory_hotplug.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 2a943ec57c85..4426abb05655 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1422,6 +1422,7 @@ static void remove_memory_blocks_and_altmaps(u64 start, u64 size)
>> 
>> 	altmap = mem->altmap;
>> 	mem->altmap = NULL;
>> +	put_device(&mem->dev);
> 
> 1) Six months from now we might not remember why we need to call
>   put_device() here.
> 
>   I would put a comment like remove_memory_block has:
> 
>   "/* drop the ref. we got via find_memory_block() */" or something like
>   that.

Make sense.

> 
> 2) I kind of dislike having an internal put_device() lingering here in
>   memory-hotplug code, it feels like it does not really belong here.
>   Ideally we should have a high-level function in drivers/base/memory.c
>   that calls put_device itself.
>   Something like "put_memblock_dev", dunno, names are hard.
> 

I share your perspective. The current naming of find_memory_block_by_id
is ambiguous as it fails to signal the internal 'get' operation. To improve
clarity and reduce errors, it should be renamed to memory_block_get_by_id.
Pairing this with a new memory_block_put function to wrap put_device
would ensure a more robust and intuitive API.

Thanks.

> 
> -- 
> Oscar Salvador
> SUSE Labs

Re: [PATCH 1/2] mm/memory_hotplug: fix memory block reference leak on remove

[Oscar Salvador]

On Mon, Apr 27, 2026 at 04:02:17PM +0800, Muchun Song wrote:
> 
> 
> > On Apr 27, 2026, at 15:49, Oscar Salvador <osalvador@suse.de> wrote:
> > 2) I kind of dislike having an internal put_device() lingering here in
> >   memory-hotplug code, it feels like it does not really belong here.
> >   Ideally we should have a high-level function in drivers/base/memory.c
> >   that calls put_device itself.
> >   Something like "put_memblock_dev", dunno, names are hard.
> > 
> 
> I share your perspective. The current naming of find_memory_block_by_id
> is ambiguous as it fails to signal the internal 'get' operation. To improve
> clarity and reduce errors, it should be renamed to memory_block_get_by_id.
> Pairing this with a new memory_block_put function to wrap put_device
> would ensure a more robust and intuitive API.

Even better, yes.

Thanks!


-- 
Oscar Salvador
SUSE Labs