Matias,
Can you pick this up for 4.20? Even though it is not an error per se, it
does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
set.
Thanks,
Javier
Javier González (1):
lightnvm: pblk: avoid ref warning on cache creation
drivers/lightnvm/pblk-init.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
--
2.7.4
The current kref implementation around pblk global caches triggers a
false positive on refcount_inc_checked() (when called) as the kref is
initialized to 0. Instead of usint kref_inc() on a 0 reference, which is
in principle correct, use kref_init() to avoid the check. This is also
more explicit about what actually happens on cache creation.
In the process, do a small refactoring to use kref helpers.
Signed-off-by: Javier González <[email protected]>
---
drivers/lightnvm/pblk-init.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 13822594647c..e225bd60cbb4 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -350,23 +350,19 @@ static int pblk_create_global_caches(void)
static int pblk_get_global_caches(void)
{
- int ret;
+ int ret = 0;
mutex_lock(&pblk_caches.mutex);
- if (kref_read(&pblk_caches.kref) > 0) {
- kref_get(&pblk_caches.kref);
- mutex_unlock(&pblk_caches.mutex);
- return 0;
- }
+ if (kref_get_unless_zero(&pblk_caches.kref))
+ goto out;
ret = pblk_create_global_caches();
-
if (!ret)
- kref_get(&pblk_caches.kref);
+ kref_init(&pblk_caches.kref);
+out:
mutex_unlock(&pblk_caches.mutex);
-
return ret;
}
--
2.7.4
> On 23 Nov 2018, at 09.14, Matias Bjørling <[email protected]> wrote:
>
>> On 11/22/2018 02:46 PM, Javier González wrote:
>> Matias,
>> Can you pick this up for 4.20? Even though it is not an error per se, it
>> does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
>> set.
>> Thanks,
>> Javier
>> Javier González (1):
>> lightnvm: pblk: avoid ref warning on cache creation
>> drivers/lightnvm/pblk-init.c | 14 +++++---------
>> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> It is too late to get it for 4.20. I'll pull it in for 4.21.
Well, if it is a fix it’s not too late AFAIK. It also applies to a patch picked up in this series. We should have catches this before.
But I guess it’s your call.
>
> We can put on a Fixes: 1864de94ec9d6 "lightnvm: pblk: stop recreating global caches" if you want Greg and friends to pick picked it up for stable kernels?
I didn’t add it as it is not a bug in itself. But it is good to add it. Can you do it.
Javier.
On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
>
>
>> On 23 Nov 2018, at 09.14, Matias Bjørling <[email protected]> wrote:
>>
>>> On 11/22/2018 02:46 PM, Javier González wrote:
>>> Matias,
>>> Can you pick this up for 4.20? Even though it is not an error per se, it
>>> does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
>>> set.
>>> Thanks,
>>> Javier
>>> Javier González (1):
>>> lightnvm: pblk: avoid ref warning on cache creation
>>> drivers/lightnvm/pblk-init.c | 14 +++++---------
>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> It is too late to get it for 4.20. I'll pull it in for 4.21.
>
> Well, if it is a fix it’s not too late AFAIK. It also applies to a patch picked up in this series. We should have catches this before.
>
> But I guess it’s your call.
There is a likelihood that Linus would catch it, with the result of fire
& fury. I don't feel like using one of my cat life's for this one.
>
>>
>> We can put on a Fixes: 1864de94ec9d6 "lightnvm: pblk: stop recreating global caches" if you want Greg and friends to pick picked it up for stable kernels?
>
> I didn’t add it as it is not a bug in itself. But it is good to add it. Can you do it.
>
I'll do it.
> Javier.
>
On 11/22/2018 02:46 PM, Javier González wrote:
> Matias,
>
> Can you pick this up for 4.20? Even though it is not an error per se, it
> does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
> set.
>
> Thanks,
> Javier
>
> Javier González (1):
> lightnvm: pblk: avoid ref warning on cache creation
>
> drivers/lightnvm/pblk-init.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
It is too late to get it for 4.20. I'll pull it in for 4.21.
We can put on a Fixes: 1864de94ec9d6 "lightnvm: pblk: stop recreating
global caches" if you want Greg and friends to pick picked it up for
stable kernels?
> On 23 Nov 2018, at 09.24, Matias Bjørling <[email protected]> wrote:
>
> On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
>>> On 23 Nov 2018, at 09.14, Matias Bjørling <[email protected]> wrote:
>>>
>>>> On 11/22/2018 02:46 PM, Javier González wrote:
>>>> Matias,
>>>> Can you pick this up for 4.20? Even though it is not an error per se, it
>>>> does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
>>>> set.
>>>> Thanks,
>>>> Javier
>>>> Javier González (1):
>>>> lightnvm: pblk: avoid ref warning on cache creation
>>>> drivers/lightnvm/pblk-init.c | 14 +++++---------
>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> It is too late to get it for 4.20. I'll pull it in for 4.21.
>> Well, if it is a fix it’s not too late AFAIK. It also applies to a patch picked up in this series. We should have catches this before.
>> But I guess it’s your call.
>
> There is a likelihood that Linus would catch it, with the result of fire & fury. I don't feel like using one of my cat life's for this one.
I don’t get how a fix in rc3 will result in Jens or Linus complaining. But Ok, let’s release 4.20 with a false positive warning for use-after-free on pblk creation.
>>> We can put on a Fixes: 1864de94ec9d6 "lightnvm: pblk: stop recreating global caches" if you want Greg and friends to pick picked it up for stable kernels?
>> I didn’t add it as it is not a bug in itself. But it is good to add it. Can you do it.
>
> I'll do it.
Thanks.
Javier.
On 11/23/2018 09:31 AM, Javier Gonzalez wrote:
>
>
>> On 23 Nov 2018, at 09.24, Matias Bjørling <[email protected]> wrote:
>>
>> On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
>>>> On 23 Nov 2018, at 09.14, Matias Bjørling <[email protected]> wrote:
>>>>
>>>>> On 11/22/2018 02:46 PM, Javier González wrote:
>>>>> Matias,
>>>>> Can you pick this up for 4.20? Even though it is not an error per se, it
>>>>> does trigger an ugly false positive warning when CONFIG_REFCOUNT_FULL is
>>>>> set.
>>>>> Thanks,
>>>>> Javier
>>>>> Javier González (1):
>>>>> lightnvm: pblk: avoid ref warning on cache creation
>>>>> drivers/lightnvm/pblk-init.c | 14 +++++---------
>>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>>
>>>> It is too late to get it for 4.20. I'll pull it in for 4.21.
>>> Well, if it is a fix it’s not too late AFAIK. It also applies to a patch picked up in this series. We should have catches this before.
>>> But I guess it’s your call.
>>
>> There is a likelihood that Linus would catch it, with the result of fire & fury. I don't feel like using one of my cat life's for this one.
>
> I don’t get how a fix in rc3 will result in Jens or Linus complaining. But Ok, let’s release 4.20 with a false positive warning for use-after-free on pblk creation.
>
It is a warning that would trigger only for kernel developers. The
REFCOUNT_FULL is used when debugging code, and is usually not on in
production kernels.
To my understanding, the rc's, are meant for errors that would affect
end-users.
>
>>>> We can put on a Fixes: 1864de94ec9d6 "lightnvm: pblk: stop recreating global caches" if you want Greg and friends to pick picked it up for stable kernels?
>>> I didn’t add it as it is not a bug in itself. But it is good to add it. Can you do it.
>>
>> I'll do it.
>
> Thanks.
>
> Javier.
>
Great catch Javier! Nice refactoring work too.
Reviewed-by: Hans Holmberg <[email protected]>
On Thu, Nov 22, 2018 at 2:47 PM Javier González <[email protected]> wrote:
>
> The current kref implementation around pblk global caches triggers a
> false positive on refcount_inc_checked() (when called) as the kref is
> initialized to 0. Instead of usint kref_inc() on a 0 reference, which is
> in principle correct, use kref_init() to avoid the check. This is also
> more explicit about what actually happens on cache creation.
>
> In the process, do a small refactoring to use kref helpers.
>
> Signed-off-by: Javier González <[email protected]>
> ---
> drivers/lightnvm/pblk-init.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index 13822594647c..e225bd60cbb4 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -350,23 +350,19 @@ static int pblk_create_global_caches(void)
>
> static int pblk_get_global_caches(void)
> {
> - int ret;
> + int ret = 0;
>
> mutex_lock(&pblk_caches.mutex);
>
> - if (kref_read(&pblk_caches.kref) > 0) {
> - kref_get(&pblk_caches.kref);
> - mutex_unlock(&pblk_caches.mutex);
> - return 0;
> - }
> + if (kref_get_unless_zero(&pblk_caches.kref))
> + goto out;
>
> ret = pblk_create_global_caches();
> -
> if (!ret)
> - kref_get(&pblk_caches.kref);
> + kref_init(&pblk_caches.kref);
>
> +out:
> mutex_unlock(&pblk_caches.mutex);
> -
> return ret;
> }
>
> --
> 2.7.4
>
On 11/23/2018 11:04 AM, Hans Holmberg wrote:
> Great catch Javier! Nice refactoring work too.
>
> Reviewed-by: Hans Holmberg <[email protected]>
> On Thu, Nov 22, 2018 at 2:47 PM Javier González <[email protected]> wrote:
>>
>> The current kref implementation around pblk global caches triggers a
>> false positive on refcount_inc_checked() (when called) as the kref is
>> initialized to 0. Instead of usint kref_inc() on a 0 reference, which is
>> in principle correct, use kref_init() to avoid the check. This is also
>> more explicit about what actually happens on cache creation.
>>
>> In the process, do a small refactoring to use kref helpers.
>>
>> Signed-off-by: Javier González <[email protected]>
>> ---
>> drivers/lightnvm/pblk-init.c | 14 +++++---------
>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index 13822594647c..e225bd60cbb4 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -350,23 +350,19 @@ static int pblk_create_global_caches(void)
>>
>> static int pblk_get_global_caches(void)
>> {
>> - int ret;
>> + int ret = 0;
>>
>> mutex_lock(&pblk_caches.mutex);
>>
>> - if (kref_read(&pblk_caches.kref) > 0) {
>> - kref_get(&pblk_caches.kref);
>> - mutex_unlock(&pblk_caches.mutex);
>> - return 0;
>> - }
>> + if (kref_get_unless_zero(&pblk_caches.kref))
>> + goto out;
>>
>> ret = pblk_create_global_caches();
>> -
>> if (!ret)
>> - kref_get(&pblk_caches.kref);
>> + kref_init(&pblk_caches.kref);
>>
>> +out:
>> mutex_unlock(&pblk_caches.mutex);
>> -
>> return ret;
>> }
>>
>> --
>> 2.7.4
>>
Picked up for 4.21.