[PATCH] refcount: always allow checked forms

[PATCH] refcount: always allow checked forms

[Mark Rutland]

In many cases, it would be useful to be able to use the full
sanity-checked refcount helpers regardless of CONFIG_REFCOUNT_FULL, as
this would help to avoid duplicate warnings where callers try to
sanity-check refcount manipulation.

This patch refactors things such that the full refcount helpers were
always built, as refcount_${op}_checked(), such that they can be used
regardless of CONFIG_REFCOUNT_FULL. This will allow code which *always*
wants a checked refcount to opt-in, avoiding the need to duplicate the
logic for warnings.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: David Sterba <dsterba@suse.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 include/linux/refcount.h | 27 +++++++++++++++++-------
 lib/refcount.c           | 53 +++++++++++++++++++++++-------------------------
 2 files changed, 45 insertions(+), 35 deletions(-)

Dave pointed out that it would be useful to be able to opt-in to full checks
regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
always want checks. I've spotted a few of these in code which is still awaiting
conversion.

I'm assuming that the atomics group is intended to own the refcount code, even
though this isn't currently the case in MAINTAINERS.

Mark.

diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index a685da2c4522..b505f75ccf68 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -42,17 +42,30 @@ static inline unsigned int refcount_read(const refcount_t *r)
 	return atomic_read(&r->refs);
 }
 
+extern __must_check bool refcount_add_not_zero_checked(unsigned int i, refcount_t *r);
+extern void refcount_add_checked(unsigned int i, refcount_t *r);
+
+extern __must_check bool refcount_inc_not_zero_checked(refcount_t *r);
+extern void refcount_inc_checked(refcount_t *r);
+
+extern __must_check bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r);
+
+extern __must_check bool refcount_dec_and_test_checked(refcount_t *r);
+extern void refcount_dec_checked(refcount_t *r);
+
 #ifdef CONFIG_REFCOUNT_FULL
-extern __must_check bool refcount_add_not_zero(unsigned int i, refcount_t *r);
-extern void refcount_add(unsigned int i, refcount_t *r);
 
-extern __must_check bool refcount_inc_not_zero(refcount_t *r);
-extern void refcount_inc(refcount_t *r);
+#define refcount_add_not_zero	refcount_add_not_zero_checked
+#define refcount_add		refcount_add_checked
+
+#define refcount_inc_not_zero	refcount_inc_not_zero_checked
+#define refcount_inc		refcount_inc_checked
+
+#define refcount_sub_and_test	refcount_sub_and_test_checked
 
-extern __must_check bool refcount_sub_and_test(unsigned int i, refcount_t *r);
+#define refcount_dec_and_test	refcount_dec_and_test_checked
+#define refcount_dec		refcount_dec_checked
 
-extern __must_check bool refcount_dec_and_test(refcount_t *r);
-extern void refcount_dec(refcount_t *r);
 #else
 # ifdef CONFIG_ARCH_HAS_REFCOUNT
 #  include <asm/refcount.h>
diff --git a/lib/refcount.c b/lib/refcount.c
index d3b81cefce91..3d514f915999 100644
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -38,10 +38,8 @@
 #include <linux/refcount.h>
 #include <linux/bug.h>
 
-#ifdef CONFIG_REFCOUNT_FULL
-
 /**
- * refcount_add_not_zero - add a value to a refcount unless it is 0
+ * refcount_add_not_zero_checked - add a value to a refcount unless it is 0
  * @i: the value to add to the refcount
  * @r: the refcount
  *
@@ -58,7 +56,7 @@
  *
  * Return: false if the passed refcount is 0, true otherwise
  */
-bool refcount_add_not_zero(unsigned int i, refcount_t *r)
+bool refcount_add_not_zero_checked(unsigned int i, refcount_t *r)
 {
 	unsigned int new, val = atomic_read(&r->refs);
 
@@ -79,10 +77,10 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 
 	return true;
 }
