[PATCH 2/2] staging: android: ashmem: Don't allow range_alloc() to fail.

Joel Fernandes joel at joelfernandes.org
Thu Feb 14 16:22:51 UTC 2019


On Sat, Feb 09, 2019 at 11:24:03AM +0900, Tetsuo Handa wrote:
> On 2019/02/08 23:45, Joel Fernandes wrote:
> > On Wed, Feb 06, 2019 at 08:45:32AM +0900, Tetsuo Handa wrote:
> >> Joel Fernandes wrote:
> >>> On Tue, Feb 05, 2019 at 07:28:41PM +0900, Tetsuo Handa wrote:
> >>>> ashmem_pin() is calling range_shrink() without checking whether
> >>>> range_alloc() succeeded. Since memory allocation fault injection might
> >>>> force range_alloc() to fail while range_alloc() is called for only once
> >>>> for one ioctl() request, make range_alloc() not to fail.
> >>>
> >>> Why does this not need to fail? I am worried your change will introduce
> >>> unwanted endless looping in the kernel instead of gracefully failing a
> >>> pin/unpin request.
> >>
> >> This patch won't introduce endless looping in the kernel, due to a rule called
> >>
> >>   The "too small to fail" memory-allocation rule
> >>   https://lwn.net/Articles/627419/
> >>
> >> . In short, memory allocation by range_alloc() might fail only if current thread
> >> was killed by the OOM killer or memory allocation fault injection mechanism
> >> forced it to fail. And this patch helps doing fuzzing test with minimal changes.
> >>
> >>>
> >>> Unless there is a good reason, I suggest to drop this patch from the series;
> >>> but let us discuss more if you want.
> >>
> >> We can allocate memory for range_alloc() before holding ashmem_mutex at
> >> ashmem_pin_unpin() if you don't want to use __GFP_NOFAIL. It is better to
> >> avoid memory allocation with ashmem_mutex because ashmem_mutex is held by
> >> shrinker function. But given that this module is going to be replaced shortly,
> >> does it worth moving the memory allocation to the caller?
> > 
> > Even if memory allocation does not fail right now, I still want to return the
> > error code correctly via ashmem_pin_unpin() so that if in the future there
> > are changes to the allocator algorithm, then a silent success isn't reported
> > when a failure should be reported..
> > 
> > It doesn't make sense to return success when an allocation failed.. even if
> > you are asking this code to rely on the allocator's "too small to fail"
> > behavior.. can we guarantee this allocator behavior will always exist?
> > Probably not.
> > 
> 
> Does always exist. Small GFP_KERNEL allocation without __GFP_NORETRY or
> __GFP_RETRY_MAYFAIL won't fail unless current thread was killed by the OOM
> killer or memory allocation fault injection mechanism forced it to fail.
> Failing such allocation instead of invoking the OOM killer is effectively
> equivalent to sending SIGKILL to current thread. If current thread got
> SIGKILL, current thread can't receive the error code of ioctl() from
> ashmem_pin_unpin() because userspace code is no longer executed.
> 
> MM people want to make !GFP_KERNEL memory allocations fail rather than
> retry forever. But failing small GFP_KERNEL memory allocations is such
> a horrible idea. Anyway, here is an updated patch...

I am sorry, what has changed in the updated patch? Please send clear diff
notes in your patches or at least mention exactly what changed since previous
patch revision. It is very difficult to review if you don't even mention what
changed since previous revision. Please resend the patches again.

Also I am OK with assuming small alloc success, so we are on the same page
about that.

One more comment below..

Thanks!

