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

[Joel Fernandes]

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.

thanks,

 - Joel