* [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block()
@ 2022-04-22 7:15 Christophe JAILLET
2022-04-22 7:34 ` Michal Hocko
2022-04-22 7:39 ` David Hildenbrand
0 siblings, 2 replies; 4+ messages in thread
From: Christophe JAILLET @ 2022-04-22 7:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Scott Cheloha,
Michal Hocko, Andrew Morton, Nathan Lynch, David Hildenbrand
Cc: linux-kernel, kernel-janitors, Christophe JAILLET
There is no point in doing put_device()/device_unregister() on a
device that has just been registered a few lines above. This will lead to
a double reference decrement.
I guess that this put_device()/device_unregister() is a cut'n'paste from
remove_memory_block() (i.e. unregister_memory() at the time being) which
does need it.
Fixes: 4fb6eabf1037 ("drivers/base/memory.c: cache memory blocks in xarray to accelerate lookup")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/base/memory.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 7222ff9b5e05..084d67fd55cc 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -636,10 +636,9 @@ static int __add_memory_block(struct memory_block *memory)
}
ret = xa_err(xa_store(&memory_blocks, memory->dev.id, memory,
GFP_KERNEL));
- if (ret) {
- put_device(&memory->dev);
+ if (ret)
device_unregister(&memory->dev);
- }
+
return ret;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block()
2022-04-22 7:15 [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block() Christophe JAILLET
@ 2022-04-22 7:34 ` Michal Hocko
2022-04-22 8:01 ` Christophe JAILLET
2022-04-22 7:39 ` David Hildenbrand
1 sibling, 1 reply; 4+ messages in thread
From: Michal Hocko @ 2022-04-22 7:34 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Scott Cheloha,
Andrew Morton, Nathan Lynch, David Hildenbrand, linux-kernel,
kernel-janitors
On Fri 22-04-22 09:15:21, Christophe JAILLET wrote:
> There is no point in doing put_device()/device_unregister() on a
> device that has just been registered a few lines above. This will lead to
> a double reference decrement.
This is a bit confusing. I would rephrase.
"
__add_memory_block calls both put_device and device_unregister when
storing the memory block into the xarray. This is incorrect because
xarray doesn't take an additional reference and device_unregister
already calls put_device.
"
Btw. I do not think this failure path can be triggered, or is there a
way to hit it?
> I guess that this put_device()/device_unregister() is a cut'n'paste from
> remove_memory_block() (i.e. unregister_memory() at the time being) which
> does need it.
>
> Fixes: 4fb6eabf1037 ("drivers/base/memory.c: cache memory blocks in xarray to accelerate lookup")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Other than that looks good to me. With the changelog clarified,
especially the part that evaluates whether this is a real or a
theoretical problem, feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
Thanks!
> ---
> drivers/base/memory.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 7222ff9b5e05..084d67fd55cc 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -636,10 +636,9 @@ static int __add_memory_block(struct memory_block *memory)
> }
> ret = xa_err(xa_store(&memory_blocks, memory->dev.id, memory,
> GFP_KERNEL));
> - if (ret) {
> - put_device(&memory->dev);
> + if (ret)
> device_unregister(&memory->dev);
> - }
> +
> return ret;
> }
>
> --
> 2.32.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block()
2022-04-22 7:15 [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block() Christophe JAILLET
2022-04-22 7:34 ` Michal Hocko
@ 2022-04-22 7:39 ` David Hildenbrand
1 sibling, 0 replies; 4+ messages in thread
From: David Hildenbrand @ 2022-04-22 7:39 UTC (permalink / raw)
To: Christophe JAILLET, Greg Kroah-Hartman, Rafael J. Wysocki,
Scott Cheloha, Michal Hocko, Andrew Morton, Nathan Lynch
Cc: linux-kernel, kernel-janitors
On 22.04.22 09:15, Christophe JAILLET wrote:
> There is no point in doing put_device()/device_unregister() on a
> device that has just been registered a few lines above. This will lead to
> a double reference decrement.
>
> I guess that this put_device()/device_unregister() is a cut'n'paste from
> remove_memory_block() (i.e. unregister_memory() at the time being) which
> does need it.
>
> Fixes: 4fb6eabf1037 ("drivers/base/memory.c: cache memory blocks in xarray to accelerate lookup")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/base/memory.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 7222ff9b5e05..084d67fd55cc 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -636,10 +636,9 @@ static int __add_memory_block(struct memory_block *memory)
> }
> ret = xa_err(xa_store(&memory_blocks, memory->dev.id, memory,
> GFP_KERNEL));
> - if (ret) {
> - put_device(&memory->dev);
> + if (ret)
> device_unregister(&memory->dev);
> - }
> +
> return ret;
> }
>
Looks correct to me. I assume this will only happen once the stars
align, but it's most certainly wrong I think.
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block()
2022-04-22 7:34 ` Michal Hocko
@ 2022-04-22 8:01 ` Christophe JAILLET
0 siblings, 0 replies; 4+ messages in thread
From: Christophe JAILLET @ 2022-04-22 8:01 UTC (permalink / raw)
To: Michal Hocko, David Hildenbrand
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Scott Cheloha,
Andrew Morton, Nathan Lynch, linux-kernel, kernel-janitors
Le 22/04/2022 à 09:34, Michal Hocko a écrit :
> On Fri 22-04-22 09:15:21, Christophe JAILLET wrote:
>> There is no point in doing put_device()/device_unregister() on a
>> device that has just been registered a few lines above. This will lead to
>> a double reference decrement.
>
> This is a bit confusing. I would rephrase.
> "
> __add_memory_block calls both put_device and device_unregister when
> storing the memory block into the xarray. This is incorrect because
> xarray doesn't take an additional reference and device_unregister
> already calls put_device.
> "
>
> Btw. I do not think this failure path can be triggered, or is there a
> way to hit it?
>
>> I guess that this put_device()/device_unregister() is a cut'n'paste from
>> remove_memory_block() (i.e. unregister_memory() at the time being) which
>> does need it.
>>
>> Fixes: 4fb6eabf1037 ("drivers/base/memory.c: cache memory blocks in xarray to accelerate lookup")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> Other than that looks good to me. With the changelog clarified,
> especially the part that evaluates whether this is a real or a
> theoretical problem, feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
I'll send a v2 with the updated changelog.
I do agree that this is certainly a theoretical issue.
Moreover, should it be triggered, I think that it would only print a
warning message about an erroneous decrement on a ref counted resource
that is already 0.
Well, it also saves 2 LoC and will look more logical to other readers
(and static checkers :) )
Thanks for the quick review for both of you.
CJ
>
> Thanks!
>
>> ---
>> drivers/base/memory.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 7222ff9b5e05..084d67fd55cc 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -636,10 +636,9 @@ static int __add_memory_block(struct memory_block *memory)
>> }
>> ret = xa_err(xa_store(&memory_blocks, memory->dev.id, memory,
>> GFP_KERNEL));
>> - if (ret) {
>> - put_device(&memory->dev);
>> + if (ret)
>> device_unregister(&memory->dev);
>> - }
>> +
>> return ret;
>> }
>>
>> --
>> 2.32.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-22 8:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-22 7:15 [PATCH] drivers/base/memory: Fix a reference counting issue in __add_memory_block() Christophe JAILLET
2022-04-22 7:34 ` Michal Hocko
2022-04-22 8:01 ` Christophe JAILLET
2022-04-22 7:39 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).