> From f2c8a098ebf69122fc440d9e9ca3c9cd786cce8a Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> Date: Sat, 9 Feb 2019 11:07:07 +0900
> Subject: [PATCH] staging: android: ashmem: Avoid range_alloc() allocation with
>  ashmem_mutex held.
> 
> ashmem_pin() is calling range_shrink() without checking whether
> range_alloc() succeeded. Also, doing memory allocation with ashmem_mutex
> held should be avoided because ashmem_shrink_scan() tries to hold it.
> 
> Therefore, move memory allocation for range_alloc() to ashmem_pin_unpin()
> and make range_alloc() not to fail.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> ---
>  drivers/staging/android/ashmem.c | 42 +++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index ade8438..910826d 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -171,19 +171,15 @@ static inline void lru_del(struct ashmem_range *range)
>   * @end:	   The ending page (inclusive)
>   *
>   * This function is protected by ashmem_mutex.
> - *
> - * Return: 0 if successful, or -ENOMEM if there is an error
>   */
> -static int range_alloc(struct ashmem_area *asma,
> -		       struct ashmem_range *prev_range, unsigned int purged,
> -		       size_t start, size_t end)
> +static void range_alloc(struct ashmem_area *asma,
> +			struct ashmem_range *prev_range, unsigned int purged,
> +			size_t start, size_t end,
> +			struct ashmem_range **new_range)
>  {
> -	struct ashmem_range *range;
> -
> -	range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
> -	if (!range)
> -		return -ENOMEM;
> +	struct ashmem_range *range = *new_range;
>  
> +	*new_range = NULL;
>  	range->asma = asma;
>  	range->pgstart = start;
>  	range->pgend = end;
> @@ -193,8 +189,6 @@ static int range_alloc(struct ashmem_area *asma,
>  
>  	if (range_on_lru(range))
>  		lru_add(range);
> -
> -	return 0;
>  }
>  
>  /**
> @@ -596,7 +590,8 @@ static int get_name(struct ashmem_area *asma, void __user *name)
>   *
>   * Caller must hold ashmem_mutex.
>   */
> -static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
> +static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend,
> +		      struct ashmem_range **new_range)
>  {
>  	struct ashmem_range *range, *next;
>  	int ret = ASHMEM_NOT_PURGED;
> @@ -649,7 +644,7 @@ static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
>  			 * second half and adjust the first chunk's endpoint.
>  			 */
>  			range_alloc(asma, range, range->purged,
> -				    pgend + 1, range->pgend);
> +				    pgend + 1, range->pgend, new_range);
>  			range_shrink(range, range->pgstart, pgstart - 1);
>  			break;
>  		}
> @@ -663,7 +658,8 @@ static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
>   *
>   * Caller must hold ashmem_mutex.
>   */
> -static int ashmem_unpin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
> +static int ashmem_unpin(struct ashmem_area *asma, size_t pgstart, size_t pgend,
> +			struct ashmem_range **new_range)
>  {
>  	struct ashmem_range *range, *next;
>  	unsigned int purged = ASHMEM_NOT_PURGED;
> @@ -689,7 +685,8 @@ static int ashmem_unpin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
>  		}
>  	}
>  
> -	return range_alloc(asma, range, purged, pgstart, pgend);
> +	range_alloc(asma, range, purged, pgstart, pgend, new_range);
> +	return 0;
>  }
>  
>  /*
> @@ -722,10 +719,17 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
>  	struct ashmem_pin pin;
>  	size_t pgstart, pgend;
>  	int ret = -EINVAL;
> +	struct ashmem_range *range = NULL;
>  
>  	if (copy_from_user(&pin, p, sizeof(pin)))
>  		return -EFAULT;
>  
> +	if (cmd == ASHMEM_PIN || cmd == ASHMEM_UNPIN) {
> +		range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
> +		if (!range)
> +			return -ENOMEM;

According to the too-small-to-fail rule, why are you checking for errors
here?

> +	}
> +
>  	mutex_lock(&ashmem_mutex);
>  	wait_event(ashmem_shrink_wait, !atomic_read(&ashmem_shrink_inflight));
>  
> @@ -750,10 +754,10 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
>  
>  	switch (cmd) {
>  	case ASHMEM_PIN:
> -		ret = ashmem_pin(asma, pgstart, pgend);
> +		ret = ashmem_pin(asma, pgstart, pgend, &range);
>  		break;
>  	case ASHMEM_UNPIN:
> -		ret = ashmem_unpin(asma, pgstart, pgend);
> +		ret = ashmem_unpin(asma, pgstart, pgend, &range);
>  		break;
>  	case ASHMEM_GET_PIN_STATUS:
>  		ret = ashmem_get_pin_status(asma, pgstart, pgend);
> @@ -762,6 +766,8 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
>  
>  out_unlock:
>  	mutex_unlock(&ashmem_mutex);
> +	if (range)
> +		kmem_cache_free(ashmem_range_cachep, range);
>  
>  	return ret;
>  }
> -- 
> 1.8.3.1


More information about the devel mailing list