[PATCH] kref: minor cleanup

[PATCH] kref: minor cleanup

[Anatol Pomozov]

Follow-up for https://lkml.org/lkml/2013/4/12/391

* make warning smp-safe
* result of atomic _unless_zero functions should be checked by caller
    to avoid use-after-free error

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 include/linux/kref.h | 9 ++++++---
 lib/kobject.c        | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..092529a 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
  */
 static inline void kref_get(struct kref *kref)
 {
-	WARN_ON(!atomic_read(&kref->refcount));
-	atomic_inc(&kref->refcount);
+	/* If refcount was 0 before incrementing then we have a race
+	 * condition when this kref is freing by some other thread right now.
+	 * In this case one should use kref_get_unless_zero()
+	 */
+	WARN_ON(atomic_inc_return(&kref->refcount) < 2);
 }
 
 /**
@@ -100,7 +103,7 @@ static inline int kref_put_mutex(struct kref *kref,
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
-        if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
+	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
 		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
 			mutex_unlock(lock);
diff --git a/lib/kobject.c b/lib/kobject.c
index a654866..bbd7362 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -529,7 +529,8 @@ struct kobject *kobject_get(struct kobject *kobj)
 	return kobj;
 }
 
-static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
+static struct kobject *__must_check kobject_get_unless_zero(
+		struct kobject *kobj)
 {
 	if (!kref_get_unless_zero(&kobj->kref))
 		kobj = NULL;
-- 
1.8.2.1

Re: [PATCH] kref: minor cleanup

[Greg KH]

On Fri, Apr 19, 2013 at 06:33:54PM -0700, Anatol Pomozov wrote:
> Follow-up for https://lkml.org/lkml/2013/4/12/391
> 
> * make warning smp-safe
> * result of atomic _unless_zero functions should be checked by caller
>     to avoid use-after-free error
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  include/linux/kref.h | 9 ++++++---
>  lib/kobject.c        | 3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index 4972e6e..092529a 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
>   */
>  static inline void kref_get(struct kref *kref)
>  {
> -	WARN_ON(!atomic_read(&kref->refcount));
> -	atomic_inc(&kref->refcount);
> +	/* If refcount was 0 before incrementing then we have a race
> +	 * condition when this kref is freing by some other thread right now.
> +	 * In this case one should use kref_get_unless_zero()
> +	 */
> +	WARN_ON(atomic_inc_return(&kref->refcount) < 2);

What happens if you disable WARN_ON(), does the atomic_inc_return() go
away as well?  Or did we fix that?

>  }
>  
>  /**
> @@ -100,7 +103,7 @@ static inline int kref_put_mutex(struct kref *kref,
>  				 struct mutex *lock)
>  {
>  	WARN_ON(release == NULL);
> -        if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
> +	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
>  		mutex_lock(lock);
>  		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
>  			mutex_unlock(lock);
> diff --git a/lib/kobject.c b/lib/kobject.c
> index a654866..bbd7362 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -529,7 +529,8 @@ struct kobject *kobject_get(struct kobject *kobj)
>  	return kobj;
>  }
>  
> -static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
> +static struct kobject *__must_check kobject_get_unless_zero(
> +		struct kobject *kobj)

__must_check needs to be in the .h file, not the .c file.

thanks,

greg k-h

Re: [PATCH] kref: minor cleanup

[Anatol Pomozov]

Hi

On Fri, Apr 19, 2013 at 7:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 19, 2013 at 06:33:54PM -0700, Anatol Pomozov wrote:
>> Follow-up for https://lkml.org/lkml/2013/4/12/391
>>
>> * make warning smp-safe
>> * result of atomic _unless_zero functions should be checked by caller
>>     to avoid use-after-free error
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  include/linux/kref.h | 9 ++++++---
>>  lib/kobject.c        | 3 ++-
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index 4972e6e..092529a 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
>>   */
>>  static inline void kref_get(struct kref *kref)
>>  {
>> -     WARN_ON(!atomic_read(&kref->refcount));
>> -     atomic_inc(&kref->refcount);
>> +     /* If refcount was 0 before incrementing then we have a race
>> +      * condition when this kref is freing by some other thread right now.
>> +      * In this case one should use kref_get_unless_zero()
>> +      */
>> +     WARN_ON(atomic_inc_return(&kref->refcount) < 2);
>
> What happens if you disable WARN_ON(), does the atomic_inc_return() go
> away as well?  Or did we fix that?

If we disable warnings then expression still evaluated, this is true
for BUG_ON as well. It is how the functions are implemented now.

Tejun Heo once mentioned that such behavior is specification of the
functions. So I believe it is safe to execute code inside WARN_ON.

>>  /**
>> @@ -100,7 +103,7 @@ static inline int kref_put_mutex(struct kref *kref,
>>                                struct mutex *lock)
>>  {
>>       WARN_ON(release == NULL);
>> -        if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
>> +     if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
>>               mutex_lock(lock);
>>               if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
>>                       mutex_unlock(lock);
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index a654866..bbd7362 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -529,7 +529,8 @@ struct kobject *kobject_get(struct kobject *kobj)
>>       return kobj;
>>  }
>>
>> -static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
>> +static struct kobject *__must_check kobject_get_unless_zero(
>> +             struct kobject *kobj)
>
> __must_check needs to be in the .h file, not the .c file.

This function is static and defined in *.c. But I think the function
should be declared in *.h as it might be useful for others. I'll
resend patch tomorrow.

[PATCH] kref: minor cleanup

[Anatol Pomozov]

Follow-up for https://lkml.org/lkml/2013/4/12/391

* make warning smp-safe
* result of atomic _unless_zero functions should be checked by caller
    to avoid use-after-free error

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 include/linux/kobject.h | 1 +
 include/linux/kref.h    | 9 ++++++---
 lib/kobject.c           | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 939b112..bfad936 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -101,6 +101,7 @@ extern int __must_check kobject_rename(struct kobject *, const char *new_name);
 extern int __must_check kobject_move(struct kobject *, struct kobject *);
 
 extern struct kobject *kobject_get(struct kobject *kobj);
+extern struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj);
 extern void kobject_put(struct kobject *kobj);
 
 extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..817d7f1 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
  */
 static inline void kref_get(struct kref *kref)
 {
-	WARN_ON(!atomic_read(&kref->refcount));
-	atomic_inc(&kref->refcount);
+	/* If refcount was 0 before incrementing then we have a race
+	 * condition when this kref is freeing by some other thread right now.
+	 * In this case one should use kref_get_unless_zero()
+	 */
+	WARN_ON(atomic_inc_return(&kref->refcount) < 2);
 }
 
 /**
@@ -100,7 +103,7 @@ static inline int kref_put_mutex(struct kref *kref,
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
-        if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
+	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
 		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
 			mutex_unlock(lock);
diff --git a/lib/kobject.c b/lib/kobject.c
index a654866..af32119 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -529,7 +529,7 @@ struct kobject *kobject_get(struct kobject *kobj)
 	return kobj;
 }
 
-static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
+struct kobject *kobject_get_unless_zero(struct kobject *kobj)
 {
 	if (!kref_get_unless_zero(&kobj->kref))
 		kobj = NULL;
-- 
1.8.2.1

Re: [PATCH] kref: minor cleanup

[Greg KH]

On Fri, Apr 19, 2013 at 09:27:20PM -0700, Anatol Pomozov wrote:
> >> -static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
> >> +static struct kobject *__must_check kobject_get_unless_zero(
> >> +             struct kobject *kobj)
> >
> > __must_check needs to be in the .h file, not the .c file.
> 
> This function is static and defined in *.c. But I think the function
> should be declared in *.h as it might be useful for others. I'll
> resend patch tomorrow.

No, I was trying to give you a hint in that it is not needed because
this function is not exported, sorry for being vague.

greg k-h

Re: [PATCH] kref: minor cleanup

[Greg KH]

On Sat, Apr 20, 2013 at 09:15:10AM -0700, Anatol Pomozov wrote:
> Follow-up for https://lkml.org/lkml/2013/4/12/391

That's not needed in a changelog comment.

> * make warning smp-safe
> * result of atomic _unless_zero functions should be checked by caller
>     to avoid use-after-free error

You also do other things you don't list here.

> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  include/linux/kobject.h | 1 +
>  include/linux/kref.h    | 9 ++++++---
>  lib/kobject.c           | 2 +-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index 939b112..bfad936 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -101,6 +101,7 @@ extern int __must_check kobject_rename(struct kobject *, const char *new_name);
>  extern int __must_check kobject_move(struct kobject *, struct kobject *);
>  
>  extern struct kobject *kobject_get(struct kobject *kobj);
> +extern struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj);

Don't add apis that no one uses.

And even if you did, you forgot to export it to modules :(


>  extern void kobject_put(struct kobject *kobj);
>  
>  extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index 4972e6e..817d7f1 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
>   */
>  static inline void kref_get(struct kref *kref)
>  {
> -	WARN_ON(!atomic_read(&kref->refcount));
> -	atomic_inc(&kref->refcount);
> +	/* If refcount was 0 before incrementing then we have a race
> +	 * condition when this kref is freeing by some other thread right now.
> +	 * In this case one should use kref_get_unless_zero()
> +	 */

Did you use scripts/checkpatch.pl?

> +	WARN_ON(atomic_inc_return(&kref->refcount) < 2);

As this is just a warning, I really doubt this type of change is needed,
have you ever hit it?

This is to catch people who forget to initialize a kref before doing the
first kref_get(), it's a help in debugging their code, nothing
"critical".

thanks,

greg k-h

Re: [PATCH] kref: minor cleanup

[Anatol Pomozov]

Hi

On Sat, Apr 20, 2013 at 3:34 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Apr 20, 2013 at 09:15:10AM -0700, Anatol Pomozov wrote:
>> Follow-up for https://lkml.org/lkml/2013/4/12/391
>
> That's not needed in a changelog comment.
>
>> * make warning smp-safe
>> * result of atomic _unless_zero functions should be checked by caller
>>     to avoid use-after-free error
>
> You also do other things you don't list here.
>
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  include/linux/kobject.h | 1 +
>>  include/linux/kref.h    | 9 ++++++---
>>  lib/kobject.c           | 2 +-
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
>> index 939b112..bfad936 100644
>> --- a/include/linux/kobject.h
>> +++ b/include/linux/kobject.h
>> @@ -101,6 +101,7 @@ extern int __must_check kobject_rename(struct kobject *, const char *new_name);
>>  extern int __must_check kobject_move(struct kobject *, struct kobject *);
>>
>>  extern struct kobject *kobject_get(struct kobject *kobj);
>> +extern struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj);
>
> Don't add apis that no one uses.

I am puzzled should I add the function to *.h or not? From my point of
view this function might be useful for other people.

But in any case "__must_check" should be added to the function
signature. Users suppose to check the return value otherwise they
might have use-after-free race condition.

> And even if you did, you forgot to export it to modules :(
>
>
>>  extern void kobject_put(struct kobject *kobj);
>>
>>  extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index 4972e6e..817d7f1 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
>>   */
>>  static inline void kref_get(struct kref *kref)
>>  {
>> -     WARN_ON(!atomic_read(&kref->refcount));
>> -     atomic_inc(&kref->refcount);
>> +     /* If refcount was 0 before incrementing then we have a race
>> +      * condition when this kref is freeing by some other thread right now.
>> +      * In this case one should use kref_get_unless_zero()
>> +      */
>
> Did you use scripts/checkpatch.pl?
>
>> +     WARN_ON(atomic_inc_return(&kref->refcount) < 2);
>
> As this is just a warning, I really doubt this type of change is needed

Warning is needed. And the check should be SMP-safe to avoid situation
when kref_put happens between refcount check and refcount increment.

> have you ever hit it?
> This is to catch people who forget to initialize a kref before doing the
> first kref_get(), it's a help in debugging their code, nothing
> "critical".

Users also hit this warning they when try to kobject_get() an element
that has refcount equal to zero (i.e. kobject in destroy path). One
situation when it might happen is iterating kset->list and the element
is removing by someone else. In this situation object is visible in
the list but one cannot use it as it is destroying right now. If one
tries to use it it'll get use-after-free bug. On debug kernel with
DEBUG_PAGEALLOC enabled it will cause a crash, without the flag it
will silently use corrupted data.

Having this warning (+ my comment) will at least give developers a
clue what is going on here.

[PATCH] kref: minor cleanup

[Anatol Pomozov]

* make warning smp-safe
* result of atomic _unless_zero functions should be checked by caller
    to avoid use-after-free error
* minor stylistic fixes

Link: https://lkml.org/lkml/2013/4/12/391

Tested: compile x86, boot machine and run xfstests
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 include/linux/kref.h | 9 ++++++---
 lib/kobject.c        | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..817d7f1 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -39,8 +39,11 @@ static inline void kref_init(struct kref *kref)
  */
 static inline void kref_get(struct kref *kref)
 {
-	WARN_ON(!atomic_read(&kref->refcount));
-	atomic_inc(&kref->refcount);
+	/* If refcount was 0 before incrementing then we have a race
+	 * condition when this kref is freeing by some other thread right now.
+	 * In this case one should use kref_get_unless_zero()
+	 */
+	WARN_ON(atomic_inc_return(&kref->refcount) < 2);
 }
 
 /**
@@ -100,7 +103,7 @@ static inline int kref_put_mutex(struct kref *kref,
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
-        if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
+	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
 		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
 			mutex_unlock(lock);
diff --git a/lib/kobject.c b/lib/kobject.c
index a654866..7b13611 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -529,7 +529,8 @@ struct kobject *kobject_get(struct kobject *kobj)
 	return kobj;
 }
 
-static struct kobject *kobject_get_unless_zero(struct kobject *kobj)
+static struct kobject * __must_check kobject_get_unless_zero(
+	struct kobject *kobj)
 {
 	if (!kref_get_unless_zero(&kobj->kref))
 		kobj = NULL;
-- 
1.8.2.1