lightnvm: pblk: avoid ref warning on cache creation

lightnvm: pblk: avoid ref warning on cache creation

[Javier González]

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

[PATCH] lightnvm: pblk: avoid ref warning on cache creation

[Javier González]

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 <javier@cnexlabs.com>
---
 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

Re: lightnvm: pblk: avoid ref warning on cache creation

[Matias Bjørling]

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?

Re: lightnvm: pblk: avoid ref warning on cache creation

[Javier Gonzalez]

> On 23 Nov 2018, at 09.14, Matias Bjørling <mb@lightnvm.io> 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. 

Re: lightnvm: pblk: avoid ref warning on cache creation

[Matias Bjørling]

On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
> 
> 
>> On 23 Nov 2018, at 09.14, Matias Bjørling <mb@lightnvm.io> 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.
> 

Re: lightnvm: pblk: avoid ref warning on cache creation

[Javier Gonzalez]

> On 23 Nov 2018, at 09.24, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
>>> On 23 Nov 2018, at 09.14, Matias Bjørling <mb@lightnvm.io> 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. 

Re: lightnvm: pblk: avoid ref warning on cache creation

[Matias Bjørling]

On 11/23/2018 09:31 AM, Javier Gonzalez wrote:
> 
> 
>> On 23 Nov 2018, at 09.24, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 11/23/2018 09:16 AM, Javier Gonzalez wrote:
>>>> On 23 Nov 2018, at 09.14, Matias Bjørling <mb@lightnvm.io> 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.
> 

Re: [PATCH] lightnvm: pblk: avoid ref warning on cache creation

[Hans Holmberg]

Great catch Javier! Nice refactoring work too.

Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
On Thu, Nov 22, 2018 at 2:47 PM Javier González <javier@javigon.com> 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 <javier@cnexlabs.com>
> ---
>  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
>

Re: [PATCH] lightnvm: pblk: avoid ref warning on cache creation

[Matias Bjørling]

On 11/23/2018 11:04 AM, Hans Holmberg wrote:
> Great catch Javier! Nice refactoring work too.
> 
> Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> On Thu, Nov 22, 2018 at 2:47 PM Javier González <javier@javigon.com> 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 <javier@cnexlabs.com>
>> ---
>>   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.