Re: [PATCH v4 06/10] lib/refcount: Move saturation warnings out of line

[Kees Cook <keescook@chromium.org>]

On Wed, Oct 30, 2019 at 02:30:31PM +0000, Will Deacon wrote:
> Having the refcount saturation and warnings inline bloats the text,
> despite the fact that these paths should never be executed in normal
> operation.
> 
> Move the refcount saturation and warnings out of line to reduce the
> image size when refcount_t checking is enabled. Relative to an x86_64
> defconfig, the sizes reported by bloat-o-meter are:
> 
>  # defconfig+REFCOUNT_FULL, inline saturation (i.e. before this patch)
>  Total: Before=14762076, After=14915442, chg +1.04%
> 
>  # defconfig+REFCOUNT_FULL, out-of-line saturation (i.e. after this patch)
>  Total: Before=14762076, After=14835497, chg +0.50%
> 
> A side-effect of this change is that we now only get one warning per
> refcount saturation type, rather than one per problematic call-site.
> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Elena Reshetova <elena.reshetova@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Will Deacon <will@kernel.org>

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

-Kees

> ---
>  include/linux/refcount.h | 39 ++++++++++++++++++++-------------------
>  lib/refcount.c           | 28 ++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index e3b218d669ce..1cd0a876a789 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -23,6 +23,16 @@ typedef struct refcount_struct {
>  
>  #define REFCOUNT_INIT(n)	{ .refs = ATOMIC_INIT(n), }
>  
> +enum refcount_saturation_type {
> +	REFCOUNT_ADD_NOT_ZERO_OVF,
> +	REFCOUNT_ADD_OVF,
> +	REFCOUNT_ADD_UAF,
> +	REFCOUNT_SUB_UAF,
> +	REFCOUNT_DEC_LEAK,
> +};
> +
> +void refcount_warn_saturate(refcount_t *r, enum refcount_saturation_type t);
> +
>  /**
>   * refcount_set - set a refcount's value
>   * @r: the refcount
> @@ -154,10 +164,8 @@ static inline __must_check bool refcount_add_not_zero(int i, refcount_t *r)
>  			break;
>  	} while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i));
>  
> -	if (unlikely(old < 0 || old + i < 0)) {
> -		refcount_set(r, REFCOUNT_SATURATED);
> -		WARN_ONCE(1, "refcount_t: saturated; leaking memory.\n");
> -	}
> +	if (unlikely(old < 0 || old + i < 0))
> +		refcount_warn_saturate(r, REFCOUNT_ADD_NOT_ZERO_OVF);
>  
>  	return old;
>  }
> @@ -182,11 +190,10 @@ static inline void refcount_add(int i, refcount_t *r)
>  {
>  	int old = atomic_fetch_add_relaxed(i, &r->refs);
>  
> -	WARN_ONCE(!old, "refcount_t: addition on 0; use-after-free.\n");
> -	if (unlikely(old <= 0 || old + i <= 0)) {
> -		refcount_set(r, REFCOUNT_SATURATED);
> -		WARN_ONCE(old, "refcount_t: saturated; leaking memory.\n");
> -	}
> +	if (unlikely(!old))
> +		refcount_warn_saturate(r, REFCOUNT_ADD_UAF);
> +	else if (unlikely(old < 0 || old + i < 0))
> +		refcount_warn_saturate(r, REFCOUNT_ADD_OVF);
>  }
>  
>  /**
> @@ -253,10 +260,8 @@ static inline __must_check bool refcount_sub_and_test(int i, refcount_t *r)
>  		return true;
>  	}
>  
> -	if (unlikely(old < 0 || old - i < 0)) {
> -		refcount_set(r, REFCOUNT_SATURATED);
> -		WARN_ONCE(1, "refcount_t: underflow; use-after-free.\n");
> -	}
> +	if (unlikely(old < 0 || old - i < 0))
> +		refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
>  
>  	return false;
>  }
> @@ -291,12 +296,8 @@ static inline __must_check bool refcount_dec_and_test(refcount_t *r)
>   */
>  static inline void refcount_dec(refcount_t *r)
>  {
> -	int old = atomic_fetch_sub_release(1, &r->refs);
> -
> -	if (unlikely(old <= 1)) {
> -		refcount_set(r, REFCOUNT_SATURATED);
> -		WARN_ONCE(1, "refcount_t: decrement hit 0; leaking memory.\n");
> -	}
> +	if (unlikely(atomic_fetch_sub_release(1, &r->refs) <= 1))
> +		refcount_warn_saturate(r, REFCOUNT_DEC_LEAK);
>  }
>  #else /* CONFIG_REFCOUNT_FULL */
>  
> diff --git a/lib/refcount.c b/lib/refcount.c
> index 3a534fbebdcc..8b7e249c0e10 100644
> --- a/lib/refcount.c
> +++ b/lib/refcount.c
> @@ -8,6 +8,34 @@
>  #include <linux/spinlock.h>
>  #include <linux/bug.h>
>  
> +#define REFCOUNT_WARN(str)	WARN_ONCE(1, "refcount_t: " str ".\n")
> +
> +void refcount_warn_saturate(refcount_t *r, enum refcount_saturation_type t)
> +{
> +	refcount_set(r, REFCOUNT_SATURATED);
> +
> +	switch (t) {
> +	case REFCOUNT_ADD_NOT_ZERO_OVF:
> +		REFCOUNT_WARN("saturated; leaking memory");
> +		break;
> +	case REFCOUNT_ADD_OVF:
> +		REFCOUNT_WARN("saturated; leaking memory");
> +		break;
> +	case REFCOUNT_ADD_UAF:
> +		REFCOUNT_WARN("addition on 0; use-after-free");
> +		break;
> +	case REFCOUNT_SUB_UAF:
> +		REFCOUNT_WARN("underflow; use-after-free");
> +		break;
> +	case REFCOUNT_DEC_LEAK:
> +		REFCOUNT_WARN("decrement hit 0; leaking memory");
> +		break;
> +	default:
> +		REFCOUNT_WARN("unknown saturation event!?");
> +	}
> +}
> +EXPORT_SYMBOL(refcount_warn_saturate);
> +
>  /**
>   * refcount_dec_if_one - decrement a refcount if it is 1
>   * @r: the refcount
> -- 
> 2.24.0.rc0.303.g954a862665-goog
> 

-- 
Kees Cook