Re: [PATCH v2] dma-buf/heaps: system_heap: Avoid DoS by limiting single allocations to half of all memory

["T.J. Mercier" <tjmercier@google.com>]

On Thu, Apr 6, 2023 at 4:38 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 6 Apr 2023 16:27:28 -0700 "T.J. Mercier" <tjmercier@google.com> wrote:
>
> > > When you say "decide what's the largest reasonable size", I think it
> > > is difficult as with the variety of RAM sizes and buffer sizes I don't
> > > think there's a fixed limit. Systems with more ram will use larger
> > > buffers for image/video capture buffers.  And yes, you're right that
> > > ram/2-1 in a single allocation is just as broken, but I'm not sure how
> > > to establish a better guard rail.
> > >
> > > thanks
> > > -john
> >
> > I like ENOMEM with the len / PAGE_SIZE > totalram_pages() check and
> > WARN_ON. We know for sure that's an invalid request, and it's pretty
> > cheap to check as opposed to trying a bunch of reclaim before failing.
>
> Well, if some buggy caller has gone and requested eleventy bigabytes of
:)
> memory, doing a lot of reclaiming before failing isn't really a problem
> - we don't want to optimize for this case!
>
The issue I see is that it could delay other non-buggy callers, or
cause reclaim that wouldn't have happened if we just outright rejected
a known-bad allocation request from the beginning.

> > For buffers smaller than that I agree with John in that I'm not sure
> > there's a definitive threshold.
>
> Well...  why do we want to do _anything_ here?  Why cater for buggy
> callers?  I think it's because "dma-buf behaves really badly with very
> large allocation requests".  Again, can we fix that instead?
>
There are a variety of different allocation strategies used by
different exporters so I don't think there's one dma-buf thing we
could fix for slow, large allocations in general. For the system_heap
in this patch it's really just alloc_pages. I'm saying I don't think
the kernel should ever ask alloc_pages for more memory than exists on
the system, which seems like a pretty reasonable sanity check to me.
Given that, I don't think we should do anything for buffers smaller
than totalram_pages() (except maybe to prevent OOM panics via
__GFP_RETRY_MAYFAIL when we attempt to exhaust system memory on any
request - valid or otherwise).