-EXPORT_SYMBOL(refcount_add_not_zero);
+EXPORT_SYMBOL(refcount_add_not_zero_checked);
 
 /**
- * refcount_add - add a value to a refcount
+ * refcount_add_checked - add a value to a refcount
  * @i: the value to add to the refcount
  * @r: the refcount
  *
@@ -97,14 +95,14 @@ EXPORT_SYMBOL(refcount_add_not_zero);
  * cases, refcount_inc(), or one of its variants, should instead be used to
  * increment a reference count.
  */
-void refcount_add(unsigned int i, refcount_t *r)
+void refcount_add_checked(unsigned int i, refcount_t *r)
 {
-	WARN_ONCE(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+	WARN_ONCE(!refcount_add_not_zero_checked(i, r), "refcount_t: addition on 0; use-after-free.\n");
 }
-EXPORT_SYMBOL(refcount_add);
+EXPORT_SYMBOL(refcount_add_checked);
 
 /**
- * refcount_inc_not_zero - increment a refcount unless it is 0
+ * refcount_inc_not_zero_checked - increment a refcount unless it is 0
  * @r: the refcount to increment
  *
  * Similar to atomic_inc_not_zero(), but will saturate at UINT_MAX and WARN.
@@ -115,7 +113,7 @@ EXPORT_SYMBOL(refcount_add);
  *
  * Return: true if the increment was successful, false otherwise
  */
-bool refcount_inc_not_zero(refcount_t *r)
+bool refcount_inc_not_zero_checked(refcount_t *r)
 {
 	unsigned int new, val = atomic_read(&r->refs);
 
@@ -134,10 +132,10 @@ bool refcount_inc_not_zero(refcount_t *r)
 
 	return true;
 }
-EXPORT_SYMBOL(refcount_inc_not_zero);
+EXPORT_SYMBOL(refcount_inc_not_zero_checked);
 
 /**
- * refcount_inc - increment a refcount
+ * refcount_inc_checked - increment a refcount
  * @r: the refcount to increment
  *
  * Similar to atomic_inc(), but will saturate at UINT_MAX and WARN.
@@ -148,14 +146,14 @@ EXPORT_SYMBOL(refcount_inc_not_zero);
  * Will WARN if the refcount is 0, as this represents a possible use-after-free
  * condition.
  */
-void refcount_inc(refcount_t *r)
+void refcount_inc_chcked(refcount_t *r)
 {
-	WARN_ONCE(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+	WARN_ONCE(!refcount_inc_not_zero_checked(r), "refcount_t: increment on 0; use-after-free.\n");
 }
-EXPORT_SYMBOL(refcount_inc);
+EXPORT_SYMBOL(refcount_inc_checked);
 
 /**
- * refcount_sub_and_test - subtract from a refcount and test if it is 0
+ * refcount_sub_and_test_checked - subtract from a refcount and test if it is 0
  * @i: amount to subtract from the refcount
  * @r: the refcount
  *
@@ -174,7 +172,7 @@ EXPORT_SYMBOL(refcount_inc);
  *
  * Return: true if the resulting refcount is 0, false otherwise
  */
-bool refcount_sub_and_test(unsigned int i, refcount_t *r)
+bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r)
 {
 	unsigned int new, val = atomic_read(&r->refs);
 
@@ -192,10 +190,10 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 
 	return !new;
 }
-EXPORT_SYMBOL(refcount_sub_and_test);
+EXPORT_SYMBOL(refcount_sub_and_test_checked);
 
 /**
- * refcount_dec_and_test - decrement a refcount and test if it is 0
+ * refcount_dec_and_test_checked - decrement a refcount and test if it is 0
  * @r: the refcount
  *
  * Similar to atomic_dec_and_test(), it will WARN on underflow and fail to
@@ -207,14 +205,14 @@ EXPORT_SYMBOL(refcount_sub_and_test);
  *
  * Return: true if the resulting refcount is 0, false otherwise
  */
-bool refcount_dec_and_test(refcount_t *r)
+bool refcount_dec_and_test_checked(refcount_t *r)
 {
-	return refcount_sub_and_test(1, r);
+	return refcount_sub_and_test_checked(1, r);
 }
-EXPORT_SYMBOL(refcount_dec_and_test);
+EXPORT_SYMBOL(refcount_dec_and_test_checked);
 
 /**
- * refcount_dec - decrement a refcount
+ * refcount_dec_checked - decrement a refcount
  * @r: the refcount
  *
  * Similar to atomic_dec(), it will WARN on underflow and fail to decrement
@@ -223,12 +221,11 @@ EXPORT_SYMBOL(refcount_dec_and_test);
  * Provides release memory ordering, such that prior loads and stores are done
  * before.
  */
-void refcount_dec(refcount_t *r)
+void refcount_dec_checked(refcount_t *r)
 {
-	WARN_ONCE(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+	WARN_ONCE(refcount_dec_and_test_checked(r), "refcount_t: decrement hit 0; leaking memory.\n");
 }
-EXPORT_SYMBOL(refcount_dec);
-#endif /* CONFIG_REFCOUNT_FULL */
+EXPORT_SYMBOL(refcount_dec_checked);
 
 /**
  * refcount_dec_if_one - decrement a refcount if it is 1
-- 
2.11.0

Re: [PATCH] refcount: always allow checked forms

[Andrea Parri]

Hi Mark,

a typo below:


>  /**
> - * refcount_inc - increment a refcount
> + * refcount_inc_checked - increment a refcount
>   * @r: the refcount to increment
>   *
>   * Similar to atomic_inc(), but will saturate at UINT_MAX and WARN.
> @@ -148,14 +146,14 @@ EXPORT_SYMBOL(refcount_inc_not_zero);
>   * Will WARN if the refcount is 0, as this represents a possible use-after-free
>   * condition.
>   */
> -void refcount_inc(refcount_t *r)
> +void refcount_inc_chcked(refcount_t *r)

s/chcked/checked

  Andrea

Re: [PATCH] refcount: always allow checked forms

[Mark Rutland]

On Tue, Jul 03, 2018 at 12:33:20PM +0200, Andrea Parri wrote:
> Hi Mark,
> 
> a typo below:
> 
> 
> >  /**
> > - * refcount_inc - increment a refcount
> > + * refcount_inc_checked - increment a refcount
> >   * @r: the refcount to increment
> >   *
> >   * Similar to atomic_inc(), but will saturate at UINT_MAX and WARN.
> > @@ -148,14 +146,14 @@ EXPORT_SYMBOL(refcount_inc_not_zero);
> >   * Will WARN if the refcount is 0, as this represents a possible use-after-free
> >   * condition.
> >   */
> > -void refcount_inc(refcount_t *r)
> > +void refcount_inc_chcked(refcount_t *r)
> 
> s/chcked/checked

The kbuild test robot also just spotted this; I've fixed tht up locally
and pushed it out to my atomics/refcount-checked branch.

Thanks for the spot!

Mark.

Re: [PATCH] refcount: always allow checked forms

[Kees Cook]

On Tue, Jul 3, 2018 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> In many cases, it would be useful to be able to use the full
> sanity-checked refcount helpers regardless of CONFIG_REFCOUNT_FULL, as
> this would help to avoid duplicate warnings where callers try to
> sanity-check refcount manipulation.
>
> This patch refactors things such that the full refcount helpers were
> always built, as refcount_${op}_checked(), such that they can be used
> regardless of CONFIG_REFCOUNT_FULL. This will allow code which *always*
> wants a checked refcount to opt-in, avoiding the need to duplicate the
> logic for warnings.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>

Looks good to me! Thanks for doing this. :)

Acked-by: Kees Cook <keescook@chromium.org>

> ---
>  include/linux/refcount.h | 27 +++++++++++++++++-------
>  lib/refcount.c           | 53 +++++++++++++++++++++++-------------------------
>  2 files changed, 45 insertions(+), 35 deletions(-)
>
> Dave pointed out that it would be useful to be able to opt-in to full checks
> regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> always want checks. I've spotted a few of these in code which is still awaiting
> conversion.

Yeah, I need to go through the cocci output -- Elena had several
outstanding patches that never got picked up.

> I'm assuming that the atomics group is intended to own the refcount code, even
> though this isn't currently the case in MAINTAINERS.

That's how it has landed in the past, yes, but if there is a
dependency on these for code that will use it, maybe it should go that
way?

-Kees

>
> Mark.
>
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index a685da2c4522..b505f75ccf68 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -42,17 +42,30 @@ static inline unsigned int refcount_read(const refcount_t *r)
>         return atomic_read(&r->refs);
>  }
>
> +extern __must_check bool refcount_add_not_zero_checked(unsigned int i, refcount_t *r);
> +extern void refcount_add_checked(unsigned int i, refcount_t *r);
> +
> +extern __must_check bool refcount_inc_not_zero_checked(refcount_t *r);
> +extern void refcount_inc_checked(refcount_t *r);
> +
> +extern __must_check bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r);
> +
> +extern __must_check bool refcount_dec_and_test_checked(refcount_t *r);
> +extern void refcount_dec_checked(refcount_t *r);
> +
>  #ifdef CONFIG_REFCOUNT_FULL
> -extern __must_check bool refcount_add_not_zero(unsigned int i, refcount_t *r);
> -extern void refcount_add(unsigned int i, refcount_t *r);
>
> -extern __must_check bool refcount_inc_not_zero(refcount_t *r);
> -extern void refcount_inc(refcount_t *r);
> +#define refcount_add_not_zero  refcount_add_not_zero_checked
> +#define refcount_add           refcount_add_checked
> +
> +#define refcount_inc_not_zero  refcount_inc_not_zero_checked
> +#define refcount_inc           refcount_inc_checked
> +
> +#define refcount_sub_and_test  refcount_sub_and_test_checked
>
> -extern __must_check bool refcount_sub_and_test(unsigned int i, refcount_t *r);
> +#define refcount_dec_and_test  refcount_dec_and_test_checked
> +#define refcount_dec           refcount_dec_checked
>
> -extern __must_check bool refcount_dec_and_test(refcount_t *r);
> -extern void refcount_dec(refcount_t *r);
>  #else
>  # ifdef CONFIG_ARCH_HAS_REFCOUNT
>  #  include <asm/refcount.h>
> diff --git a/lib/refcount.c b/lib/refcount.c
> index d3b81cefce91..3d514f915999 100644
> --- a/lib/refcount.c
> +++ b/lib/refcount.c
> @@ -38,10 +38,8 @@
>  #include <linux/refcount.h>
>  #include <linux/bug.h>
>
> -#ifdef CONFIG_REFCOUNT_FULL
> -
>  /**
> - * refcount_add_not_zero - add a value to a refcount unless it is 0
> + * refcount_add_not_zero_checked - add a value to a refcount unless it is 0
>   * @i: the value to add to the refcount
>   * @r: the refcount
>   *
> @@ -58,7 +56,7 @@
>   *
>   * Return: false if the passed refcount is 0, true otherwise
>   */
> -bool refcount_add_not_zero(unsigned int i, refcount_t *r)
> +bool refcount_add_not_zero_checked(unsigned int i, refcount_t *r)
>  {
>         unsigned int new, val = atomic_read(&r->refs);
>
> @@ -79,10 +77,10 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
>
>         return true;
>  }
> -EXPORT_SYMBOL(refcount_add_not_zero);
> +EXPORT_SYMBOL(refcount_add_not_zero_checked);
>
>  /**
> - * refcount_add - add a value to a refcount
> + * refcount_add_checked - add a value to a refcount
>   * @i: the value to add to the refcount
>   * @r: the refcount
>   *
> @@ -97,14 +95,14 @@ EXPORT_SYMBOL(refcount_add_not_zero);
>   * cases, refcount_inc(), or one of its variants, should instead be used to
>   * increment a reference count.
>   */
> -void refcount_add(unsigned int i, refcount_t *r)
> +void refcount_add_checked(unsigned int i, refcount_t *r)
>  {
> -       WARN_ONCE(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
> +       WARN_ONCE(!refcount_add_not_zero_checked(i, r), "refcount_t: addition on 0; use-after-free.\n");
>  }
> -EXPORT_SYMBOL(refcount_add);
> +EXPORT_SYMBOL(refcount_add_checked);
>
>  /**
> - * refcount_inc_not_zero - increment a refcount unless it is 0
> + * refcount_inc_not_zero_checked - increment a refcount unless it is 0
>   * @r: the refcount to increment
>   *
>   * Similar to atomic_inc_not_zero(), but will saturate at UINT_MAX and WARN.
> @@ -115,7 +113,7 @@ EXPORT_SYMBOL(refcount_add);
>   *
>   * Return: true if the increment was successful, false otherwise
>   */
> -bool refcount_inc_not_zero(refcount_t *r)
> +bool refcount_inc_not_zero_checked(refcount_t *r)
>  {
>         unsigned int new, val = atomic_read(&r->refs);
>
> @@ -134,10 +132,10 @@ bool refcount_inc_not_zero(refcount_t *r)
>
>         return true;
>  }
> -EXPORT_SYMBOL(refcount_inc_not_zero);
> +EXPORT_SYMBOL(refcount_inc_not_zero_checked);
>
>  /**
> - * refcount_inc - increment a refcount
> + * refcount_inc_checked - increment a refcount
>   * @r: the refcount to increment
>   *
>   * Similar to atomic_inc(), but will saturate at UINT_MAX and WARN.
> @@ -148,14 +146,14 @@ EXPORT_SYMBOL(refcount_inc_not_zero);
>   * Will WARN if the refcount is 0, as this represents a possible use-after-free
>   * condition.
>   */
> -void refcount_inc(refcount_t *r)
> +void refcount_inc_chcked(refcount_t *r)
>  {
> -       WARN_ONCE(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
> +       WARN_ONCE(!refcount_inc_not_zero_checked(r), "refcount_t: increment on 0; use-after-free.\n");
>  }
> -EXPORT_SYMBOL(refcount_inc);
> +EXPORT_SYMBOL(refcount_inc_checked);
>
>  /**
> - * refcount_sub_and_test - subtract from a refcount and test if it is 0
> + * refcount_sub_and_test_checked - subtract from a refcount and test if it is 0
>   * @i: amount to subtract from the refcount
>   * @r: the refcount
>   *
> @@ -174,7 +172,7 @@ EXPORT_SYMBOL(refcount_inc);
>   *
>   * Return: true if the resulting refcount is 0, false otherwise
>   */
> -bool refcount_sub_and_test(unsigned int i, refcount_t *r)
> +bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r)
>  {
>         unsigned int new, val = atomic_read(&r->refs);
>
> @@ -192,10 +190,10 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
>
>         return !new;
>  }
> -EXPORT_SYMBOL(refcount_sub_and_test);
> +EXPORT_SYMBOL(refcount_sub_and_test_checked);
>
>  /**
> - * refcount_dec_and_test - decrement a refcount and test if it is 0
> + * refcount_dec_and_test_checked - decrement a refcount and test if it is 0
>   * @r: the refcount
>   *
>   * Similar to atomic_dec_and_test(), it will WARN on underflow and fail to
> @@ -207,14 +205,14 @@ EXPORT_SYMBOL(refcount_sub_and_test);
>   *
>   * Return: true if the resulting refcount is 0, false otherwise
>   */
> -bool refcount_dec_and_test(refcount_t *r)
> +bool refcount_dec_and_test_checked(refcount_t *r)
>  {
> -       return refcount_sub_and_test(1, r);
> +       return refcount_sub_and_test_checked(1, r);
>  }
> -EXPORT_SYMBOL(refcount_dec_and_test);
> +EXPORT_SYMBOL(refcount_dec_and_test_checked);
>
>  /**
> - * refcount_dec - decrement a refcount
> + * refcount_dec_checked - decrement a refcount
>   * @r: the refcount
>   *
>   * Similar to atomic_dec(), it will WARN on underflow and fail to decrement
> @@ -223,12 +221,11 @@ EXPORT_SYMBOL(refcount_dec_and_test);
>   * Provides release memory ordering, such that prior loads and stores are done
>   * before.
>   */
> -void refcount_dec(refcount_t *r)
> +void refcount_dec_checked(refcount_t *r)
>  {
> -       WARN_ONCE(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
> +       WARN_ONCE(refcount_dec_and_test_checked(r), "refcount_t: decrement hit 0; leaking memory.\n");
>  }
> -EXPORT_SYMBOL(refcount_dec);
> -#endif /* CONFIG_REFCOUNT_FULL */
> +EXPORT_SYMBOL(refcount_dec_checked);
>
>  /**
>   * refcount_dec_if_one - decrement a refcount if it is 1
> --
> 2.11.0
>



-- 
Kees Cook
Pixel Security

Re: [PATCH] refcount: always allow checked forms

[David Sterba]

On Tue, Jul 03, 2018 at 11:01:02AM +0100, Mark Rutland wrote:
> In many cases, it would be useful to be able to use the full
> sanity-checked refcount helpers regardless of CONFIG_REFCOUNT_FULL, as
> this would help to avoid duplicate warnings where callers try to
> sanity-check refcount manipulation.
> 
> This patch refactors things such that the full refcount helpers were
> always built, as refcount_${op}_checked(), such that they can be used
> regardless of CONFIG_REFCOUNT_FULL. This will allow code which *always*
> wants a checked refcount to opt-in, avoiding the need to duplicate the
> logic for warnings.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>

I dare to give it my

Reviewed-by: David Sterba <dsterba@suse.com>

as my POC implementations were crap and Mark's version is much better.

> ---
>  include/linux/refcount.h | 27 +++++++++++++++++-------
>  lib/refcount.c           | 53 +++++++++++++++++++++++-------------------------
>  2 files changed, 45 insertions(+), 35 deletions(-)
> 
> Dave pointed out that it would be useful to be able to opt-in to full checks
> regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> always want checks. I've spotted a few of these in code which is still awaiting
> conversion.

The motivation was code like

	WARN_ON(refcount_read(&ref));
	if (refcount_dec_and_test(&ref)) { ... }

so the warning is redundant for REFCOUNT_FULL, but I'm going to use the
_checked versions everywhere the performance of refcounts is not
critical.

Re: [PATCH] refcount: always allow checked forms

[Mark Rutland]

On Tue, Jul 03, 2018 at 11:30:38AM -0700, Kees Cook wrote:
> On Tue, Jul 3, 2018 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > In many cases, it would be useful to be able to use the full
> > sanity-checked refcount helpers regardless of CONFIG_REFCOUNT_FULL, as
> > this would help to avoid duplicate warnings where callers try to
> > sanity-check refcount manipulation.
> >
> > This patch refactors things such that the full refcount helpers were
> > always built, as refcount_${op}_checked(), such that they can be used
> > regardless of CONFIG_REFCOUNT_FULL. This will allow code which *always*
> > wants a checked refcount to opt-in, avoiding the need to duplicate the
> > logic for warnings.
> >
> > There should be no functional change as a result of this patch.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: David Sterba <dsterba@suse.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> 
> Looks good to me! Thanks for doing this. :)

Thank David; I rather stole his thunder here.

> Acked-by: Kees Cook <keescook@chromium.org>
> 
> > ---
> >  include/linux/refcount.h | 27 +++++++++++++++++-------
> >  lib/refcount.c           | 53 +++++++++++++++++++++++-------------------------
> >  2 files changed, 45 insertions(+), 35 deletions(-)
> >
> > Dave pointed out that it would be useful to be able to opt-in to full checks
> > regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> > always want checks. I've spotted a few of these in code which is still awaiting
> > conversion.
> 
> Yeah, I need to go through the cocci output -- Elena had several
> outstanding patches that never got picked up.
> 
> > I'm assuming that the atomics group is intended to own the refcount code, even
> > though this isn't currently the case in MAINTAINERS.
> 
> That's how it has landed in the past, yes, but if there is a
> dependency on these for code that will use it, maybe it should go that
> way?

That sounds reasonable to me. I was just wanted to be clear as to why I'd Cc'd
the atomics maintainers. :)

I'll spin a v2 with the fixup Andrea noted.

Thanks,
Mark.

Re: [PATCH] refcount: always allow checked forms

[Mark Rutland]

On Wed, Jul 04, 2018 at 10:46:41AM +0200, David Sterba wrote:
> On Tue, Jul 03, 2018 at 11:01:02AM +0100, Mark Rutland wrote:
> > In many cases, it would be useful to be able to use the full
> > sanity-checked refcount helpers regardless of CONFIG_REFCOUNT_FULL, as
> > this would help to avoid duplicate warnings where callers try to
> > sanity-check refcount manipulation.
> > 
> > This patch refactors things such that the full refcount helpers were
> > always built, as refcount_${op}_checked(), such that they can be used
> > regardless of CONFIG_REFCOUNT_FULL. This will allow code which *always*
> > wants a checked refcount to opt-in, avoiding the need to duplicate the
> > logic for warnings.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: David Sterba <dsterba@suse.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> 
> I dare to give it my
> 
> Reviewed-by: David Sterba <dsterba@suse.com>

Cheers!

> as my POC implementations were crap and Mark's version is much better.

Please don't think that your implementations were bad; I just already had an
idea as to what this could look like.

> > ---
> >  include/linux/refcount.h | 27 +++++++++++++++++-------
> >  lib/refcount.c           | 53 +++++++++++++++++++++++-------------------------
> >  2 files changed, 45 insertions(+), 35 deletions(-)
> > 
> > Dave pointed out that it would be useful to be able to opt-in to full checks
> > regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> > always want checks. I've spotted a few of these in code which is still awaiting
> > conversion.
> 
> The motivation was code like
> 
> 	WARN_ON(refcount_read(&ref));
> 	if (refcount_dec_and_test(&ref)) { ... }
> 
> so the warning is redundant for REFCOUNT_FULL, but I'm going to use the
> _checked versions everywhere the performance of refcounts is not
> critical.

If you will have conversion patches, do you want to pick this up as the start
of a series?

Thanks,
Mark.

Re: [PATCH] refcount: always allow checked forms

[David Sterba]

On Wed, Jul 11, 2018 at 06:49:46AM +0100, Mark Rutland wrote:
> > > Dave pointed out that it would be useful to be able to opt-in to full checks
> > > regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> > > always want checks. I've spotted a few of these in code which is still awaiting
> > > conversion.
> > 
> > The motivation was code like
> > 
> > 	WARN_ON(refcount_read(&ref));
> > 	if (refcount_dec_and_test(&ref)) { ... }
> > 
> > so the warning is redundant for REFCOUNT_FULL, but I'm going to use the
> > _checked versions everywhere the performance of refcounts is not
> > critical.
> 
> If you will have conversion patches, do you want to pick this up as the start
> of a series?

The patches where I'd use the enhanced refcounts are nice-to-have and I
don't have an ETA so it would be better if the patch gets merged
independently. Thanks.

Re: [PATCH] refcount: always allow checked forms

[Mark Rutland]

On Wed, Jul 11, 2018 at 07:37:05PM +0200, David Sterba wrote:
> On Wed, Jul 11, 2018 at 06:49:46AM +0100, Mark Rutland wrote:
> > > > Dave pointed out that it would be useful to be able to opt-in to full checks
> > > > regardless of CONFIG_REFCOUNT_FULL, so that we can simplify callsites where we
> > > > always want checks. I've spotted a few of these in code which is still awaiting
> > > > conversion.
> > > 
> > > The motivation was code like
> > > 
> > > 	WARN_ON(refcount_read(&ref));
> > > 	if (refcount_dec_and_test(&ref)) { ... }
> > > 
> > > so the warning is redundant for REFCOUNT_FULL, but I'm going to use the
> > > _checked versions everywhere the performance of refcounts is not
> > > critical.
> > 
> > If you will have conversion patches, do you want to pick this up as the start
> > of a series?
> 
> The patches where I'd use the enhanced refcounts are nice-to-have and I
> don't have an ETA so it would be better if the patch gets merged
> independently. Thanks.

Ok.

Kees, I will leave this to you.

Mark.