Re: [PATCH v2 2/3] mm: prevent vm_area_struct::anon_name refcount saturation

[Michal Hocko <mhocko@suse.com>]

On Wed 23-02-22 07:36:12, Suren Baghdasaryan wrote:
> A deep process chain with many vmas could grow really high.
> With default sysctl_max_map_count (64k) and default pid_max (32k)
> the max number of vmas in the system is 2147450880 and the
> refcounter has headroom of 1073774592 before it reaches
> REFCOUNT_SATURATED (3221225472). Therefore it's unlikely that
> an anonymous name refcounter will overflow with these defaults.
> Currently the max for pid_max is PID_MAX_LIMIT (4194304) and
> for sysctl_max_map_count it's INT_MAX (2147483647). In this
> configuration anon_vma_name refcount overflow becomes
> theoretically possible (that still require heavy sharing of
> that anon_vma_name between processes).
> kref refcounting interface used in anon_vma_name structure will
> detect a counter overflow when it reaches REFCOUNT_SATURATED value
> but will only generate a warning about broken refcounter.

If I am reading the refcounter code properly the "overflow" will simply
make the ref counter frozen and the object will never be freed. A
determined attacker could leak memory like that but it would be rather
expensive and inefficient way to do so. Still good to have it covered.

> To ensure anon_vma_name refcount does not overflow, stop anon_vma_name
> sharing when the refcount reaches REFCOUNT_MAX (2147483647), which
> still leaves INT_MAX/2 (1073741823) values before the counter reaches
> REFCOUNT_SATURATED. This should provide enough headroom for raising
> the refcounts temporarily.

I am not sure this is the intended way refcounter users should avoid
overflows but I do not see other interface that would be usable. Maybe
somebody else can come up with a better suggestion but this approach
makes sense to me.
> 
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
> changes in v2:
> - Updated description to include calculation details, per Michal Hocko
> 
>  include/linux/mm_inline.h | 18 ++++++++++++++----
>  mm/madvise.c              |  3 +--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index 4bad32507570..f82085ff8a6b 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -161,15 +161,25 @@ static inline void anon_vma_name_put(struct anon_vma_name *anon_name)
>  		kref_put(&anon_name->kref, anon_vma_name_free);
>  }
>  
> +static inline
> +struct anon_vma_name *anon_vma_name_reuse(struct anon_vma_name *anon_name)
> +{
> +	/* Prevent anon_name refcount saturation early on */
> +	if (kref_read(&anon_name->kref) < REFCOUNT_MAX) {
> +		anon_vma_name_get(anon_name);
> +		return anon_name;
> +
> +	}
> +	return anon_vma_name_alloc(anon_name->name);
> +}
> +
>  static inline void dup_anon_vma_name(struct vm_area_struct *orig_vma,
>  				     struct vm_area_struct *new_vma)
>  {
>  	struct anon_vma_name *anon_name = anon_vma_name(orig_vma);
>  
> -	if (anon_name) {
> -		anon_vma_name_get(anon_name);
> -		new_vma->anon_name = anon_name;
> -	}
> +	if (anon_name)
> +		new_vma->anon_name = anon_vma_name_reuse(anon_name);
>  }
>  
>  static inline void free_anon_vma_name(struct vm_area_struct *vma)
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 081b1cded21e..1f2693dccf7b 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -113,8 +113,7 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
>  	if (anon_vma_name_eq(orig_name, anon_name))
>  		return 0;
>  
> -	anon_vma_name_get(anon_name);
> -	vma->anon_name = anon_name;
> +	vma->anon_name = anon_vma_name_reuse(anon_name);
>  	anon_vma_name_put(orig_name);
>  
>  	return 0;
> -- 
> 2.35.1.473.g83b2b277ed-goog

-- 
Michal Hocko
SUSE Labs